Disallow trigger_error(), which adds dangerous side-effects to PHP projects

[Ocramius]

As mentioned in detail in the patch, and experienced first-hand in at least 5 real-world projects
so far, plus numerous packages within the doctrine ecosystem itself:

Do not use runtime errors as a way to convey deprecations to users.
Warnings, notices, and errors in general (which aren't exceptions) are not usable
in downstream projects, and propagate to global error handlers, causing massive issues
in anything relying on STDOUT, STDERR, aggressive logging, or just expects decent performance
from a dependency. In addition to that, introducing additional runtime effects is a potential
BC break

I don't remember when we agreed adding any @trigger_error() calls to the codebase, but I remember
for sure that I stated (clearly) that @deprecated is my preferred way, and even with its own limitations,
it still is much simpler to deal with downstream (and from a maintenance PoV).

[morozov]

I'm all for it. The need to temporarily un-deprecate things looks like madness to me (doctrine/persistence#87).

[alcaeus]

Strong 👎 from me: it is currently the only meaningful way we have to properly communicate those deprecations. Until there is a comparable alternative, we can’t merge this.

[Ocramius]

Until there is a comparable alternative, we can’t merge this.

I think a better alternative can be researched. I've already started work on tools like roave/you-are-using-it-wrong, but indeed, the current approach is a mess, and it is only spawning work. It is not a solution: it's only problems, and I'm not talking about just doctrine/* work, but work I do in closed source software, where I literally got called out a few times for breaking stuff (I'm still seen as "the doctrine guy" in some places).

Overall, I want trigger_error() and all runtime warnings to be gone from any future work inside the @doctrine organisation.

[alcaeus]

Once an alternative has been researched and is available, we can rediscuss this. Until then, it’s a firm “no” from me.

[Ocramius]

The alternative is @deprecated, and it worked fine so far. We can also prefer duplicating methods, and adding @deprecated on the deprecated branches: this forces better design overall, and would remove the need for consumers to care about deep execution paths inside our sources (not their problem).

Heck, if I'm 10 levels in a stack trace, and a deprecation is caused by two interacting dependencies, that's not useful information anyway.

[alcaeus]

There is not enough widespread tooling for dealing with deprecation annotations (yet). I remember discussing this with the doctrine core team a while ago and since then nothing has changed. Back then it was thought that phpstan-deprecation-rules would be able to do this, but it didn’t gain enough traction, so we’re still at the same point.

From personal experience dealing with significant BC breaks in ODM 2.0, not everything can be expressed using a deprecation annotation, and often the hoops one would have to go through to accomplish that would be too severe to justify it.

[Ocramius]

Not everything has to be expressed, there can be documentation about it, and it would still be better than introducing more side-effects, especially runtime ones.

[alcaeus]

There are better ways to communicate BC breaks than upgrade notes. I’m sure there’s a reason this has worked for Symfony for more than 5 years now, whether you agree with it or not.

[Ocramius]

Yeah, I disagree with many symfony things, including this one. The fact that symfony is dragged in the discussion is irrelevant anyway, since that's a separate and independent organisation.

[alcaeus]

You may disagree, but having updated multiple applications from one Symfony major to the next, the tooling provided there makes it a lot easier than reading upgrade notes and going code-hunting. If it didn’t work, the team would try something different.

[Ocramius]

It already didn't work here. Marco Pivetta http://twitter.com/Ocramius http://ocramius.github.com/

…

On Fri, Jan 10, 2020 at 6:39 PM Andreas Braun ***@***.***> wrote: You may disagree, but having updated multiple applications from one Symfony major to the next, the tooling provided there makes it a lot easier than reading upgrade notes and going code-hunting. If it didn’t work, the team would try something different. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#150?email_source=notifications&email_token=AABFVEB75H5QHSJQWRKW2JTQ5CXDTA5CNFSM4KFKOEV2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIUVFNY#issuecomment-573133495>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABFVEHKRTTFW7XYU6VLUNLQ5CXDTANCNFSM4KFKOEVQ> .

[alcaeus]

