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

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

Merged
merged 1 commit into from Jun 28, 2023

Conversation

GaryPEGEOT
Copy link
Contributor

@GaryPEGEOT GaryPEGEOT commented Jun 27, 2023

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.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jun 27, 2023

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                                       
 ----------------- -------------------------------------------- 

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

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

Thank you @GaryPEGEOT.

@nicolas-grekas nicolas-grekas merged commit 7885223 into symfony:6.4 Jun 28, 2023
6 of 9 checks passed
@GaryPEGEOT GaryPEGEOT deleted the feat/defined-processor branch June 28, 2023 19:40
This was referenced Oct 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants