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

Cast env vars to null or bool when referencing them using #[Autowire(env: '...')] depending on the signature of the corresponding parameter #53971

Merged
merged 1 commit into from
Apr 13, 2024

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
@smnandre
Copy link
Contributor

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
Contributor

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
…them using `#[Autowire(env: '...')]` depending on the signature of the corresponding parameter
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.

@fabpot fabpot merged commit 50d7ce0 into symfony:7.1 Apr 13, 2024
6 of 10 checks passed
@ruudk ruudk deleted the add-bool-env-processor branch April 13, 2024 08:25
@fabpot fabpot mentioned this pull request May 2, 2024
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
5 participants