Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[DependencyInjection] Fix autocasting null env values to empty string#50837
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
[DependencyInjection] Fix autocasting null env values to empty string#50837
Uh oh!
There was an error while loading.Please reload this page.
Conversation
3b996e9 to7fd7de2CompareUh oh!
There was an error while loading.Please reload this page.
764d623 to07f00e1Compare| } | ||
| $returnNull =false; | ||
| if ('' ===$prefix) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
'' becomes a magic value that acts as thestring prefix except for null where we return null
d42f633 to024526aCompare024526a to2c0815dComparenicolas-grekas commentedJul 3, 2023
Thank you@fancyweb. |
melkamar commentedJul 12, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I may have shot myself in the foot with#50415 😁 I have now upgraded to a version including this fix and am having issues with Swiftmailer configuration: swiftmailer:encryption:'%env(string:default::MAILER_ENCRYPTION)%' With this and When I change the definition of the parameter to swiftmailer:encryption:'%env(default::MAILER_ENCRYPTION)%' which, as I understand this PR, should do what I expect it to (pass This is because the SwiftMailerconfiguration expects a scalar node but the result of the env-var-processing may be an array (though not in my particular case, I think). This seems to be the case that you wrote about in the PR description
It feels like there is a disconnect between what's possible with the parameter/yaml based configuration and what's possible to do with environment variables. One solution to the problem above would be to have a "nullable" set of prefixes, but I don't know if that's making things even more complicated. Edit: this seems to do the trick in our code, at least: swiftmailer:encryption:'%env(nullablestring:default::MAILER_ENCRYPTION)%' <?phpuseSymfony\Component\DependencyInjection\EnvVarProcessorInterface;useSymfony\Component\DependencyInjection\Exception\RuntimeException;finalclass NullableStringEnvProcessorimplements EnvVarProcessorInterface{/** * {@inheritDoc} */publicfunctiongetEnv(string$prefix,string$name,\Closure$getEnv) {if ($prefix ==='nullablestring') {$val =$getEnv($name);if ($val ===null) {returnnull; }return (string)$val; }thrownewRuntimeException(\sprintf('Unsupported env var prefix "%s" for env name "%s".',$prefix,$name)); }/** * {@inheritDoc} */publicstaticfunctiongetProvidedTypes() {return ['nullablestring' =>'string', ]; }} |
fancyweb commentedJul 12, 2023
Could you open a new issue with a reproducer and a full stack trace plz? |
| } | ||
| $processor =$processors->has($prefix) ?$processors->get($prefix) :newEnvVarProcessor($this); | ||
| if ($processors->has($prefix)) { |
bendaviesJul 31, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this doesn't work as$prefix is always set tostring on 390.
bendavies commentedJul 31, 2023
confirmed that this fix doesn't work unfortunately, empty stings instead of nulls are still passed to services. |
fancyweb commentedJul 31, 2023
I tested it and indeed it doesn't work in the full framework context. I'll fix it again, for the last time I hope 😓 |
… empty string with `container.env_var_processors_locator` (fancyweb)This PR was merged into the 5.4 branch.Discussion----------[DependencyInjection] Fix autocasting `null` env values to empty string with `container.env_var_processors_locator`| Q | A| ------------- | ---| Branch? | 5.4| Bug fix? | yes| New feature? | no| Deprecations? | no| Tickets |#50837 (comment)| License | MIT| Doc PR | noThe previous fix doesn't work when `$processors` comes from `container.env_var_processors_locator`.Commits-------766bc6e [DependencyInjection] Fix autocasting null env values to empty string with container.env_var_processors_locator
Uh oh!
There was an error while loading.Please reload this page.
This PR fixes autocasting
nullenv values to''.We continue to autocast all other values to string when there's no prefix.
It doesn't revert#50517, it just fixes an unintended side effect.
It doesn'tfix#50815 request, it confirms the behavior: the solution for this issue would be to remove the
string:prefix.There's still no solution to have the "keep null as null" and "cast if not null" behavior.