Idea: Support Symfony's deprecation-contracts

[Toflar]

Hey Markus,

Nice extension! I'm not sure if I'll ever need it but the first thing that came to my mind when I read about the version support, was support for Symfony's trigger_deprecation() which basically also consists of a package name and a version. So devs wouldn't even need to check the deprecations anymore, your extension would already detect it during static analysis 😉

Anyway - just an idea and I thought I'd leave it here.

Happy Holidays!

[staabm]

Hey Yanick,

Could you point me to a concrete code sample?

I don't know these symfony things well :-)

[Toflar]

Oh, of course - sorry. I thought you did, my bad! Basically Symfony provides a trigger_deprecation() function that can be used anywhere in your code to log/report/do-whatever with the data.

See https://github.com/symfony/deprecation-contracts.

So you'd write trigger_deprecation('symfony/blockchain', '8.9', 'Using "%s" is deprecated, use "%s" instead.', 'bitcoin', 'fabcoin'); - meaning the first argument is the Composer package and the second one is the version.

If your phpstan extension would support this, all Symfony users would instantly know if they forgot to remove a deprecated feature before releasing a new major version 😎

[staabm]

I had a look into it. I don't think this one is doable, as I don't see a way how phpstan should know under which conditions trigger_deprecation is executed.

---

see e.g.

https://github.com/symfony/symfony/blob/272f021c2f1962086910bb28d1d76bcb7b623cd9/src/Symfony/Component/Form/Extension/Core/Type/UrlType.php#L41-L48

the deprecation will only be triggered, when no default protocol is defined in the using app.

---

every deprecation has different semantics under which circumstances it will be triggered.
it does not only depend on the args given to the trigger_deprecation but also when the acual line of code will be executed.

when the codebase contains trigger_deprecation('symfony/blockchain', '8.9', 'Using "%s" is deprecated, use "%s" instead.', 'bitcoin', 'fabcoin'); it does not mean the app using a library which contains this line will actually trigger the deprecation.

I will close for now, but feel free to provide more context/feedback in case I missed something

[Toflar]

First of all, thanks for looking into it and spending time on an idea I wrote down! ❤️

Maybe we've misunderstood each other slightly: Yes, you cannot know when a trigger_deprecation call is executed.

Imagine this: We have a branch named 8.x where we're working on version 8 of our app. There, we introduced the deprecation you mentioned before: trigger_deprecation('symfony/blockchain', '8.9', 'Using "%s" is deprecated, use "%s" instead.', 'bitcoin', 'fabcoin')

Now we start working on version 9 of our app. So we start a new branch for 9.x and configure your extension in a way that it knows this branch is now going to be 9.0
The extension could now tell me that everywhere where we use trigger_deprecation with a version < 9 this is code that needs to be removed. It's a todo 😊 The builds for 9.x can only be green once I removed our example from the code base. It doesn't matter if the code is executed or not in this case - it's a piece of code that must be removed before a release in any case.

[staabm]

Imagine this: We have a branch named 8.x where we're working on version 8 of our app. There, we introduced the deprecation you mentioned before: trigger_deprecation('symfony/blockchain', '8.9', 'Using "%s" is deprecated, use "%s" instead.', 'bitcoin', 'fabcoin')

in your example symfony/blockchain is the current project which I am working on, not a dependency defined via composer.json, right?

[Toflar]

Exactly, yes :)

[staabm]

Hmm do you really use this mechanism to declare deprecations for your own package?

I feel the trigger_deprecation use-case is more about informing users of your lib about deprecations.

Managing the "todos" of your very own project feels more natural using regular TODO comments..?

[staabm]

In #86 I have implemented it for composer dependencies but I am not yet sure thats useful either 🥸

[Toflar]

Love it! ❤️

Hmm do you really use this mechanism to declare deprecations for your own package?
I feel the trigger_deprecation use-case is more about informing users of your lib about deprecations.
Managing the "todos" of your very own project feels more natural using regular TODO comments..?

Of course, we could do this as well. Just means that everywhere where we deprecate an API using trigger_deprecation(), we also have to add a // TODO: >=9.0 This has to be removed before releasing 9.0. Basically, trigger_deprecation() is just the alternative to adding a TODO comment. It just provides additional info for other developers on top.

[staabm]

Released #86 with experimental support.

https://github.com/staabm/phpstan-todo-by/releases/tag/0.1.24

please give it a try

[Toflar]

Tried it and it reports all trigger_deprecation() calls perfectly! ❤️

The only problem left before I could create a PR add your extension to our CI workflow is which version it compares it to. I would need a way to determine the version based on the branch name. Why?

We have version branches like so:

• 4.9 (LTS version)
• 4.13 (LTS version)
• 5.3 (LTS and current version)
• 5.x (future version 5.4 that we work on)

So my PR would go against 5.x. Right now, all of our trigger_deprecation() calls are reported by your extension. Which is wrong. Here's how we use it for example:

trigger_deprecation('contao/core-bundle', '5.0', 'Using the "contao_figure" Twig function has been deprecated and will no longer work in Contao 6. Use the "figure" Twig function instead.');

I would need to be able to tell the extension that contao/core-bundle is my package (could be more than one, think of a monorepository) and the version we're working on in this branch is 5.4 (or if reading the branch name and common patterns like 5.x could be supported, that would be awesome, then I don't have to configure anything at all).
Meaning that only by the time we create a 6.x branch, we want all these issues to be reported. Because now they have to be removed from the code base :)

[staabm]

we have a "referenceVersion" for other rules.

maybe we should use that when trigger_deprecation is called with a 1st param which matches the current composer.json name

[Toflar]

In our case this would not work as we have a monorepository, so it should match for multiple package names (which is a general concept that might be interesting because imho monorepos are pretty common). Yes I actually tried referenceVersion but then saw that this is not considered and also it works with tags but in our case we'd need branch names, probably.

[staabm]

Maybe you could filter the errors using phpstan ignore rules based on the message?

[Toflar]

Yeah, maybe our use case is too specific so a custom rule would be better anyway 🤔

[staabm]

yeah, your use case seems pretty specific. if you can make it work with phpstan ignore patterns and the rule we ship in todo-by, you can go for it. otherwise it might make sense to take inspiration from the rule I have implemented and customize it for your specific needs

[Toflar]

I did, thanks a lot for your help Markus. Should I ever have the pleasure to meet you in person, I'll owe you one 😊

[github-actions]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.