doctrine / DoctrineBundle

Symfony Bundle for Doctrine ORM and DBAL
https://www.doctrine-project.org/projects/doctrine-bundle.html
MIT License
4.72k stars 454 forks source link

Logging Doctrine deprecations with Symfony? #1662

Closed mpdude closed 1 year ago

mpdude commented 1 year ago

What is the easiest way to log deprecations emitted by the Doctrine codebase in the same way, same place, using the same mechanism that is used to deal with Symfony deprecations?

Is this something this bundle can help with?

greg0ire commented 1 year ago

The easiest way would be to do this I suppose: https://github.com/doctrine/DoctrineBundle/blob/e6da248067559c88792aa12320f9b11323583937/Tests/bootstrap.php#L7

I think maybe that's one of the things the bundle should do… when booting? Or maybe later, conditionally, based on a config setting? In case this might be unwanted/considered a breaking change? Or maybe it should be possible to fully configure the library with https://github.com/doctrine/deprecations#usage-from-consumer-perspective

IIRC there was at some point a discussion about splitting this repo into a DBAL bundle and an ORM bundle, so I'm not 100% sure if this is the right place, or if the doctrine bridge would preferred for this.

stof commented 1 year ago

See https://github.com/doctrine/DoctrineBundle/issues/1300#issuecomment-1563899734 for the discussion.

The dedicated bundle should do it on boot time IMO. I don't think we should expose the full config of the library. In the context of Symfony, it makes sense to always bring them with other deprecations (Symfony already allows to customize how they are logged as it uses a dedicated channel of the MonologBundle configuration)

nicolas-grekas commented 1 year ago

I'm glad this discussion finally moves forward. Instead of a bundle, what about a package that'd ship a file required by composer directly? This file would run the line that configures the deprecation lib so that it uses trigger_error. People that'd get this as a transitive dep could "replace" the lib if they'd prefer another configuration. This strategy would remove the overhead of a bundle.

greg0ire commented 1 year ago

I think that would be acceptable, since it's about a one-liner that does not need any configuration.

People that'd get this as a transitive dep could "replace" the lib if they'd prefer another configuration.

Definitely something we should document if we go that way.

@stof said it could be required by the bundle, I suppose it would be suggested in 2.x, and required in 3.x then, right? We would only be left with the hard problem of naming… doctrine/deprecations-symfony?

stof commented 1 year ago

@greg0ire we can add the new requirements in the next minor 2.x version IMO. This new package won't break projects (the Symfony deprecation tooling is precisely meant to not break things when deprecations are reported)

nicolas-grekas commented 1 year ago

I thought we might save ourself from creating any new package by allowing this to be configured via env var. I submitted https://github.com/doctrine/deprecations/pull/41 to do so.

mpdude commented 1 year ago

What was the reason for not making DoctrineBundle set up this way of error logging by default?

That there may be too many deprecation notices that would cause a performance impact?

Or that it would not cover all doctrine/* package use cases (people using doctrine/something but not DoctrineBundle)?

Or that we don’t want DoctrineBundle to configure the error reporting that might affect doctrine/* packages other than DBAL/ORM?

mpdude commented 1 year ago

Instead of a bundle, what about a package that'd ship a file required by composer directly? This file would run the line that configures the deprecation lib so that it uses trigger_error. People that'd get this as a transitive dep could "replace" the lib if they'd prefer another configuration. This strategy would remove the overhead of a bundle.

Neat technical trick, but seems completely unexpected/unusual to me and nothing I‘d favor from the DX perspective.

ostrolucky commented 1 year ago

Neat technical trick, but seems completely unexpected/unusual to me and nothing I‘d favor from the DX perspective.

I have opposite opinion. Nobody wants to have mixed approach of handling deprecations in a symfony project and that's how it is unless you enable trigger_error for doctrine/deprecations. Rest of the symfony packages don't have this configurability. Merging that would make all deprecations in symfony projects to behave consistently, which is what is expected.

What was the reason for not making DoctrineBundle set up this way of error logging by default?

Because it would not work for other doctrine bundles, such as doctrine/mongodb-odm-bundle or awaited doctrine/dbal-bundle. Hence it's a wrong project. This is why I opened a PR for symfony/doctrine-bridge, since it's something all of these share. Answer was symfony/doctrine-bridge is not generic enough. Well, in that case other existing bundles definitely ain't generic enough either.

nicolas-grekas commented 1 year ago

Should be fixed by https://github.com/symfony/symfony/pull/50468