What do you mean “it didn’t work here”? If you’re referring to persistence, we chose to revert because we need to coordinate releases if we want to avoid raising errors due to deprecations. Using a different solution (e.g. the phpstan package mentioned earlier) would’ve yielded the same result, regardless of how we deprecated it. The problem wasn’t that we deprecated code, but that we didn’t update our own downstream libraries soon enough. Reverting the deprecations just happened to be the quicker fix than merging and releasing a pull request in ORM.

[ostrolucky]

@deprecated can't be used for conditional depreciations, eg deprecating setting certain value of argument. Nuff said. People complain about doctrine depreciations because doctrine got sloppy and regularly deprecates stuff while still using it.

[malarzm]

For the same reason @alcaeus and @ostrolucky - it's not entirely @trigger_error's problem that lead to revert, it's slow adoption throughout our own organization. Also, as I mostly develop in Symfony, these notices are extremely useful as they pop out in our test suite and allowing us to find usage of deprecated stuff than way easier than any upgrade note would. I'd agree with changing course if there'd be any better solution proposed/available, but there's none.

[Majkl578]

Strong 👍 from me as a former OSS maintainer. ❤️

[beberlei]

👎 from me. We need the trigger_error deprecations to guide users to new APIs, @deprecated is not enough especially with the large scale changes that we do.

The temporary undeprecate in persistence was a learning, that we shouldn't deprecate stuff without having alternatives in place for ourselves.

trigger_error(E_USER_DEPRECATED) is the exact use case for what we want to do. I have seen several times how helpful the runtime generated notices are to upgrade code basis vs @deprecated, because you can virtually guarantee a breakless upgrade with preprations based on this. I would really like us to go this way forward, especially with the breaks that are still coming.

[greg0ire]

I would be fully on board with forbidding a trigger_error without an @

