Skip to content

Conversation

ruudk
Copy link
Contributor

@ruudk ruudk commented Feb 16, 2024

Q A
Branch? 7.1
Bug fix? no
New feature? yes
Deprecations? no
Issues Fix #53918
License MIT

See #53918

This PR automatically adds bool Environment Variable Processors on #[Autowire(env: 'KEY')] bool $key arguments.

The idea behind this, is to prevent mistakes being made. If you omit the bool env var processor, passing KEY=false will become true-ish and thus mark $key as true.

With the bool env processor, KEY=false becomes false.

It also automatically adds the default:: prefix if the default value of an argument is null.

@ruudk ruudk requested a review from dunglas as a code owner February 16, 2024 14:16
@carsonbot carsonbot added this to the 7.1 milestone Feb 16, 2024
@ruudk ruudk changed the title Add bool env processor when not defined [DependencyInjection] Add bool env processor when not defined Feb 16, 2024
@ruudk ruudk force-pushed the add-bool-env-processor branch from 21386a2 to 6b838ba Compare February 16, 2024 14:17
@smnandre
Copy link
Member

I know this is not a "good" practice, but i have use multiple times the following method to "feature-flag" some services.

SERVICE_SUPER_KEY=my_super_key
[Autowire(env: 'SERVICE_SUPER_KEY')] bool $superServiceEnabled

With your PR that would create the opposite problem of your example :|

🤷

@nicolas-grekas
Copy link
Member

@smnandre your use case could be covered if the code for the bool processor were doing this in EnvVarProcessor:

filter_var($env, \FILTER_VALIDATE_BOOL, \FILTER_NULL_ON_FAILURE) ?? filter_var($env, \FILTER_VALIDATE_INT, \FILTER_NULL_ON_FAILURE) ?? filter_var($env, \FILTER_VALIDATE_FLOAT, \FILTER_NULL_ON_FAILURE) ?? true

Worth considering?

@nicolas-grekas
Copy link
Member

Similarly, we could also add default:: to nullable parameters, WDYT?

@smnandre
Copy link
Member

Worth considering?

As i said, this usage was a bit hacky, so we can "ignore it", but that will deserve some warning / upgrade mentions, no ?

@fabpot
Copy link
Member

fabpot commented Feb 22, 2024

It makes sense to me but the changelog should explain a bit more the change and the new behavior.

@nicolas-grekas nicolas-grekas changed the title [DependencyInjection] Add bool env processor when not defined [DependencyInjection] Cast env vars to null or bool when referencing them using #[Autowire(env: '...')] depending on the signature of the corresponding parameter Apr 11, 2024
@nicolas-grekas nicolas-grekas force-pushed the add-bool-env-processor branch from 6b838ba to 83c82e0 Compare April 11, 2024 08:45
…them using `#[Autowire(env: '...')]` depending on the signature of the corresponding parameter
@nicolas-grekas nicolas-grekas force-pushed the add-bool-env-processor branch from 83c82e0 to 5890327 Compare April 11, 2024 08:48
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.

I finished the PR, improving the logic a bit and adding support for default:: when a parameter defaults to null.

@nicolas-grekas nicolas-grekas added ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" Ready labels Apr 11, 2024
@carsonbot carsonbot changed the title [DependencyInjection] Cast env vars to null or bool when referencing them using #[Autowire(env: '...')] depending on the signature of the corresponding parameter Cast env vars to null or bool when referencing them using #[Autowire(env: '...')] depending on the signature of the corresponding parameter Apr 13, 2024
@fabpot
Copy link
Member

fabpot commented Apr 13, 2024

Thank you @ruudk.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Ready ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" Status: Reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DependencyInjection] Warn when not using bool environment processor on bool argument together with #[Autowire(env: 'ENV')]
5 participants