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

[4.1 BC Break][DI] Env placeholder defaulting to float now rejected for scalar config node #27455

Closed
Majkl578 opened this issue May 31, 2018 · 3 comments

Comments

@Majkl578
Copy link
Contributor

Symfony version(s) affected: 4.1.0

Description
Symfony 4.1.0 introduced validation of env vars in #23888. This breaks env defaults defined as parameters directly in YAML config for seemingly incompatible types, i.e. env defaulting to float value while node is declared as scalarNode (float is scalar too btw).

In ValidateEnvPlaceholdersPass.php line 54:
                                                                                                              
  [Symfony\Component\DependencyInjection\Exception\LogicException]                                            
  Invalid type for env parameter "env(ABC)". Expected "string", but got "float".

How to reproduce

This is based on real configuration of SncRedisBundle which now suffers from this BC break.

Have a bundle with DI configuration having a scalar config node as such:

...
->arrayNode('options')
    ->children()
        ->scalarNode('profile')->defaultValue('default')

Then configured using YAML like this:

parameters:
    env(REDIS_PROFILE): 3.2

snc_redis:
    clients:
        default:
            type: predis
            alias: default
            dsn: '%env(REDIS_DSN)%'
            options:
                profile: '%env(REDIS_PROFILE)%'

Since options.profile is defined as scalarNode(), default value 3.2 (being a float) is rejected and exception above is thrown.

Additional context

Exception trace:
 Symfony\Component\DependencyInjection\Compiler\ValidateEnvPlaceholdersPass->process() at vendor/symfony/dependency-injection/Compiler/Compiler.php:95
 Symfony\Component\DependencyInjection\Compiler\Compiler->compile() at vendor/symfony/dependency-injection/ContainerBuilder.php:736
 Symfony\Component\DependencyInjection\ContainerBuilder->compile() at vendor/symfony/http-kernel/Kernel.php:513
 Symfony\Component\HttpKernel\Kernel->initializeContainer() at vendor/symfony/http-kernel/Kernel.php:125
 Symfony\Component\HttpKernel\Kernel->boot() at vendor/symfony/framework-bundle/Console/Application.php:64
 Symfony\Bundle\FrameworkBundle\Console\Application->doRun() at vendor/symfony/console/Application.php:143
 Symfony\Component\Console\Application->run() at bin/console:37
@ro0NL
Copy link
Contributor

ro0NL commented May 31, 2018

Gave it a quick look, and i think we can include all scalar types here:

As those types are in fact allowed as env parameter defaults (or only number|string i believe)

cc @nicolas-grekas

@ro0NL
Copy link
Contributor

ro0NL commented May 31, 2018

On the other side.. we can also deprecate non-string values here 🤔 as we imply a default string prefix (for envs being string values)

$prefix = (false === $i = strpos($env, ':')) ? 'string' : substr($env, 0, $i);

@nicolas-grekas
Copy link
Member

We should not force ppl to be stricter than PHP's default mode. Let's follow the default coercion rules.

nicolas-grekas added a commit that referenced this issue Jun 3, 2018
This PR was squashed before being merged into the 4.1 branch (closes #27470).

Discussion
----------

[DI] Remove default env type check on validate

| Q             | A
| ------------- | ---
| Branch?       | 4.1
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #27455
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

The default env is already validated to be a scalar or null on `get()`

Commits
-------

a0015a1 [DI] Remove default env type check on validate
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants