-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[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 our terms of service and privacy 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
Conversation
3b996e9
to
7fd7de2
Compare
764d623
to
07f00e1
Compare
@@ -126,6 +126,12 @@ public function getEnv(string $prefix, string $name, \Closure $getEnv) | |||
} | |||
} | |||
|
|||
$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 the string
prefix except for null where we return null
d42f633
to
024526a
Compare
024526a
to
2c0815d
Compare
Thank you @fancyweb. |
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 SwiftMailer configuration 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)%' <?php
use Symfony\Component\DependencyInjection\EnvVarProcessorInterface;
use Symfony\Component\DependencyInjection\Exception\RuntimeException;
final class NullableStringEnvProcessor implements EnvVarProcessorInterface
{
/**
* {@inheritDoc}
*/
public function getEnv(string $prefix, string $name, \Closure $getEnv)
{
if ($prefix === 'nullablestring') {
$val = $getEnv($name);
if ($val === null) {
return null;
}
return (string) $val;
}
throw new RuntimeException(\sprintf('Unsupported env var prefix "%s" for env name "%s".', $prefix, $name));
}
/**
* {@inheritDoc}
*/
public static function getProvidedTypes()
{
return [
'nullablestring' => 'string',
];
}
} |
Could you open a new issue with a reproducer and a full stack trace plz? |
@@ -390,7 +390,15 @@ protected function getEnv(string $name) | |||
$prefix = 'string'; | |||
$localName = $name; | |||
} | |||
$processor = $processors->has($prefix) ? $processors->get($prefix) : new EnvVarProcessor($this); | |||
|
|||
if ($processors->has($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.
this doesn't work as $prefix
is always set to string
on 390.
confirmed that this fix doesn't work unfortunately, empty stings instead of nulls are still passed to services. |
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 | no The 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
This PR fixes autocasting
null
env 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't fix #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.