[config] Fix issue when key removed and left value only #14082

zerustech · 2015-03-27T15:49:08Z

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#15270
License	MIT
Doc PR	n/a

When a key attribute is mapped and the key is removed from the value array, if
only 'value' element is left in the array, it should replace its wrapper
array.

Assume the original value array is as follows (key attribute is 'id').

array(
    'things' => array(
        array('id' => 'option1', 'value' => 'value1'),
        array('id' => 'option2', 'value' => 'value2')
    )
)

After normalized, the above shall be converted to the following array.

array(
    'things' => array(
        'option1' => 'value1',
        'option2' => 'value2'
    )
)

It's also possible to mix 'value-only' and 'none-value-only' elements in
the array:

array(
    'things' => array(
        array('id' => 'option1', 'value' => 'value1'),
        array('id' => 'option2', 'value' => 'value2', 'foo' => 'foo2')
    )
)

The above shall be converted to the following array.

array(
    'things' => array(
        'option1' => 'value1',
        'option2' => array('value' => 'value2','foo' => 'foo2')
    )
)

The 'value' element can also be array:

array(
    'things' => array(
        array(
            'id' => 'option1',
            'value' => array('foo'=>'foo1', 'bar' => 'bar1')
        )
    )
)

The above shall be converted to the following array.

array(
    'things' => array(
        'option1' => array('foo' => 'foo1', 'bar' => 'bar1')
    )
)

When using VariableNode for value element, it's also possible to mix
different types of value elements:

array(
    'things' => array(
        array('id' => 'option1', 'value' => array('foo'=>'foo1', 'bar' => 'bar1')),
        array('id' => 'option2', 'value' => 'value2')
    )
)

The above shall be converted to the following array.

array(
    'things' => array(
        'option1' => array('foo'=>'foo1', 'bar' => 'bar1'),
        'option2' => 'value2'
    )
)

fabpot · 2016-03-02T13:43:51Z

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

+     *
+     * @return mixed The prototype instance
+     */
+    protected function getPrototypeForChild($key)


This method should be private.

fabpot · 2016-03-02T13:47:51Z

The examples you give in the description seems good to me.

fabpot · 2016-03-02T13:48:40Z

Apart from the minor formatting comments, 👍

@zerustech Can you rebase on current 2.3 so that tests can be re-run? Thanks.

zerustech · 2016-03-03T08:52:01Z

@fabpot Done, please check. Thanks.

fabpot · 2016-03-03T12:37:06Z

@zerustech Thanks, it looks like it breaks some tests. Can you check?

zerustech · 2016-03-04T10:36:39Z

@fabpot Looks like it was caused by TwigBundle :

Testing src/Symfony/Bundle/TwigBundle
.....EEF.......EE.......

Time: 17.06 seconds, Memory: 13.25Mb

xabbuh · 2016-03-07T11:16:54Z

@zerustech Yeah, but this happens when the bunde's config is evaluated and the failures are related to the prototype handling from this PR.

Status: Needs work

zerustech · 2016-03-10T12:40:52Z

@xabbuh @fabpot Fixed the following issues:

When the 'value' element in an array is null, it does not replace its wrapper array, because isset() returns false on null element.
TwigBundle manipulates its configuration values with the normalization closures of its prototype nodes. For example, under the globals node, "foo" is changed to array("value" => "foo") and etc, so TwigBundle always expects the 'value' key to present. When a PrototypedArrayNode node replaces the wrapper array with its 'value' element, the 'value' key disappears and the actual prototype node will be replaced as well: array("value" => "foo") becomes "foo" and PrototypedArrayNode becomes VariableNode, so when normalizing the element, the original normalization closures are gone, "foo" won't be changed back to array("value" => "foo"), which will result in errors. To fix this issue, we can copy the original normalization closures into the new prototype node. I know it's not the perfect solution, but at this moment, it's the least-worst solution.

fabpot · 2016-09-14T21:00:15Z

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

+    /**
+     * @var NodeInterface[] An array of the prototypes of the simplified value children
+     */
+    protected $valuePrototypes = array();


should be private IMO

@fabpot Fixed, please test.

fabpot · 2016-09-14T21:01:20Z

👍 for merge in 2.7 after the small asked change.

When a key attribute is mapped and the key is removed from the value array, if only 'value' element is left in the array, it should replace its wrapper array. Assume the original value array is as follows (key attribute is 'id'). ```php array( 'things' => array( array('id' => 'option1', 'value' => 'value1'), array('id' => 'option2', 'value' => 'value2') ) ) ``` After normalized, the above shall be converted to the following array. ```php array( 'things' => array( 'option1' => 'value1', 'option2' => 'value2' ) ) ``` It's also possible to mix 'value-only' and 'none-value-only' elements in the array: ```php array( 'things' => array( array('id' => 'option1', 'value' => 'value1'), array('id' => 'option2', 'value' => 'value2', 'foo' => 'foo2') ) ) ``` The above shall be converted to the following array. ```php array( 'things' => array( 'option1' => 'value1', 'option2' => array('value' => 'value2','foo' => 'foo2') ) ) ``` The 'value' element can also be array: ```php array( 'things' => array( array( 'id' => 'option1', 'value' => array('foo'=>'foo1', 'bar' => 'bar1') ) ) ) ``` The above shall be converted to the following array. ```php array( 'things' => array( 'option1' => array('foo' => 'foo1', 'bar' => 'bar1') ) ) ``` When using VariableNode for value element, it's also possible to mix different types of value elements: ```php array( 'things' => array( array('id' => 'option1', 'value' => array('foo'=>'foo1', 'bar' => 'bar1')), array('id' => 'option2', 'value' => 'value2') ) ) ``` The above shall be converted to the following array. ```php array( 'things' => array( 'option1' => array('foo'=>'foo1', 'bar' => 'bar1'), 'option2' => 'value2' ) ) ``` | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | symfony#15270 | License | MIT | Doc PR | n/a

fabpot · 2016-12-14T08:03:10Z

Thank you @zerustech.

