check final validation closures

symfony · Oct 16, 2018 · 65e2ccf · 65e2ccf
1 parent e7728c0
commit 65e2ccf
Show file tree

Hide file tree

Showing 6 changed files with 23 additions and 9 deletions.
diff --git a/UPGRADE-4.2.md b/UPGRADE-4.2.md
@@ -16,7 +16,7 @@ Config
 
  * Deprecated constructing a `TreeBuilder` without passing root node information.
  * Deprecated `FileLoaderLoadException`, use `LoaderLoadException` instead.
- * Deprecated using environment variables with `cannotBeEmpty()`
+ * Deprecated using environment variables with `cannotBeEmpty()` (only in combination with `validate()`)
 
 Console
 -------

diff --git a/UPGRADE-5.0.md b/UPGRADE-5.0.md
@@ -18,7 +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()` will throw an exception.
+ * Using environment variables with `cannotBeEmpty()` (only in combination with `validate()`) will throw an exception.
 
 Console
 -------

diff --git a/src/Symfony/Component/Config/CHANGELOG.md b/src/Symfony/Component/Config/CHANGELOG.md
@@ -6,7 +6,7 @@ CHANGELOG
 
  * deprecated constructing a `TreeBuilder` without passing root node information
  * renamed `FileLoaderLoadException` to `LoaderLoadException`
- * deprecated using environment variables with `cannotBeEmpty()`
+ * deprecated using environment variables with `cannotBeEmpty()` (only in combination with `validate()`)
 
 4.1.0
 -----

diff --git a/src/Symfony/Component/Config/Definition/ScalarNode.php b/src/Symfony/Component/Config/Definition/ScalarNode.php
@@ -48,8 +48,9 @@ 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()) {
-            // @deprecated since Symfony 4.2, to be removed in 5.0
             return false;
         }
 

diff --git a/src/Symfony/Component/Config/Definition/VariableNode.php b/src/Symfony/Component/Config/Definition/VariableNode.php
@@ -81,7 +81,9 @@ protected function validateType($value)
      */
     protected function finalizeValue($value)
     {
-        if (!$this->allowEmptyValue && $this->isHandlingPlaceholder()) {
+        // 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()" or include a prefix/suffix 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.', $this->getPath(), json_encode($value)));
 //            if ($hint = $this->getInfo()) {
@@ -131,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
@@ -27,9 +27,11 @@ public function testEnvsAreValidatedInConfig()
     {
         $container = new ContainerBuilder();
         $container->setParameter('env(NULLED)', null);
+        $container->setParameter('env(FLOATISH)', 3.2);
         $container->registerExtension($ext = new EnvExtension());
         $container->prependExtensionConfig('env_extension', $expected = array(
             'scalar_node' => '%env(NULLED)%',
+            'scalar_node_not_empty' => '%env(FLOATISH)%',
             'int_node' => '%env(int:FOO)%',
             'float_node' => '%env(float:BAR)%',
         ));
@@ -199,15 +201,14 @@ public function testEnvIsNotUnset()
      * NOT LEGACY (test exception in 5.0).
      *
      * @group legacy
-     * @expectedDeprecation Setting path "env_extension.scalar_node_not_empty" to an environment variable is deprecated since Symfony 4.2. Remove "cannotBeEmpty()" or include a prefix/suffix instead.
+     * @expectedDeprecation Setting path "env_extension.scalar_node_not_empty_validated" to an environment variable is deprecated since Symfony 4.2. Remove "cannotBeEmpty()" or include a prefix/suffix instead.
      */
-    public function testEmptyEnvWhichCannotBeEmptyForScalarNode(): void
+    public function testEmptyEnvWhichCannotBeEmptyForScalarNodeWithValidation(): void
     {
         $container = new ContainerBuilder();
-        $container->setParameter('env(FLOATISH)', 3.2);
         $container->registerExtension($ext = new EnvExtension());
         $container->prependExtensionConfig('env_extension', $expected = array(
-            'scalar_node_not_empty' => '%env(FLOATISH)%',
+            'scalar_node_not_empty_validated' => '%env(SOME)%',
         ));
 
         $this->doProcess($container);
@@ -299,6 +300,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()