[DI] Deprecated using env vars with cannotBeEmpty()

ro0NL · nicolas-grekas · commit 397c19ee5fe0 · 2018-12-01T10:03:58.000+01:00
diff --git a/UPGRADE-4.2.md b/UPGRADE-4.2.md
@@ -16,6 +16,7 @@ Config
 
  * Deprecated constructing a `TreeBuilder` without passing root node information.
  * Deprecated `FileLoaderLoadException`, use `LoaderLoadException` instead.
+ * Deprecated using environment variables with `cannotBeEmpty()` if the value is validated with `validate()`
 
 Console
 -------
diff --git a/UPGRADE-5.0.md b/UPGRADE-5.0.md
@@ -18,6 +18,7 @@ Config
  * Added the `getChildNodeDefinitions()` method to `ParentNodeDefinitionInterface`.
  * The `Processor` class has been made final
  * Removed `FileLoaderLoadException`, use `LoaderLoadException` instead.
+ * Using environment variables with `cannotBeEmpty()` if the value is validated with `validate()` will throw an exception.
 
 Console
 -------
diff --git a/src/Symfony/Component/Config/CHANGELOG.md b/src/Symfony/Component/Config/CHANGELOG.md
@@ -6,6 +6,7 @@ CHANGELOG
 
  * deprecated constructing a `TreeBuilder` without passing root node information
  * renamed `FileLoaderLoadException` to `LoaderLoadException`
+ * deprecated using environment variables with `cannotBeEmpty()` if the value is validated with `validate()`
 
 4.1.0
 -----
diff --git a/src/Symfony/Component/Config/Definition/ScalarNode.php b/src/Symfony/Component/Config/Definition/ScalarNode.php
@@ -48,6 +48,8 @@ protected function validateType($value)
      */
     protected function isValueEmpty($value)
     {
+        // assume environment variables are never empty (which in practice is likely to be true during runtime)
+        // not doing so breaks many configs that are valid today
         if ($this->isHandlingPlaceholder()) {
             return false;
         }
diff --git a/src/Symfony/Component/Config/Definition/VariableNode.php b/src/Symfony/Component/Config/Definition/VariableNode.php
@@ -81,6 +81,19 @@ protected function validateType($value)
      */
     protected function finalizeValue($value)
     {
+        // deny environment variables only when using custom validators
+        // this avoids ever passing an empty value to final validation closures
+        if (!$this->allowEmptyValue && $this->isHandlingPlaceholder() && $this->finalValidationClosures) {
+            @trigger_error(sprintf('Setting path "%s" to an environment variable is deprecated since Symfony 4.2. Remove "cannotBeEmpty()", "validate()" or include a prefix/suffix value instead.', $this->getPath()), E_USER_DEPRECATED);
+//            $e = new InvalidConfigurationException(sprintf('The path "%s" cannot contain an environment variable when empty values are not allowed by definition and are validated.', $this->getPath(), json_encode($value)));
+//            if ($hint = $this->getInfo()) {
+//                $e->addHint($hint);
+//            }
+//            $e->setPath($this->getPath());
+//
+//            throw $e;
+        }
+
         if (!$this->allowEmptyValue && $this->isValueEmpty($value)) {
             $ex = new InvalidConfigurationException(sprintf('The path "%s" cannot contain an empty value, but got %s.', $this->getPath(), json_encode($value)));
             if ($hint = $this->getInfo()) {
@@ -120,6 +133,8 @@ protected function mergeValues($leftSide, $rightSide)
      * @param mixed $value
      *
      * @return bool
+     *
+     * @see finalizeValue()
      */
     protected function isValueEmpty($value)
     {
diff --git a/src/Symfony/Component/DependencyInjection/Tests/Compiler/ValidateEnvPlaceholdersPassTest.php b/src/Symfony/Component/DependencyInjection/Tests/Compiler/ValidateEnvPlaceholdersPassTest.php
@@ -210,6 +210,38 @@ public function testEmptyEnvWhichCannotBeEmptyForScalarNode(): void
         $this->assertSame($expected, $container->resolveEnvPlaceholders($ext->getConfig()));
     }
 
+    /**
+     * NOT LEGACY (test exception in 5.0).
+     *
+     * @group legacy
+     * @expectedDeprecation Setting path "env_extension.scalar_node_not_empty_validated" to an environment variable is deprecated since Symfony 4.2. Remove "cannotBeEmpty()", "validate()" or include a prefix/suffix value instead.
+     */
+    public function testEmptyEnvWhichCannotBeEmptyForScalarNodeWithValidation(): void
+    {
+        $container = new ContainerBuilder();
+        $container->registerExtension($ext = new EnvExtension());
+        $container->prependExtensionConfig('env_extension', $expected = array(
+            'scalar_node_not_empty_validated' => '%env(SOME)%',
+        ));
+
+        $this->doProcess($container);
+
+        $this->assertSame($expected, $container->resolveEnvPlaceholders($ext->getConfig()));
+    }
+
+    public function testPartialEnvWhichCannotBeEmptyForScalarNode(): void
+    {
+        $container = new ContainerBuilder();
+        $container->registerExtension($ext = new EnvExtension());
+        $container->prependExtensionConfig('env_extension', $expected = array(
+            'scalar_node_not_empty_validated' => 'foo %env(SOME)% bar',
+        ));
+
+        $this->doProcess($container);
+
+        $this->assertSame($expected, $container->resolveEnvPlaceholders($ext->getConfig()));
+    }
+
     public function testEnvWithVariableNode(): void
     {
         $container = new ContainerBuilder();
@@ -281,6 +313,14 @@ public function getConfigTreeBuilder()
             ->children()
                 ->scalarNode('scalar_node')->end()
                 ->scalarNode('scalar_node_not_empty')->cannotBeEmpty()->end()
+                ->scalarNode('scalar_node_not_empty_validated')
+                    ->cannotBeEmpty()
+                    ->validate()
+                        ->always(function ($value) {
+                            return $value;
+                        })
+                    ->end()
+                ->end()
                 ->integerNode('int_node')->end()
                 ->floatNode('float_node')->end()
                 ->booleanNode('bool_node')->end()

Original file line number	Diff line number	Diff line change
`@@ -48,6 +48,8 @@ protected function validateType($value)`
`48`	`48`	`*/`
`49`	`49`	`protected function isValueEmpty($value)`
`50`	`50`	`{`
	`51`	`+ // assume environment variables are never empty (which in practice is likely to be true during runtime)`
	`52`	`+ // not doing so breaks many configs that are valid today`
`51`	`53`	`if ($this->isHandlingPlaceholder()) {`
`52`	`54`	`return false;`
`53`	`55`	`}`