Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a new use_trigger_error_for_deprecations config setting #1663

Closed
wants to merge 1 commit into from

Conversation

mpdude
Copy link
Contributor

@mpdude mpdude commented May 25, 2023

This adds a new use_trigger_error_for_deprecations config setting that can be used to make doctrine/deprecations report deprecations through trigger_error.

That way, those deprecations should show up in the same place/way as other Symfony deprecations.

See #1662 for the preceding discussion.

TODO

  • Document the new setting
  • Add UPGRADING notice?

$rootNode
->children()
->booleanNode('use_trigger_error_for_deprecations')
->defaultFalse()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this default change in 3.0.0? If yes, should we deprecate not configuring this node explicitly?
As a follow-up, I think a contribution should be made to the Symfony recipe, setting it to true.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t see why it should change?

Copy link
Member

@greg0ire greg0ire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we merge like this, we close the door on having the possibility to rather use a PSR logger, or even something custom that will use getTriggeredDeprecations.

Another possibility would be to have something more complex:

deprecations:
    strategy: trigger_error|psr_logger|just_track # defaults to trigger_error
    strategy_options:
        logger: my_psr_logger # only valid when the strategy is psr_logger

@mpdude
Copy link
Contributor Author

mpdude commented May 25, 2023

That's true.

My point is that, from a Symfony user's perspective, I do not care all too much about all the options the doctrine/deprecations package offers. I understand that it needs to provide this flexibility since it is a library underlying Symfony and other frameworks.

However, in Symfony, there is a way to capture, see and log deprecation messages – and I do not see why DoctrineBundle users would want to use any other (additional) way.

So, this flag is available to say "I want to see and notice Doctrine deprecations just like I see Symfony deprecations".

If you need any other way of specialized Doctrine deprecation handling, you can still configure it yourself.

@ostrolucky
Copy link
Member

See discussion at symfony/symfony#41408 for reasons why this wasn't and won't be done in a bundle.

@mpdude
Copy link
Contributor Author

mpdude commented May 26, 2023

I find that discussion difficult to understand. If I get it right, the conclusion is that since doctrine/deprecations would be enabled (or should be possible to enable it) for all Doctrine project libraries, not only ORM or ODM, the only place suitable for such code would be a DoctrineDeprecationsBundle.

Speaking for Doctrine ORM users, those that actively go out and search for ORM deprecations will end up adding a single line to their bootstrapping code. Those who only know about (and rely on) the “Symfony way” of reporting deprecations will be surprised when they try the next major upgrade.

@mpdude
Copy link
Contributor Author

mpdude commented May 26, 2023

#1300 was discussed two years ago but did not implement anything in the end.

@ostrolucky
Copy link
Member

From what I understood it doesn't need to be a new package but bundle code could be part of doctrine/deprecations. So effort to improve this situation should go there. And yeah I guess reason why it wasn't implemented was that nobody cares enough to implement it, like usual. Btw this should be non-confiruable true IMHO, nothing else makes sense in symfony framework world.

@ostrolucky ostrolucky closed this May 27, 2023
@mpdude mpdude deleted the trigger-error-deprecations branch May 27, 2023 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants