[Config] Fix ArrayNode extra keys "ignore" and "remove" behaviors #15593
Conversation
|
So, what about this bug fix ? |
|
I can not evaluate this fix for now, but can confirm the issue in 2.8. To give a practical example... since #14238, it is possible to write meaningless configuration in a Symfony Standard Edition like: or more serious without throwing an exception. Where the second example is missing the |
|
I think Status: Needs work |
|
@xelaris : You're right. I removed these methods. |
|
Looks good to me. Status: Reviewed |
| @@ -29,7 +29,7 @@ class ArrayNode extends BaseNode implements PrototypeNodeInterface | |||
| protected $addIfNotSet = false; | |||
| protected $performDeepMerging = true; | |||
| protected $ignoreExtraKeys = false; | |||
| protected $removeExtraKeys = true; | |||
| protected $removeExtraKeys = false; | |||
There was a problem hiding this comment.
This is not BC: if someone was ignoring values, you would not remove values anymore by default
There was a problem hiding this comment.
When I thought about that earlier, I was of the opinion that if someone was ignoring values before, setIgnoreExtraKeys must be called to do that and the $remove parameter was added with the default value of true, which would keeps BC then. When rethinking it now, someone could inherit from ArrayNode and change $ignoreExtraKeys via other methods as it is protected, although this should be an edge case.
But I think setting the initial value of this property to true requires no other changes, as it also throws an exception on extra keys when not ignoring them.
There was a problem hiding this comment.
Well, I don't remember why I changed those values at the time.
Updated.
|
Thanks @ogizanagi. I think this is ready to be merge now. |
|
👍 ping @symfony/deciders |
|
👍 |
|
Thank you @ogizanagi. |
…haviors (ogizanagi) This PR was merged into the 2.8 branch. Discussion ---------- [Config] Fix ArrayNode extra keys "ignore" and "remove" behaviors | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Due to #14238 , no more exception is thrown when submitting extra keys to an `ArrayNode`. For instance: ```php $builder = new TreeBuilder(); $nodeDefinition = $builder->root('root') ->children() ->scalarNode('foo') ->end() ->end(); $node = $nodeDefinition->getNode(true); $node->normalize(array( 'foo' => 'ok', 'bar' => 'ko', )); ``` will not throw a > Symfony\Component\Config\Definition\Exception\InvalidConfigurationException: Unrecognized option "bar" under "root"` anymore, as it does in 2.7. I think the expected behavior is: `Submitted data: ['bar' => 'ko']` Ignore | Remove | Expected | OK | Comment ---------| ------------ | ------------- | ------ | ---------- true | true | `[ ]` | ✔︎ | Previous behavior when ignoring. true | false | `['bar' => 'ko']` | ✔︎ | This is the result targeted by #14238. false | true | exception | ✘ | Removing makes no sense when not ignoring extra keys. <br/>The exception should still be thrown. false | false | exception | ✘ | Previous behavior (2.7). <br/>Should not have changed Commits ------- d961f7f [Config] Fix ArrayNode extra keys "ignore" and "remove" behaviors
Due to #14238 , no more exception is thrown when submitting extra keys to an
ArrayNode.For instance:
will not throw a
anymore, as it does in 2.7.
I think the expected behavior is:
Submitted data: ['bar' => 'ko'][ ]['bar' => 'ko']The exception should still be thrown.
Should not have changed