8000 [DependencyInjection] Add `defined` prefix for env var processor by GaryPEGEOT · Pull Request #50791 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[DependencyInjection] Add defined prefix for env var processor #50791

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.

Sign up for GitHub

By clicking &ldqu 8000 o;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

Jump to bottom
Merged
merged 1 commit into from
Jun 28, 2023
Merged

[DependencyInjection] Add defined prefix for env var processor #50791

merged 1 commit into from
Jun 28, 2023

Conversation

GaryPEGEOT
Copy link
Contributor
@GaryPEGEOT GaryPEGEOT commented Jun 27, 2023 •
edited by nicolas-grekas
Loading
Q A
Branch? 6.4
Bug fix? no
New feature? yes
Deprecations? no
Tickets N/A
License MIT
Doc PR TODO

Allow to add a defined prefix:

parameters:
  is_google_recaptcha_enabled: '%env(defined:GOOGLE_RECAPTCHA_SITE_KEY)%'

Returns false if the env var doesn't exist or if it's null or the empty string.
Returns true otherwise.

@carsonbot carsonbot added this to the 6.4 milestone Jun 27, 2023
@nicolas-grekas
Copy link
Member
nicolas-grekas commented Jun 27, 2023 •
edited
Loading

bool:default::GOOGLE_RECAPTCHA_SITE_KEY?

@GaryPEGEOT
Copy link
Contributor Author

bool:default::GOOGLE_RECAPTCHA_SITE_KEY?

I tried with bool first, but any other value than '1' or 'true' get interpreted as false:

 // Displaying detailed environment variable usage matching GOOGLE_RECAPTCHA_SITE_KEY                                   

%env(bool:GOOGLE_RECAPTCHA_SITE_KEY)%
-------------------------------------

 ----------------- -------------------------------------------- 
  Default value     n/a                                         
  Real value        "some_value"  
  Processed value   false                                       
 ----------------- --------------------------------------------

GromNaN
GromNaN requested changes Jun 27, 2023
View reviewed changes
Copy link
Member
@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

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

Sounds like a useful feature.

@nicolas-grekas
Copy link
Member

I tried with bool first, but any other value than '1' or 'true' get interpreted as false:

That's unexpected. We use FILTER_VALIDATE_BOOL, which handles way more cases. The test suite is also proving that.
Not convinced personally sorry. Please provide more insights.

@ro0NL
Copy link
Contributor
ro0NL commented Jun 27, 2023

Returns true for "1", "true", "on" and "yes". Returns false otherwise.

;)

personally im not to fond of turning SOME=false into bool(true) and SOME=0 into bool(false), but yes, this processor would allow to derive a bool value as such. IMHO an empty string check should be used instead of empty().

we do this in runtime for some env: $enabled = '' !== $value

however, in hindsight i'd split it into 2 envs, rather than 1 conditional env, thus:

$enabled = bool:SOME_ENABLED
$value = string:SOME_VALUE

nicolas-grekas
nicolas-grekas reviewed Jun 28, 2023
View reviewed changes
Copy link
Member
@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

On second thought, we already do have the NO_COLOR env vars which works like that, and this is indeed not the same as bool.
On the behavior side, this should not throw when the env var is not defined. It does current if I'm not wrong.
The check should be moved below the code handling "default" and should account for not found vars. I would also not use empty but use a comparison to the empty string only.

@GaryPEGEOT
Copy link
Contributor Author

On second thought, we already do have the NO_COLOR env vars which works like that, and this is indeed not the same as bool. On the behavior side, this should not throw when the env var is not defined. It does current if I'm not wrong. The check should be moved below the code handling "default" and should account for not found vars. I would also not use empty but use a comparison to the empty string only.

@nicolas-grekas maybe check for null as well? I wouldn't consider null as defined

@nicolas-grekas
Copy link
Member

nulland false also yes

@GaryPEGEOT
Copy link
Contributor Author

Fixed the checks and added a few test cases

@stof
Copy link
Member
stof commented Jun 28, 2023

@nicolas-grekas but as we don't use FILTER_NULL_ON_FAILURE, anything that does not parse as either true or false in FILTER_VALIDATE_BOOLEAN gets returned as false. This is what happens for this key having a value some_value

@nicolas-grekas nicolas-grekas force-pushed the feat/defined-processor branch from 5f1f041 to 473fc19 Compare June 28, 2023 19:15
@nicolas-grekas
Copy link
Member

Thank you @GaryPEGEOT.

@nicolas-grekas nicolas-grekas merged commit 7885223 into symfony:6.4 Jun 28, 2023
@nicolas-grekas nicolas-grekas mentioned this pull request Jun 28, 2023
Closed
@GaryPEGEOT GaryPEGEOT deleted the feat/defined-processor branch June 28, 2023 19:40
This was referenced Oct 21, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GromNaN GromNaN GromNaN approved these changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

Assignees
No one assigned
Labels
DependencyInjection Feature Status: Reviewed
Projects
None yet
Milestone
6.4
Development

Successfully merging this pull request may close these issues.
6 participants
@GaryPEGEOT @nicolas-grekas @ro0NL @stof @GromNaN @carsonbot
0