Integrate with doctrine/deprecations

[beberlei]

As we begin to trigger deprecations through https://github.com/doctrine/deprecations/ the bundle should depend on the package and configure it to use Symfony's way of doing @trigger_error by calling the following in DoctrineBundle::boot.

\Doctrine\Deprecations\Deprecation::enableWithSuppressedTriggerError();

Maybe there should be a configuration flag to disable individual things.

[stof]

I think the bundle should keep using the Symfony way directly for things happening at build time in the bundle (in the DI extension, the Configuration class and compiler passes) and should call \Doctrine\Deprecations\Deprecation::enableWithSuppressedTriggerError(); in the boot method.
This will be the easiest way (hooks running before the build time happens won't run if the cache is not regenerated).

[ostrolucky]

That's not a good situation if we need to use mixed approach. I consider this a blocker for implementing this library. It has a global state other libraries will fight for. Solution should come from doctrine/deprecations @beberlei.

[nicolas-grekas]

doctrine/deprecations looks like a terrible idea to me, @beberlei knows it already... Global state, options, which means choices to make, which means confusion and fighting with conflicting opinions...
I'd like its existence could be reconsidered in the first place. If it really needs to exist, most options should be removed to me, so that the use of global state can be really reduced.
E.g. there could be only one global state: $logger. When not null, all deprecations go there, with context to implement any custom logic (eg filtering by packages). And when null, a silenced trigger_error() is called.

[beberlei]

How is this any different than what @tirgger_error does now? Error handling is a global problem in PHP, just have to live with it.

[beberlei]

@nicolas-grekas nobody has to make choices, it is designed to adopt the way deprecations work in the framework it nests into. Doctrine is not used by Symfony alone, and Doctrine does not have control over the error handler as Symfony does. I can understand your approach, but it does not work for Doctrine.

Symfony uses suppressed trigger errors, so we are configuring it that way.

A global logger is just the same global state with options to compete over.

[nicolas-grekas]

it is designed to adopt the way deprecations work in the framework it nests into

As @ostrolucky points out, libs will push their opinion on this configuration. Are there really such a variety of use cases btw? The way Symfony does is the most proven one. Moving this to a broader standard practice would make a big service to the PHP community IMHO.

A global logger is just the same global state with options to compete over.

The lib doesn't have to handle some concerns: in terms of decoupling/API, it would be quite different, more flexible, IMHO again (the simpler the better.)

[ostrolucky]

How is this any different than what @tirgger_error does now? Error handling is a global problem in PHP, just have to live with it.

Difference here is that if I see @trigger_error I know what's going to happen. But if I see Deprecation::trigger I don't, because what's going to happens depends on global state. That will mean in practice Deprecation::trigger call on very same file and line will have randomly different behaviour on various execution paths, because in one case library B might override this state with it's own flag, but on some other execution path library B is not triggered so nothing will override the flag.

[beberlei]

Libs didn't push their opinion on the global error handler that Symfony configures so far. Its also global state, and there is no competition around it. @ostrolucky is stating a fear that for the error handler hasn't come true either. Why is this a problem for doctrine/deprecations but Symfony is ok to pick global trigger_error?

Libs have no reason to push an opinion, they just call Deprecation::trigger and leave it up to the application developer or the framework to decide how to retrieve them.

Its perfect for a library, much less risk to introduce than what trigger_error can offer right now.

If DoctrineBundle is not enabling this approach here, Symfony will not be able to access to DBAL and ORM 2 deprecations. That is exactly what we as library want to achieve, 1.) 100% opt-in deprecations 2.) full control over side effects to users.

Please also remember that the way we deprecate things in DBAL / ORM, each request will trigger 100s of deprecations. @trigger_error in combination with logging will get easily overwhelemd by that.

[nicolas-grekas]

You should see the mess that Sentry is adding to error handling... :)
Here is an entry point with some links: getsentry/sentry-symfony#421
I'm sure we can find more examples...

[beberlei]

