Skip to content

[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

Conversation

fancyweb
Copy link
Contributor

@fancyweb fancyweb commented Jun 30, 2023

Q A
Branch? 5.4
Bug fix? yes
New feature? -
Deprecations? -
Tickets #50815 (comment)
License MIT
Doc PR -

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.

@carsonbot carsonbot added this to the 5.4 milestone Jun 30, 2023
@fancyweb fancyweb force-pushed the di/fix-autocast-null-env-to-empty-string branch from 3b996e9 to 7fd7de2 Compare June 30, 2023 16:28
@fancyweb fancyweb force-pushed the di/fix-autocast-null-env-to-empty-string branch 2 times, most recently from 764d623 to 07f00e1 Compare June 30, 2023 16:42
@@ -126,6 +126,12 @@ public function getEnv(string $prefix, string $name, \Closure $getEnv)
}
}

$returnNull = false;
if ('' === $prefix) {
Copy link
Contributor Author

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

@fancyweb fancyweb force-pushed the di/fix-autocast-null-env-to-empty-string branch 2 times, most recently from d42f633 to 024526a Compare June 30, 2023 17:37
@nicolas-grekas nicolas-grekas force-pushed the di/fix-autocast-null-env-to-empty-string branch from 024526a to 2c0815d Compare July 3, 2023 12:55
@nicolas-grekas
Copy link
Member

Thank you @fancyweb.

@nicolas-grekas nicolas-grekas merged commit 1e1d91e into symfony:5.4 Jul 3, 2023
@fancyweb fancyweb deleted the di/fix-autocast-null-env-to-empty-string branch July 3, 2023 13:00
@melkamar
Copy link

melkamar commented Jul 12, 2023

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 MAILER_ENCRYPTION unset, the parameter now becomes an empty string ``. However, the Swiftmailer library does not like that as it only handles null in a special way, not an empty string, and throws

  [InvalidArgumentException]
  The  encryption is not supported

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 null as null), the container build crashes on type mismatch:

  [Symfony\Component\Config\Definition\Exception\InvalidTypeException]
  Invalid type for path "swiftmailer.mailers.default.encryption". Expected one of "bool", "int", "float", "string", but got one of "bool", "int", "float", "string", "array".

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

There's still no solution to have the "keep null as null" and "cast if not null" behavior.

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',
        ];
    }
}

@fancyweb
Copy link
Contributor Author

Could you open a new issue with a reproducer and a full stack trace plz?

This was referenced Jul 29, 2023
@@ -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)) {
Copy link
Contributor

@bendavies bendavies Jul 31, 2023

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.

@bendavies
Copy link
Contributor

confirmed that this fix doesn't work unfortunately, empty stings instead of nulls are still passed to services.
problem noted above.

@fancyweb
Copy link
Contributor Author

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 😓

nicolas-grekas added a commit that referenced this pull request Sep 20, 2023
… 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants