[Config] Fixes the valid placeholder types for variable node #26895
Conversation
|
@ro0NL Can you have a look here? |
|
Hm, @romain-pierre you are using an env-var for a variable node type right? I think we can say it allows all types (including array AFAIK). So maybe the better fix is -if (array_diff($knownTypes, $validTypes))
+if ($validTypes && array_diff($knownTypes, $validTypes))Could you try that? |
|
Absolutely, the use case is the SncRedis configuration (see) that allows env placeholder. The resulting error is:
For your suggestion, why not, it's an other approach, if you consider that non scalar types are also allowed. So, we could consider that in the case of VariableNode use, this is the responsibility of the user to check the type, and not the one of the Config componement? If your are agree with that, I can change my PR by this way. |
That's already possible no? I can pass any value to a variable node type... |
|
Yeah you probably right, but I never see this case for now. The last question I have: there is no confusion/bug risk to disable the check when no type are allowed? It's hard to admit that |
We should add test for variable nodes yes, see https://github.com/symfony/symfony/blob/master/src/Symfony/Component/DependencyInjection/Tests/Compiler/ValidateEnvPlaceholdersPassTest.php#L227 |
The feature is new in 4.1, custom node types are not really affected yet. So yes, safe move IMHO. Empty array means all types are valid (accepting no types is nonsense :)) and is actually a BC fix i guess. |
|
You convinced me (to avoid BC break). But if accepting no types is nonsense it should throw an exception IMHO, in a perfect world. I will update this PR with your proposition. |
|
If you are ready to wait a little bit, I'm in to write some tests about this fix (in an other PR?), it could be a good training for me. |
|
Same PR i'd say. Let me know if you need help :) In general add a new node here https://github.com/symfony/symfony/blob/master/src/Symfony/Component/DependencyInjection/Tests/Compiler/ValidateEnvPlaceholdersPassTest.php#L227 and then a new test like |
|
@romain-pierre in this very PR please, to reduce the overhead. |
|
Thank you @romain-pierre. |
…de (romain-pierre) This PR was merged into the 4.1-dev branch. Discussion ---------- [Config] Fixes the valid placeholder types for variable node | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no <!-- don't forget to update src/**/CHANGELOG.md files --> | BC breaks? | no <!-- see https://symfony.com/bc --> | Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tests pass? | we will see... <!-- please add some, will be required by reviewers --> | Fixed tickets | #26894 | License | MIT This is a proposition, still in discussion. <!-- Write a short README entry for your feature/bugfix here (replace this comment block.) This will help people understand your PR and can be used as a start of the Doc PR. Additionally: - Bug fixes must be submitted against the lowest branch where they apply (lowest branches are regularly merged to upper ones so they get the fixes too). - Features and deprecations must be submitted against the master branch. --> Commits ------- 9fbdcdb Fixes the valid placeholder types for variable node
This is a proposition, still in discussion.