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

Logging Doctrine deprecations with Symfony? #1662

Closed
mpdude opened this issue May 25, 2023 · 10 comments
Closed

Logging Doctrine deprecations with Symfony? #1662

mpdude opened this issue May 25, 2023 · 10 comments

Comments

@mpdude
Copy link
Contributor

mpdude commented May 25, 2023

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
Copy link
Member

greg0ire commented May 25, 2023

The easiest way would be to do this I suppose:

Deprecation::enableWithTriggerError();

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
Copy link
Member

stof commented May 26, 2023

See #1300 (comment) 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
Copy link
Member

nicolas-grekas commented May 26, 2023

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
Copy link
Member

greg0ire commented May 26, 2023

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
Copy link
Member

stof commented May 26, 2023

@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
Copy link
Member

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

@mpdude
Copy link
Contributor Author

mpdude commented May 27, 2023

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
Copy link
Contributor Author

mpdude commented May 27, 2023

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
Copy link
Member

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
Copy link
Member

Should be fixed by symfony/symfony#50468

nicolas-grekas added a commit to symfony/symfony that referenced this issue May 30, 2023
…ations as expected (nicolas-grekas)

This PR was merged into the 6.3 branch.

Discussion
----------

[FrameworkBundle][PhpUnitBridge] Configure doctrine/deprecations as expected

| Q             | A
| ------------- | ---
| Branch?       | 6.3
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix doctrine/DoctrineBundle#1662
| License       | MIT
| Doc PR        | -

Following doctrine/deprecations#41, so that deprecations from Doctrine take the same code path as any other deprecations in Symfony apps.

Submitted to 6.3 to notify about the deprecations as early as possible but not too early to not trigger new deprecations in existing apps.

Commits
-------

cbe4808 [FrameworkBundle][PhpUnitBridge] Configure doctrine/deprecations as expected
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

No branches or pull requests

5 participants