@ostrolucky I am not sure where you know from what is going to happen if you call @trigger_error and how the current global error handler is not considered global state. You are arguing against Deprecation::trigger with exactly the same arguments that speak against using trigger_error.

[beberlei]

@nicolas-grekas yes, that is why Doctrine is not going to introduce its own error handler. Problem solved easily on our end, its your turn to hook into what we provide.

[nicolas-grekas]

I think that @ostrolucky is saying that we'd better save ourselves from having yet another global state to manage (I'd add also yet another class to load.)

Do we have any data that supports the idea that all this needs to be configurable?

Libs have no reason to push an opinion, they just call Deprecation::trigger

I hope no lib will adopt this lib honestly...
Using @trigger_error() should be the only standard practice for runtime deprecations - it provides everything to make them actionable.

(yes trigger_deprecation() makes this even more actionable, but it's a small improvement, not a mandatory one to achieve the actionability target)

[beberlei]

I think Symfony made a mistake using @tirgger_error and our desire to follow a different approach makes that clearly visible.

trigger_error interfers with the global state and its known best practice that you should avoid that. It forces everyone to agree on exactly the same approach, when the requirements are clearly very different (see Sentry error handler).

Instead of owning up to the mistake, you are now trying to force everyone to use this approach as well.

It would have made zero difference if Symfony had used any other function, for example Symfony\trigger_deprecation and put the whole stack of logic that exists right now behind that function instead of hooking into the error handler.

We do want to integrate with Symfony's way of deprecations of course, but we also don't want make calls into global state, because they will break stuff for non Symfony error handler based applications, hence the abstraction on our end.

[nicolas-grekas]

The approach used by Symfony has served the community since 2015 with great success so far, allowing ppl to migrate across 4 major versions. I would call this a proven way - not a mistake at all.
The reason it works seamlessly is that it makes userland deprecations follow the exact same path as native deprecations. Since native deprecations have to be handled anyway, following their codepath doesn't create any divergence. It creates convergence actually.

Creating another parallel path for deprecations is what I'd call a mistake, since it now forces everyone to handle every other custom paths that libs can create (Doctrine is this case.).

[beberlei]

I don't disagree that it works great for Symfony, as a Symfony user I like it very much.

but again Symfony as a framework can claim the error handler for itself, Doctrine is just a library between many and does not have this luxury. You keep ignoring this argument and argue from your POV, when we are not in the same place as Symfony.

Also PHP is notoriusly bad with deprecating things that change, a lot of the php 8 breaks were never deprecated in 7.4, so the Symfony way would have worked just as well with a different API than trigger_error, as you cant 100% rely on php deprecations

[ostrolucky]

Let's put my fear in practical example. Let's say we call Deprecation::enableWithSuppressedTriggerError() in doctrine-bundle, but ORM (or other library) will want to use (and hence call) Deprecation::enableWithTriggerError. Would you be able to tell me now what's going to happen? Which flag wins? Because I wouldn't. I would have to first check which call in in which library is triggered first. And that might depend on vendor autoload order and hence order in composer.json etc. Not easy.

I know similar situation can happen with @trigger_error, like shown with Sentry above, but usually only error handling libraries have need for this and they do this with intention of triggering some additional action on error, not erase the current handler. Such libraries have tools at disposal to do this properly, via saving previous exception handler first and then restoring it back via restore_exception_handler. I'm not sure how would deprecation triggering library like this do the same. doctrine/deprecations (or any other similar library, like symfony/deprecation-contracts) is also aimed for much wider scope of libraries than just error handling libraries like Sentry.

For the record, I don't think we need to discuss here if we need doctrine/deprecations or not. I'm fine with using such library. I just wish for it to have more guarantees in place for more consistent behaviour than it has right now and use same deprecation triggering mechanism in whole project. For example, maybe libraries shouldn't be encouraged to call Deprecation::enableWith* calls, let userspace call this? (only example, I don't know if it's good idea, it's late and I'm tired)

[beberlei]

There is also an argument for Symfony using its own deprecation function and having the Symfony error handler triggering that when E_DEPRECATED is handley. That is the other way around than it is now, but would properly decouple Symfony deprecations from global error handling and would leave the same possibility to handle and access PHP and other triggered deprecations for Symfony tooling

[nicolas-grekas]

I think the bundle should keep using the Symfony way directly for things happening at build time in the bundle (in the DI extension, the Configuration class and compiler passes) and should call \Doctrine\Deprecations\Deprecation::enableWithSuppressedTriggerError(); in the boot method.

I agree with this. The bundle should also add lines to help with preloading, similar to these:
https://github.com/symfony/symfony/blob/10d869d8357de8005a2a0feacb36020db1a63b7f/src/Symfony/Bundle/FrameworkBundle/FrameworkBundle.php#L73-L82

[beberlei]

@ostrolucky why would ORM or a library want to force one way? It uses doctrine/deprecations to leave this decision to the user. It is the reason this abstraction exists. A library would only use deprecations to benefit from the fact that it can leave this decision to user/app/framework

[nicolas-grekas]

but again Symfony as a framework can claim the error handler for itself, Doctrine is just a library between many and does not have this luxury.

Actually, all components are libraries. We've been designing the current deprecation-reporting system with this in mind: every apps/frameworks can integrate standalone Symfony components, but all apps/frameworks need to handle native deprecations. By following the same path as them, we provide interop.

You keep ignoring this argument and argue from your POV, when we are not in the same place as Symfony.

I don't ignore it. I'm telling that there is no need for any abstraction (doctrine/annotation is not an abstraction btw, since it manages state and has an implementation).

[ostrolucky]

@ostrolucky why would ORM or a library want to force one way? It uses doctrine/deprecations to leave this decision to the user. It is the reason this abstraction exists. A library would only use deprecations to benefit from the fact that it can leave this decision to user/app/framework

Additionally to what @nicolas-grekas mentioned, my thought process was that according instructions in OG post, the way we should start to use this library in doctrine-bundle is to put Deprecation::enableWithSuppressedTriggerError(); in DoctrineBundle::boot. Maybe I unnecessarily jumped to conclusions, but that's why I thought we are not leaving this decision up to user/app/framework, but each dependency is going to start using whatever approach it wants. Because I don't consider doctrine-bundle to be a user/app/framework. Could you clarify this, then? Is doctrine-bundle some kind of exception and other libraries and bundles are not expected to call these methods?

[beberlei]

@ostrolucky ah ok, we have a misunderstanding. We expect doctrine/deprecations to be initialized exactly once by framework or application.

My assumption was that DoctrineBundle is the central entry point for everything Doctrine in Symfony, so setting this behavior here would be the only point where any of the enableWith* methods are called during a Symfony request.

I was thinking with a configuration setting on doctrine we could allow users to make changes when they want to, but this isn't expected and maybe we shouldn't even do that for now.

[ostrolucky]

Ok well if intention here is that deprecation triggering behaviour is preserved with current one and no other dependencies are supposed to change these flags, I think we can start using it here. At this point it's just a wrapper for doing @trigger_error manually so it's even ok to use it during container compilation...

[nicolas-grekas]

Are you really sure about this? This is a Symfony bundle, where a deprecation framework already exists. Integrating with the native API of the framework will lead to better DX I think, as eg strack traces and messages will be less noisy.

[ostrolucky]

At the same time it's library providing integration with doctrine libraries, where a deprecation framework also already exist and is used, eg. in doctrine/dbal, so such reasoning is not going to cut it. If you mind inconsistency, it's already there because other doctrine libraries are not using symfony/deprecation-contracts. And all it takes for integration with native API of the Symfony framework is just a flip of the switch. Not sure what you mean with strack traces.

[nicolas-grekas]

what you mean with strack traces

I mean that eg:
https://github.com/doctrine/deprecations/blob/d12b33583aa1d591d8c45d31812df46d8d19e579/lib/Doctrine/Deprecations/Deprecation.php#L180

Because it provides a bit degraded DX, doctrine/deprecations should be kept for Doctrine standalone packages, not for this bundle.

[ostrolucky]

It shows where is the deprecation triggered from and last caller, not a full stack trace. I wouldn't call that noisy but useful, especially for people who don't log full log context. Symfony deprecation library also doesn't show any url or identifier associated with deprecation, which is factually worse DX.

[nicolas-grekas]

I'm not against improving the existing framework, quite the contrary actually!
But DX consistency is paramount.

[ostrolucky]

Yeah but consistency won't be there no matter what because doctrine packages use doctrine/deprecations.

[ostrolucky]

But maybe it should be consistent within the project at least. Using doctrine/deprecations would mean bundle would be forced to use combined approach due to symfony API. Eg ->setDeprecated on symfony/config will use trigger_error. That's compatible with our test cases where we rely on symfony/phpunit-bridge features and in order to support these APIs we would have to continue supporting these, but at the same time also support doctrine/deprecations way of handling test cases, which might be confusing.

More feedback welcome.

[alcaeus]

This bundle integrates Doctrine into the Symfony framework, not the other way around. For that reason, users of this bundle expect to have the Symfony way configured out of the box. This bundle should announce its own deprecations using whatever mechanism it wants, but by default all deprecations should be sent to trigger_deprecation so that users have the same experience they have for the rest of the Symfony ecosystem.

[ostrolucky]

This bundle should announce its own deprecations using whatever mechanism it wants, but by default all deprecations should be sent to trigger_deprecation so that users have the same experience they have for the rest of the Symfony ecosystem.

So what do you expect should happen with DBAL and other doctrine packages? It doesn't use trigger_deprecation, so such deprecations won't go through trigger_deprecation function for doctrine-bundle users either. Those are unrealistic expectations at the moment. Or did you do a typo and meant trigger_error? I explained that this part should be in doctrine-bridge.

[nicolas-grekas]

I explained that this part should be in doctrine-bridge.

you made some allusions to the idea, but how would that work in practice? the bridge doesn't "run" anything, it's just a lib. Only bundles hook into the runtime flow. Can you explain your idea a bit more? Maybe draft some PR?

[ostrolucky]

See symfony/symfony#41408

[ostrolucky]

Plan is now following:

• Use trigger_deprecation here for our deprecations (I explained why in Integrate with doctrine/deprecations #1300 (comment))
• If set up of default mode is desired, it should be done in one of the symfony packages (doctrine-bridge still doesn't look to be generic enough and my PR most likely won't pass, so I'll let symfony folks decide to which more generic package it should go to)

[mpdude]

Looks like I am exactly two years late to the party.

Seems I do not have a broad enough perspective myself, so I miss the point of most reasons given above.

From my POV as a Symfony + Doctrine + DoctrineBundle user, I don’t get to see Doctrine deprecations in the place where I see all/most other Symfony or *Bundle-related deprecation messages.

If I get symfony/symfony#41408 right, the last suggestion is that someone create a DoctrineDeprecationsBundle that users can install/activate to capture Doctrine deprecations as well.

Majority of users out there probably unaware of Doctrine-related deprecations. There’s not even a configuration setting/switch mentioned anywhere that they could flip.

[greg0ire]

I think you're right. We should provide a bundle, either in a separate package, or embedded right in the lib, a bit like this: https://github.com/greg0ire/enum/tree/stable/src/Bridge/Symfony

I think I'd be more in favor of a separate package for this particular case, even if it is tiny, that way:

• no optional deps
• there can be one package for Symfony, one for Laminas, one for Laravel, and we don't have a package that grows too much with the list of popular frameworks.

[stof]

I also suggest a separate package. And doctrine/doctrine-bundle could depend on that package so that users of DoctrineBundle will automatically have it (but they can also install it directly for other Doctrine libraries)

[mpdude]

And doctrine/doctrine-bundle could depend on that package

How can a bundle depend on and activate another bundle?

[stof]

symfony/flex already activates bundles automatically when installed. For the depend on, that's what composer.json is about.

[mpdude]

Probably implemented by symfony/symfony#50468