Improve the config validation in TwigBundle #14859
Conversation
|
👍 |
| ->scalarNode('auto_reload')->end() | ||
| ->scalarNode('optimizations')->end() | ||
| ->integerNode('optimizations')->end() |
There was a problem hiding this comment.
What do you think of adding a ifNotInArray() validation with the four possible values (-1, 0, 2, 4, 8) for this option? Or even create an enum() node instead of an integer one?
There was a problem hiding this comment.
optimizations are a binary flag. So it cannot be an enum node, unless we have all combinations of the binary flag (and we would have to update it each time Twig adds a new optimization)
There was a problem hiding this comment.
I could add a ->min(-1) though
There was a problem hiding this comment.
OK then, there is nothing else to do. Thanks for the explanation!
|
👍 |
| ->scalarNode('cache')->defaultValue('%kernel.cache_dir%/twig')->end() | ||
| ->scalarNode('charset')->defaultValue('%kernel.charset%')->end() | ||
| ->scalarNode('debug')->defaultValue('%kernel.debug%')->end() | ||
| ->scalarNode('strict_variables')->end() | ||
| ->booleanNode('debug')->defaultValue('%kernel.debug%')->end() |
There was a problem hiding this comment.
I think defaultValue('%kernel.debug%') will be treated as a simple string.
There was a problem hiding this comment.
Yes? It's resolved later. What is your objection?
There was a problem hiding this comment.
Using such string resolved later works because the default value is applied after the validation. FrameworkBundle already does it.
It is true however that the DI extension will still receive the string here, not the resolved value. But we don't care because the DI extension does not care about the value itself. It just sets it in the container (where it will be resolved).
For cases where the value influences the DI extension logic, it is indeed necessary to resolve the value, which can either be done explicitly in the DI extension or by passing the debug flag to the Configuration class (which is what DoctrineBundle does for instance)
|
👍 |
|
@fabpot please fix fabbot.io so that statuses are not reported as errored on Github. Having PRs marked as red because of a fabbot.io 404 is not great. |
|
Thank you @stof. |
This PR was merged into the 2.3 branch. Discussion ---------- Improve the config validation in TwigBundle | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #14858 | License | MIT | Doc PR | n/a This ensures we have better error messages in case of misconfiguration instead of having crappy errors from Twig (which does not validates its options fully either, missing these things) Commits ------- f4dfee3 Improve the config validation in TwigBundle
This ensures we have better error messages in case of misconfiguration instead of having crappy errors from Twig (which does not validates its options fully either, missing these things)