[DependencyInjection][Config] Use several placeholder unique prefixes for dynamic placeholder values #37511
+68
−8
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ro0NL
reviewed
Jul 7, 2020
There was a problem hiding this comment.
i think for config component being able to specify multiple prefixes is a nice feature addition.
i tend to believe the root issue is ideally solved in the DI component, by keeping a single prefix. But i havent looked in depth at it to fully understand the root cause here.
src/Symfony/Component/DependencyInjection/Compiler/MergeExtensionConfigurationPass.php
Show resolved
Hide resolved
b1837b1 to
d8ca8be
Compare
ro0NL
approved these changes
Jul 8, 2020
...Symfony/Component/DependencyInjection/Tests/Compiler/MergeExtensionConfigurationPassTest.php
Show resolved
Hide resolved
d8ca8be to
2948631
Compare
|
There's a failure on deps=low to investigate. |
… for dynamic placeholder values
2948631 to
3d754ad
Compare
|
The test cannot work with previous Config versions. We are not going to to bump the whole dependency for this. So the test is skipped conditionally. Remaining failures are unrelated. |
|
Thank you @fancyweb. |
This was referenced Jul 24, 2020
Merged
Merged
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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