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

Merged
nicolas-grekas merged 1 commit intosymfony:6.4from
GaryPEGEOT:feat/defined-processor
Jun 28, 2023
Merged

[DependencyInjection] Add defined prefix for env var processor#50791
nicolas-grekas merged 1 commit intosymfony:6.4from
GaryPEGEOT:feat/defined-processor

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
GaryPEGEOT commented Jun 27, 2023

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
nicolas-grekas commented Jun 27, 2023

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.

8000

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
GaryPEGEOT commented Jun 28, 2023

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
nicolas-grekas commented Jun 28, 2023

nulland false also yes

@GaryPEGEOT
Copy link
Contributor Author
GaryPEGEOT commented Jun 28, 2023

Fixed the checks and added a few test cases

nicolas-grekas
nicolas-grekas reviewed Jun 28, 2023
View reviewed changes
nicolas-grekas
nicolas-grekas reviewed Jun 28, 2023
View reviewed changes
@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
nicolas-grekas commented Jun 28, 2023

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
@fabpot fabpot mentioned this pull request Oct 21, 2023
Merged
@fabpot fabpot mentioned this pull request Oct 21, 2023
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