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

Allow configuring behavior via the DOCTRINE_DEPRECATIONS env var #41

Merged
merged 1 commit into from
May 29, 2023
Merged

Allow configuring behavior via the DOCTRINE_DEPRECATIONS env var #41

merged 1 commit into from
May 29, 2023

Conversation

nicolas-grekas
Copy link
Member

In doctrine/DoctrineBundle#1662, we're discussing about how we should best integrate this lib in Symfony, where we'd like the default to be to call trigger_error.

In order to not force autoloading the Deprecations class on every single request just to configure it, I propose to allow configuring the default behavior via the DOCTRINE_DEPRECATIONS env var.

README.md Outdated Show resolved Hide resolved
@greg0ire
Copy link
Member

Have you considered using a file auto-loaded with autoload.files, that would contain a check followed by appropriate calls to Deprecation::enableWith*? I feel it would result in less complexity.

@stof
Copy link
Member

stof commented May 26, 2023

@greg0ire this would still force to load the Deprecations class on all requests even if they never trigger a deprecation.

Copy link
Member

@derrabus derrabus left a comment

Choose a reason for hiding this comment

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

I like the idea! This will enable Symfony to ship a very simple recipe for integrating deprecations triggered by Doctrine packages into its own error handler.

@beberlei, what do you think?


Oh, and: can we cover this change with a test? 😬

lib/Doctrine/Deprecations/Deprecation.php Outdated Show resolved Hide resolved
@derrabus derrabus requested a review from beberlei May 26, 2023 09:51
@ostrolucky
Copy link
Member

With this option we would still have a problem about which package should set value of this env var by default. I don't think every user should configure this.

@mpdude
Copy link

mpdude commented May 27, 2023

☝🏻 just wanted to add this as well. Also, a bit surprising from a user‘s perspective since no other bundle/package/lib (that I am aware of) require setting env vars to have deprecations logged.

Not saying that it’s not a nice feature to be able to turn this on via env vars, just like other software allows to turn on error logging/debugging in a similar way.

Copy link
Member

@beberlei beberlei left a comment

Choose a reason for hiding this comment

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

Good improvement

@greg0ire
Copy link
Member

With this option we would still have a problem about which package should set value of this env var by default.

Maybe the idea was to use symfony/flex with an env configurator, in a recipe associated with doctrine/deprecations?

@ostrolucky
Copy link
Member

That would add the line to .env file, wouldn't it? That makes sense for options that user often need to change, but here, I can't see why would any symfony user want to opt out from this, seeing they can't opt out from this behaviour in core symfony components, those always do @trigger_error. Is there some use case I am missing where you want to have mixed deprecation trigger mechanisms in a project? Maybe I am too pedantic here, in the end it's better than nothing. But i see this env var as something that every symfony frramework user will have in their .env file but nobody will want to change.

@greg0ire
Copy link
Member

Quite unlikely to be changed indeed. Maybe the intended usage is something else, let's see.

@stof
Copy link
Member

stof commented May 29, 2023

@ostrolucky if you want a no-configuration system for Symfony projects, it would require changing the default reporting mode of Doctrine to match the Symfony one. As long as defaults are not the same, we cannot have 0 configuration.

@nicolas-grekas
Copy link
Member Author

Test case added, should be ready for merge.

About your concern for Symfony @ostrolucky, I think we first thought about adding a line in the default recipe, but you're raising a valid point. I think I'd suggest going with a line in FrameworkBundle::boot() instead then.

@greg0ire greg0ire added the enhancement New feature or request label May 29, 2023
@greg0ire greg0ire added this to the 1.1.0 milestone May 29, 2023
@greg0ire greg0ire merged commit 8cffffb into doctrine:master May 29, 2023
@greg0ire
Copy link
Member

Thanks @nicolas-grekas !

@nicolas-grekas
Copy link
Member Author

See symfony/symfony#50468 for Symfony

nicolas-grekas added a commit to symfony/symfony that referenced this pull request 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
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants