bug #37511 [DependencyInjection][Config] Use several placeholder unique prefixes for dynamic placeholder values (fancyweb)

This PR was merged into the 4.4 branch.

Discussion
----------

[DependencyInjection][Config] Use several placeholder unique prefixes for dynamic placeholder values

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | https://github.com/symfony/symfony/issues/37426
| License       | MIT
| Doc PR        |-

Currently, in Config BaseNode, we are only able to check on one dynamic placeholder unique prefix (ie one env placeholder unique prefix in our usage) to ignore the validation of config values that contain placeholder values. It isn't enough in some scenarios, we would need to be able to check on multiple prefixes.

In MergeExtensionConfigurationPass we need to not validate config values that are placeholders (they are checked later by a dedicated pass), so we set the BaseNode placeholder unique prefix for each processed extension. Because the env placeholder unique prefix depends of the bag data, if a first extension creates the env placeholder and adds a parameter to the bag, the second extension reusing the same env placeholder will generate a different env placeholder unique prefix. Therefore the validation of the value will not be skipped because the env placeholder contains the first env placeholder unique prefix while the BaseNode placeholder unique prefix is set with the second.

Another solution might be to mutate the existing env placeholders to use the new unique prefix when merging the env placeholders ? I guess it depends on which side we want to fix this (DI or Config).

cc @ro0NL

Commits
-------

3d754ad688 [DependencyInjection][Config] Use several placeholder unique prefixes for dynamic placeholder values

src/Symfony/Component/Config/Definition/BaseNode.php

@@ -26,7 +26,7 @@ abstract class BaseNode implements NodeInterface
 {
     const DEFAULT_PATH_SEPARATOR = '.';
 
-    private static $placeholderUniquePrefix;
+    private static $placeholderUniquePrefixes = [];
     private static $placeholders = [];
 
     protected $name;
@@ -74,7 +74,7 @@ abstract class BaseNode implements NodeInterface
     }
 
     /**
-     * Sets a common prefix for dynamic placeholder values.
+     * Adds a common prefix for dynamic placeholder values.
      *
      * Matching configuration values will be skipped from being processed and are returned as is, thus preserving the
      * placeholder. An exact match provided by {@see setPlaceholder()} might take precedence.
@@ -83,7 +83,7 @@ abstract class BaseNode implements NodeInterface
      */
     public static function setPlaceholderUniquePrefix(string $prefix): void
     {
-        self::$placeholderUniquePrefix = $prefix;
+        self::$placeholderUniquePrefixes[] = $prefix;
     }
 
     /**
@@ -93,7 +93,7 @@ abstract class BaseNode implements NodeInterface
      */
     public static function resetPlaceholders(): void
     {
-        self::$placeholderUniquePrefix = null;
+        self::$placeholderUniquePrefixes = [];
         self::$placeholders = [];
     }
 
@@ -513,8 +513,10 @@ abstract class BaseNode implements NodeInterface
                 return self::$placeholders[$value];
             }
 
-            if (self::$placeholderUniquePrefix && 0 === strpos($value, self::$placeholderUniquePrefix)) {
-                return [];
+            foreach (self::$placeholderUniquePrefixes as $placeholderUniquePrefix) {
+                if (0 === strpos($value, $placeholderUniquePrefix)) {
+                    return [];
+                }
             }
         }
 

src/Symfony/Component/DependencyInjection/Compiler/MergeExtensionConfigurationPass.php

@@ -79,11 +79,11 @@ class MergeExtensionConfigurationPass implements CompilerPassInterface
                     $container->getParameterBag()->mergeEnvPlaceholders($resolvingBag);
                 }
 
-                throw $e;
-            } finally {
                 if ($configAvailable) {
                     BaseNode::resetPlaceholders();
                 }
+
+                throw $e;
             }
 
             if ($resolvingBag instanceof MergeExtensionConfigurationParameterBag) {
@@ -95,6 +95,10 @@ class MergeExtensionConfigurationPass implements CompilerPassInterface
             $container->getParameterBag()->add($parameters);
         }
 
+        if ($configAvailable) {
+            BaseNode::resetPlaceholders();
+        }
+
         $container->addDefinitions($definitions);
         $container->addAliases($aliases);
     }

src/Symfony/Component/DependencyInjection/Tests/Compiler/MergeExtensionConfigurationPassTest.php

@@ -12,6 +12,7 @@
 namespace Symfony\Component\DependencyInjection\Tests\Compiler;
 
 use PHPUnit\Framework\TestCase;
+use Symfony\Component\Config\Definition\BaseNode;
 use Symfony\Component\Config\Definition\Builder\TreeBuilder;
 use Symfony\Component\Config\Definition\ConfigurationInterface;
 use Symfony\Component\Config\Resource\FileResource;
@@ -128,6 +129,23 @@ class MergeExtensionConfigurationPassTest extends TestCase
 
         $this->assertSame(['FOO'], array_keys($container->getParameterBag()->getEnvPlaceholders()));
     }
+
+    public function testReuseEnvPlaceholderGeneratedByPreviousExtension()
+    {
+        if (!property_exists(BaseNode::class, 'placeholderUniquePrefixes')) {
+            $this->markTestSkipped('This test requires symfony/config ^4.4.11|^5.0.11|^5.1.3');
+        }
+
+        $container = new ContainerBuilder();
+        $container->registerExtension(new FooExtension());
+        $container->registerExtension(new TestCccExtension());
+        $container->prependExtensionConfig('foo', ['bool_node' => '%env(bool:MY_ENV_VAR)%']);
+        $container->prependExtensionConfig('test_ccc', ['bool_node' => '%env(bool:MY_ENV_VAR)%']);
+
+        (new MergeExtensionConfigurationPass())->process($container);
+
+        $this->addToAssertionCount(1);
+    }
 }
 
 class FooConfiguration implements ConfigurationInterface
@@ -139,6 +157,7 @@ class FooConfiguration implements ConfigurationInterface
             ->children()
                 ->scalarNode('bar')->end()
                 ->scalarNode('baz')->end()
+                ->booleanNode('bool_node')->end()
             ->end();
 
         return $treeBuilder;
@@ -166,6 +185,8 @@ class FooExtension extends Extension
             $container->getParameterBag()->get('env(BOZ)');
             $container->resolveEnvPlaceholders($config['baz']);
         }
+
+        $container->setParameter('foo.param', 'ccc');
     }
 }
 
@@ -194,3 +215,36 @@ class ThrowingExtension extends Extension
         throw new \Exception();
     }
 }
+
+final class TestCccConfiguration implements ConfigurationInterface
+{
+    public function getConfigTreeBuilder(): TreeBuilder
+    {
+        $treeBuilder = new TreeBuilder('test_ccc');
+        $treeBuilder->getRootNode()
+            ->children()
+                ->booleanNode('bool_node')->end()
+            ->end();
+
+        return $treeBuilder;
+    }
+}
+
+final class TestCccExtension extends Extension
+{
+    public function getAlias(): string
+    {
+        return 'test_ccc';
+    }
+
+    public function getConfiguration(array $config, ContainerBuilder $container): ?ConfigurationInterface
+    {
+        return new TestCccConfiguration();
+    }
+
+    public function load(array $configs, ContainerBuilder $container)
+    {
+        $configuration = $this->getConfiguration($configs, $container);
+        $this->processConfiguration($configuration, $configs);
+    }
+}