Warnings, notices, and errors in general (which aren't exceptions) are not usable
in downstream projects, and propagate to global error handlers, causing massive issues
in anything relying on STDOUT, STDERR, aggressive logging, or just expects decent performance
from a dependency. In addition to that, introducing additional runtime effects is a potential
BC break

Isn't that what the @ is for?

I mean https://3v4l.org/o1E45

Your point about performance still stands though, since apparently deprecations are logged in production in what's probably the most likely configuration of our users' servers right now: https://github.com/symfony/recipes/blob/master/symfony/monolog-bundle/3.1/config/packages/prod/monolog.yaml , but I think if we are more careful about fixing deprecations on our side before propagating them to the rest of the ecosystem, we might be actually fine.

I have still no tried phpstan/phpstan-deprecation-rules which I believe is great, but it looks like I'm not the only one ( 702 151 installs vs 28 068 237 installs for the PHPUnit bridge). I know the bridge is not your favorite project, but right now it's fair to say it's one of the biggest channels through which people get their deprecations, the other being the Symfony profiler.

[lcobucci]

This is the best move IMHO. Deprecations aren't errors, they shouldn't affect test suites, and we shouldn't create a sense of urgency to make users run and update stuff.

The fact that we don't have the decent tooling spread enough shouldn't be the excuse for us to settle on a sub-optimal solution.

I honestly had to modify our global error handler to ignore Doctrine's deprecation alerts because they were messing with our logging.

I believe we should provide enough documentation to help people upgrade and not affect runtime.

[alcaeus]

To elaborate on some of the issues people have mentioned, the first step is to always deprecate things in documentation. This includes marking deprecated members, classes, interfaces, etc. as @deprecated in docblocks and documenting the BC break in the UPGRADE.md file. The deprecation notice should also help the user understand what they should do in place. This may be very terse in the docblock, but the upgrade notes can elaborate a bit. This is the absolute bare minimum that we MUST do to ensure people aren't left in the dark about deprecations. Regardless of whether we use @trigger_error or not, this has to be done.

Once code is deprecated, the question is how users notice these deprecations. Users could go through the upgrade file and then check all their code, but that's a very time-consuming and error-prone process. Tools like phpstan-deprecation-rules can help with this but aren't very widespread. Even though the Doctrine Core Team "decided" to push this tool a few years back, not even all our own packages are using it consistently. Leaving that aside, using these tools makes the process of finding deprecated code usages a lot easier and can even help fixing it if done right (e.g. by pairing it with a set of rules for Rector.

The aforementioned way of marking things as @deprecated may not always be enough. Especially with the current push to change to strictly typed interfaces, it isn't very easy to handle these deprecations. When moving from @param string $foo in the docblock to string $foo in the argument list, the strict_types directive (which is required by this coding standard) makes for a BC break in the making, as the argument no longer applies stringables. These occurrences could be solved by triggering a warning of some sort that the method is called with a string able which may no longer work. In addition to that scenario, larger applications may not always allow for complete coverage of deprecation via static inspection, e.g. because production config hits different code paths than the development config. Again, this is where highlighting deprecations in a dynamic way comes in. Whether these are logged or ignored is up to the user, which is ensured by making them "silent" by default using the error suppression operator (Note, this was missing in the first iteration that Symfony did with trigger_error which caused a lot of complaints from users).

I have mentioned listening to deprecations before. One argument that was brought up was that the deprecations create a sense of urgency to upgrade the code. This could be used as an argument against deprecating code in the first place, but not against using trigger_error: using an alternative solution to handle @deprecated annotations (like the tool mentioned earlier) creates the same sense of urgency to act on the deprecations - even more so as the tool will fail the phpstan run entirely and thus cause a build failure. For both phpstan-deprecation-rules and phpunit-bridge, there are ways to ignore certain deprecations, ignore vendor code that calls deprecated code paths, or ignore deprecations entirely by not using the tool.

One complaint that was mentioned is that these deprecations aren't errors, so they shouldn't be logged at all, shouldn't cause emails to be sent out, and so on. While the name trigger_error suggests you're triggering an error, the level we use CLEARLY isn't an error level and shouldn't be treated as such. If your logging facility sends out an email or SMS push when anything is logged with ANY severity, your logging is set up incorrectly. You shouldn't blame other people's code for that. If your sysadmin configured your server to send you an email on every INFO log line sent to the syslog, you'd tell him to stop doing that immediately. Same goes for the PHP log. It's a tool to be used, and we're using it to provide something of value to the user.

With all that said, I'm closing this PR. We should strive to improve the way we deprecate code and how users are alerted to these deprecations, but this suggestion is a step backwards as it doesn't offer a solution to the problem at hand, it just tries to sweep it under the rug.

[Ocramius]

Sorry, but no.

First of all, don't go around closing issues because you disagree with them, especially since we have diverging opinions on this thread.

Second, the trigger_error() approach does not work, and has led to a lot of work in the past (not just the last few weeks) whenever we had to introduce new symfony release support.

You could say that every *.4 release of symfony has been a major BC break for the ecosystem, since it would break builds all across the ecosystem, even if you have it 4 levels deep in a dependency tree.

The typical upgrade process is:

1. symfony does whatever the hell they usually do
2. the downstream ecosystem buzzes with pull requests and unnecessary work to bring builds back to green
3. maintainers are pressured to provide an "upgrade path" for things that aren't broken anyway

Goes hand-in-hand with what @lcobucci said: unnecessary/artificial urgency.

PHP has the same issue: an upgrade that introduces a deprecation leads to much more added work just because of runtime side-effects that can indeed lead to chaos.

The issue is about a fundamental design issue in using trigger_error() to signal something that is, in fact, not an error: we are introducing global state mutations, as well as additional I/O, to report some messages to a consumer that is completely out of our control. Libraries, especially widely used ones, should never introduce dangerous runtime effects, and if they do so, that is a clear BC break anyway.

You can ask consumers to "change their ways" dozens of times, but how the consumers are configured is completely out of our control (and it should be like that), and introducing global stateful changes (STDERR/STDOUT/error handling) where there previously weren't is a clear BC break.

[alcaeus]

First of all, don't go around closing issues because you disagree with them, especially since we have diverging opinions on this thread.

A significant part of the Doctrine Core team disagrees with this PR, which is why I closed it. If you'd rather keep it open, that's perfectly fine.

Maybe I didn't word it clearly enough in my comment above, but the work isn't because we use trigger_error, it's because we deprecate things. It doesn't matter how we deprecate code, people will always have to change their code to be able to upgrade. The only way to not create this urgency is to not deprecate code at all.

But if we go and deprecate code (which we definitely should) for removal in future versions, we need to ensure that users have a fair chance of reacting to those deprecations. @deprecated is one of the tools at our disposal. Upgrade notes is a tool at our disposal. @trigger_error() is another tool at our disposal. Forward compatibility layers (so people can remove deprecated code paths and use non-deprecated alternatives) is yet another tool we can use. Before we claim "artificial urgency" as a reason to not announce deprecations, we should ensure that we have a decent process for users to deal with any deprecations we make, regardless of how we announce them. To use an actual example, ORM 2.x deprecated the entire proxy layer based on doctrine/common. ORM 3.0 removed it and introduced the proxy layer based on ProxyManager. The problem isn't with deprecating legacy proxies. The problem isn't announcing that deprecation using @deprecated, upgrade notes, or even @trigger_error(). The problem is that users can't do anything about that deprecation until they upgrade to 3.0, at which point they'll encounter a fatal error. Instead of ideological discussions around banning use of certain functions outright, no matter what they're used for, we should be focussing on ensuring decent upgrade paths for our users. This discussion and a potential removal of announcing deprecations via @trigger_error() does not help our users deal with the deprecations we make at all. It hides them from our users only to bring them back when PHP inevitably throws a fatal error in their face when they upgrade to the next major release.

I've said it before, and I'll say it again: until we have a proper solution in place to deal with deprecations, we can't merge this. The paragraph above shows that we have more pressing issues than this, and I would suggest we start focussing our energy on that.

[ostrolucky]

First of all, don't go around closing issues because you disagree with them, especially since we have diverging opinions on this thread.

It wasn't closed because @alcaeus disagrees with it. Most members disagree with this proposal so won't be merged and that's it.

Second, the trigger_error() approach does not work, and has led to a lot of work in the past (not just the last few weeks) whenever we had to introduce new symfony release support.

Source? What I saw was that patches for supporting new symfony releases were minimal. It was mess in doctrine ecosystem causing problems, eg. supporting new doctrine persistence release. Also not seeing how it's relevant to trigger_error, since we don't opt-in into collecting these. They could actually help us if we did opt into them.

3. maintainers are pressured to provide an "upgrade path" for things that aren't broken anyway

You are arguing against upgrade path, am I getting it correctly?

introducing global stateful changes (STDERR/STDOUT/error handling) where there previously weren't is a clear BC break.

BC break is whatever we define is. Renaming private function is also technically BC break, since users can be accessing it through reflection. But we don't care about such usage. Similarly, we don't consider "bc break" something for which users have to hook into global error handler. This is opt in behaviour and these users want test suite to break if they use deprecated functionality. I would argue we are breaking their CI setup if we stop using trigger_error, because it will no longer alert them when they do use deprecated functionality, hence their setup no longer working as intended.

PHP has the same issue: an upgrade that introduces a deprecation leads to much more added work just because of runtime side-effects that can indeed lead to chaos.

So PHP disagrees with your stand as well. Runtime deprecation notices (which are easy to enable/disable) are very effective in letting consumers (who are interested into that) know that they need to update something in order for it to work in future php versions. Otherwise people would have to do massive work when doing major upgrades. I don't see why doctrine should be different than rest of the PHP ecosystem.

[localheinz]

How do you feel about symfony/symfony#35526?

[Ocramius]

Use `global $deprecations[] = 'the deprecation';`. It's still a side-effect, but at least it's not a mess that has hooks into it, and most certainly not hooks that interact with your current runtime. If you really must log these, you can then add a shutdown handler, which can indeed operate in a procedural/unsafe way.

…

On Wed, Feb 5, 2020, 12:46 Andreas Möller ***@***.***> wrote: How do you feel about symfony/symfony#35526 <symfony/symfony#35526>? — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#150?email_source=notifications&email_token=AABFVEGH65GQVRGLWBRCGJTRBKRK5A5CNFSM4KFKOEV2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEK3EMSI#issuecomment-582370889>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABFVEATYK44URL2VUPOXX3RBKRK5ANCNFSM4KFKOEVQ> .