…erustech) This PR was submitted for the 2.3 branch but it was merged into the 2.7 branch instead (closes #14082). Discussion ---------- [config] Fix issue when key removed and left value only | Q | A | | --- | --- | | Bug fix? | yes | | New feature? | no | | BC breaks? | no | | Deprecations? | no | | Tests pass? | yes | | Fixed tickets | #15270 | | License | MIT | | Doc PR | n/a | When a key attribute is mapped and the key is removed from the value array, if only 'value' element is left in the array, it should replace its wrapper array. Assume the original value array is as follows (key attribute is 'id'). ``` php array( 'things' => array( array('id' => 'option1', 'value' => 'value1'), array('id' => 'option2', 'value' => 'value2') ) ) ``` After normalized, the above shall be converted to the following array. ``` php array( 'things' => array( 'option1' => 'value1', 'option2' => 'value2' ) ) ``` It's also possible to mix 'value-only' and 'none-value-only' elements in the array: ``` php array( 'things' => array( array('id' => 'option1', 'value' => 'value1'), array('id' => 'option2', 'value' => 'value2', 'foo' => 'foo2') ) ) ``` The above shall be converted to the following array. ``` php array( 'things' => array( 'option1' => 'value1', 'option2' => array('value' => 'value2','foo' => 'foo2') ) ) ``` The 'value' element can also be array: ``` php array( 'things' => array( array( 'id' => 'option1', 'value' => array('foo'=>'foo1', 'bar' => 'bar1') ) ) ) ``` The above shall be converted to the following array. ``` php array( 'things' => array( 'option1' => array('foo' => 'foo1', 'bar' => 'bar1') ) ) ``` When using VariableNode for value element, it's also possible to mix different types of value elements: ``` php array( 'things' => array( array('id' => 'option1', 'value' => array('foo'=>'foo1', 'bar' => 'bar1')), array('id' => 'option2', 'value' => 'value2') ) ) ``` The above shall be converted to the following array. ``` php array( 'things' => array( 'option1' => array('foo'=>'foo1', 'bar' => 'bar1'), 'option2' => 'value2' ) ) ``` Commits ------- b587a72 [config] Fix issue when key removed and left value only

zerustech · 2016-12-14T09:32:20Z

@fabpot It's my pleasure.

zerustech force-pushed the config-attribute-as-key-value-only branch from 1d67c56 to 96f65cc Compare March 27, 2015 15:53

zerustech closed this Mar 27, 2015

zerustech reopened this Mar 27, 2015

zerustech force-pushed the config-attribute-as-key-value-only branch from 96f65cc to 4e750d8 Compare March 30, 2015 11:27

zerustech closed this Mar 30, 2015

zerustech reopened this Mar 30, 2015

zerustech force-pushed the config-attribute-as-key-value-only branch from 4e750d8 to 260cac4 Compare March 30, 2015 11:30

zerustech closed this Mar 30, 2015

zerustech reopened this Mar 30, 2015

zerustech force-pushed the config-attribute-as-key-value-only branch 2 times, most recently from beda431 to 28d2fbb Compare July 8, 2015 11:41

zerustech mentioned this pull request Jul 14, 2015

[config] Error occurs when the key attribute is removed from the child node and only a key named 'value' is left there #15270

Closed

zerustech force-pushed the config-attribute-as-key-value-only branch from 28d2fbb to 7859855 Compare July 14, 2015 09:21

fabpot added the Config label Oct 5, 2015

fabpot reviewed Mar 2, 2016
View reviewed changes

zerustech force-pushed the config-attribute-as-key-value-only branch from 7859855 to d4b2a2f Compare March 3, 2016 08:44

javiereguiluz added the Bug label Mar 3, 2016

carsonbot added the Status: Needs Work label Mar 7, 2016

zerustech force-pushed the config-attribute-as-key-value-only branch from d4b2a2f to 66bd18d Compare March 10, 2016 12:19

fabpot reviewed Sep 14, 2016

View reviewed changes

zerustech force-pushed the config-attribute-as-key-value-only branch from 66bd18d to 12488c5 Compare September 19, 2016 07:34

nicolas-grekas added this to the 2.7 milestone Dec 6, 2016

fabpot closed this Dec 14, 2016

This was referenced Jan 12, 2017

Release v2.7.23 #21258

Merged

Release v2.8.16 #21259

Merged

Release v3.1.9 #21260

Merged

Release v3.2.2 #21261

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[config] Fix issue when key removed and left value only #14082

[config] Fix issue when key removed and left value only #14082

Uh oh!

zerustech commented Mar 27, 2015

Uh oh!

fabpot Mar 2, 2016

Uh oh!

fabpot commented Mar 2, 2016

Uh oh!

fabpot commented Mar 2, 2016

Uh oh!

zerustech commented Mar 3, 2016

Uh oh!

fabpot commented Mar 3, 2016

Uh oh!

zerustech commented Mar 4, 2016

Uh oh!

xabbuh commented Mar 7, 2016

Uh oh!

zerustech commented Mar 10, 2016

Uh oh!

fabpot Sep 14, 2016

Uh oh!

zerustech Sep 19, 2016

Uh oh!

fabpot commented Sep 14, 2016

Uh oh!

fabpot commented Dec 14, 2016

Uh oh!

zerustech commented Dec 14, 2016

Uh oh!

Uh oh!

Uh oh!

[config] Fix issue when key removed and left value only #14082

[config] Fix issue when key removed and left value only #14082

Uh oh!

Conversation

zerustech commented Mar 27, 2015

Uh oh!

fabpot Mar 2, 2016

Choose a reason for hiding this comment

Uh oh!

fabpot commented Mar 2, 2016

Uh oh!

fabpot commented Mar 2, 2016

Uh oh!

zerustech commented Mar 3, 2016

Uh oh!

fabpot commented Mar 3, 2016

Uh oh!

zerustech commented Mar 4, 2016

Uh oh!

xabbuh commented Mar 7, 2016

Uh oh!

zerustech commented Mar 10, 2016

Uh oh!

fabpot Sep 14, 2016

Choose a reason for hiding this comment

Uh oh!

zerustech Sep 19, 2016

Choose a reason for hiding this comment

Uh oh!

fabpot commented Sep 14, 2016

Uh oh!

fabpot commented Dec 14, 2016

Uh oh!

zerustech commented Dec 14, 2016

Uh oh!

Uh oh!