RFC: change deprecation error handler to use weak_vendors mode by default

[alcaeus]

This RFC was prompted by the recent deprecations in various Doctrine projects, which now make some peoples' tests fail (example: doctrine/orm#7306). For a Symfony project, the default value for the deprecation handler should be weak_vendors, not 0 - any deprecations within vendors are not within the scope of the application and thus shouldn't make tests fail.

With Symfony Standard Edition being deprecated and Symfony Flex being the new Gold Standard (™️) I wasn't sure where to bring this up, so I thought I'd just ask around here. Would you be in favor for a change like that, and where would we make it?

[nicolas-grekas]

I've always been quite skeptical with weak_vendor mode. My reasoning is that you call the vendor code, so in the end there is no such thing as vendor-only deprecations. The example here is a good one: thanks to the default mode, we identified quite early that Doctrine was in "WIP" state. Without this early notice, it could have been left unidentified for much longer, thus fixed much later. I think the current default encourages a constructive open-source attitude: instead of having a "not my fault" attitude, it encourages ppl to own their vendor. That also works on Symfony: since we decided to build on top of Doctrine, we own a responsibility here also and keeping eyes actively open on the project is vital for the ecosystem.

[Majkl578]

we identified quite early that Doctrine was in "WIP" state.

Can you please explain what exactly was WIP there? Two new silent (@) deprecations appeared in dev version, for two classes deprecated ~6 years ago, and some more using phpDoc annotation @deprecated.
Although I agree that in the ideal situation the usage of deprecated classes shouldn't have existed by the time, I disagree with your actions regarding #27609 (the revert + discussion) where you discarded the work solely because of not-yet-migrated usage coming from a yet another development version.

Without this early notice, it could have been left unidentified for much longer, thus fixed much later.

Yes. And everything would work as before. It's not a bug, using deprecated API doesn't inherently break anything and the deprecated API stays in place until end of minor version (semver-compatible approach).

since we decided to build on top of Doctrine, we own a responsibility here also and keeping eyes actively open on the project is vital for the ecosystem.

This is not relevant to whether weak_vendors is / should be default or not, you can opt in for such behavior regardless.

The deprecations using @trigger_error(...) are called opt-in, but with your assumptions, you are actually making opt-in deprecations opt-out. Sadly this completely breaks the original concept of opt-inness - these deprecations are no longer opt-in since you force everyone to opt-in and comply.
In the end what you are saying is in contradiction with Symfony's conventions:

Without the @-silencing operator, users would need to opt-out from deprecation notices. Silencing swaps this behavior and allows users to opt-in when they are ready to cope with them (by adding a custom error handler like the one used by the Web Debug Toolbar or by the PHPUnit bridge).

we own a responsibility here also and keeping eyes actively open on the project is vital for the ecosystem

I agree it's everyone's responsibility to keep an eye on the vital state of their dependencies.
I agree it's a common interest to forward any breakages / incompatibilities / bugs to the maintainers of the dependencies in question as soon as discovered.
I do not agree that silent/phpDoc deprecations should be something covered by any of these.

Which brings me back to:

thanks to the default mode

@ no longer means opt-in. (Even with weak_vendors, there is still a notice for deprecations happening in vendors.)

[stof]

@the usage of the listener reporting deprecation and making tests fail for them is the opting in. This listener is optional, and deprecations are not making the testsuite fail if you don't have it.

[Majkl578]

Is it really though? Only having the package installed seems it gets enabled automagically in the report everything mode: https://github.com/symfony/phpunit-bridge/blob/master/bootstrap.php

Just installed it in a project without previously having the bridge (try yourself on doctrine/orm:2.6 for example), didn't change phpunit.xml at all, but vendor/bin/phpunit suddenly ends with exit code 1 and summary of deprecations.

composer create-project symfony/website-skeleton installs symfony/phpunit-bridge automatically.

This is not opt-in.

[nicolas-grekas]

This should be considered on the recipe for the bridge I suppose.

[greg0ire]

I've always been quite skeptical with weak_vendor mode. My reasoning is that you call the vendor code, so in the end there is no such thing as vendor-only deprecations.

There should absolutely be no such thing, but unfortunately, it turns out people make mistakes, and both the caller and the callee (which triggers) will be in the same Composer package sometimes, because of careless mistakes. weak_vendors was made to help libraries avoid that kind of mistake, which results in "unfixable deprecations".

[ChangePlaces]

As I understand, the only way to silence these errors is to use an environment var <env name="SYMFONY_DEPRECATIONS_HELPER" value="disabled"/>, but I have opened as issue as this still doesn't silence them as it appears that during simple phpunit bootstrap, getenv is not accessing the phpunit.xml vars - #28028

[nicolas-grekas]

I was not sure what weak_vendor meant, but now that I checked the code and chatted a bit with @greg0ire, I'm pretty confident we should not do this: weak_vendor ignores any deprecations thrown by any vendor; it keeps only deprecations thrown by your own src/. That's not something for apps. For libs, it may make sense of course.

For this reason, I'd be 👎. (see below)

[greg0ire]

See symfony/recipes#442

[nicolas-grekas]

After a constructive discussion on Slack with @greg0ire, I updated my understanding of the topic.
What is wanted here is that CIs don't fail on deprecations. While this lowers the urge to contribute fixes to vendors that trigger these deprecations, this may be more compatible with daily job tasks.

So here we are:
symfony/recipes#442

This doesn't use weak_vendor because this mode is meant for libs. Instead it uses a maximum number of deprecations. But the end result is similar: the CI won't fail by default, and the deprecation summary will still be printed.

[alcaeus]

I believe the solution implemented now is a good compromise. People can still complain about the deprecations or (ideally) fix them upstream without the deprecations disrupting their daily workflow. Thanks @nicolas-grekas!