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 integration with doctrine/deprecations library. #1323

Closed
wants to merge 1 commit into from

Conversation

beberlei
Copy link
Member

A default off integration with doctrine/deprecations, enable with:

doctrine:
    deprecations: %kernel.debug%

@beberlei beberlei added this to the 2.4.0 milestone Apr 10, 2021
@ostrolucky
Copy link
Member

Shouldn't part of this integration also be to change current deprecations in bundle to utilize this library now?

@greg0ire
Copy link
Member

Wasn't there a plan at some point to split this bundle into orm bundle and dbal bundle? What's the status? And if it's still relevant, what will happen with this configuration node? It seems overkill to create a separate repository just to be able to configure this, so maybe we should have framework integrations directly in the repository doctrine/deprecations? See here an example of how it could look: https://github.com/sonata-project/exporter/tree/2.x/src/Bridge/Symfony

@ostrolucky
Copy link
Member

I'm not actually convinced we even need configuration for this. I would just enable it automatically, because that's what symfony does

@beberlei
Copy link
Member Author

@ostrolucky symfony does not trigger so many deprecations usually, the ORM alone can easily trigger DBAL deprecations over 1000 times at the moment per request. This has real overhead.

@ostrolucky
Copy link
Member

But symfony already has mechanisms to disable deprecations in production. It also uses FingersCrossed handler by default which doesn't log anything unless error happens. Not sure it makes sense to treat deprecations coming from libraries using doctrine/deprecations differently than symfony ones. Anyways I'm not comfortable on deciding this, any opinions from eg. symfony folks on this topic?

cc @nicolas-grekas @wouterj @stof @weaverryan

@ostrolucky
Copy link
Member

Alright no input from symfony folks, so I take it that they don't think it's going to be a problem that symfony users would be spammed with deprecations. Hence we will just turn it on by default, no need for config option for now. We can resurrect this PR later, if needed.

@nicolas-grekas
Copy link
Member

I agree to not make this configurable. If someone proves that this would provide an actual benefit, this could happen later as you mention.
But for now, this should always flow to the normal path of deprecations - aka trigger_error().

@beberlei
Copy link
Member Author

beberlei commented Jun 1, 2021

I want to revisit this, we should really make this configurable because our deprecations are a bit heavier due to the Deprecation::triggerIfCalledFromOutside API.

When enabled it does a debug_backtrace call pretty early on every call even before deduplication.

This will essentially make it highly recommended to be configurable based on kernel.debug, because this method is used from such deprecations as Statement::fetch() - which can happen 100-1000s of times per request.

In addition it is clear that there is no entry point in the bridge to offer this functionality. Users that don't use DoctrineBundle should just be advised to call \Doctrine\Deprecations\Deprecation::enableWithTriggerError() in their code.

@beberlei beberlei reopened this Jun 1, 2021
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jun 1, 2021

See symfony/symfony#41408 for a longer discussion. TLDR, many think this doesn't fit either the bridge nor the bundle. My preference would be for doctrine/deprecation to ship a small bundle for this purpose.

@ostrolucky
Copy link
Member

ostrolucky commented Jun 1, 2021

Yeah, also see #1300.

We don't enable it by default, currently it's up to user. Since we don't enable it by default (and neither symfony looks interested in doing that), we don't need to consider performance.

It makes sense to provide some kind of neat configuration for symfony users by default, but it's out of scope of this bundle, because doctrine/deprecations isn't for doctrine/orm and doctrine/dbal only, but doctrine/doctrine-bundle is. In other words, symfony users shouldn't have to install doctrine-bundle in order to configure this if they don't use orm/dbal, but do use some other doctrine libraries.

Let us know your opinion on that conclusion @beberlei.

@ostrolucky ostrolucky closed this Jun 1, 2021
@ostrolucky ostrolucky removed this from the 2.4.0 milestone Jun 1, 2021
@ostrolucky ostrolucky reopened this Jun 1, 2021
@ostrolucky ostrolucky closed this Jun 3, 2021
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

4 participants