[DependencyInjection] Changing enum: example away from APP_ENV
#17507
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Not sure what to do about this. At first I was against this PR ... but now I think I'm in favor of it. What do others think about this? Thanks! |
javiereguiluz
added
the
Waiting team decision
Reason: When I first read this, I was expecting that the Symfony-internal ways to get the environment (`{{ app.environment }}`, `$routingConfigurator->env()` ,etc.) would now return the enum. But this is not the case. So if you explain the new processor with exactly this example, there needs to be a caution box, explaining that Symfony's methods still return the string.
* If you don't agree with this change, I'd still open a separate PR to add this info (taken from https://symfony.com/blog/new-in-symfony-6-2-improved-enum-support#enums-in-environment-variables), cause that's a missing piece of information:
> The value stored in the ``CARD_SUIT`` env var would be a string like `'spades'` but the
> application will use the ``Suit::Spdes`` enum value.
* Besides, as the list of processors is getting longer and longer, I'd suggest to create a sub-heading for each (under "Built-In Environment Variable Processors"), to get rid of the endless indentation, and improve scanability, and get them included in the TOC.
Should I create a PR for this?
javiereguiluz
force-pushed
the
patch-2
branch
from
0000e79 to
c099758
Compare
carsonbot
changed the title
enum: example away from APP_ENVenum: example away from APP_ENV
|
This is now merged. I agree that using some env var related to Symfony environments can be too confusing. Thanks Thomas! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
https://symfony.com/doc/current/configuration/env_var_processors.html#built-in-environment-variable-processors
Reason: When I first read this, I was expecting that the Symfony-internal ways to get the environment (
{{ app.environment }},$routingConfigurator->env(),etc.) would now return the enum. But this is not the case. So if you explain the new processor with exactly this example, there needs to be a caution box, explaining that Symfony's methods still return the string.If you don't agree with this change, I'd still open a separate PR to add the following info (taken from https://symfony.com/blog/new-in-symfony-6-2-improved-enum-support#enums-in-environment-variables), cause that's a missing piece of information here:
Besides, as the list of processors is getting longer and longer, I'd suggest to create a sub-heading for each (under "Built-In Environment Variable Processors") and sort them alphabetically, to get rid of the endless indentation, and improve scanability, and get them included in the TOC.
Should I create a PR for this? If yes, for which version?