New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into
base: 2.3
from

Conversation

Projects
None yet
6 participants
@zerustech
Contributor

zerustech commented Mar 27, 2015

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

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Mar 2, 2016

Member

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

Member

fabpot commented Mar 2, 2016

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

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Mar 2, 2016

Member

Apart from the minor formatting comments, 👍

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

Member

fabpot commented Mar 2, 2016

Apart from the minor formatting comments, 👍

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

@zerustech

This comment has been minimized.

Show comment
Hide comment
@zerustech

zerustech Mar 3, 2016

Contributor

@fabpot Done, please check. Thanks.

Contributor

zerustech commented Mar 3, 2016

@fabpot Done, please check. Thanks.

@javiereguiluz javiereguiluz added the Bug label Mar 3, 2016

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Mar 3, 2016

Member

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

Member

fabpot commented Mar 3, 2016

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

@zerustech

This comment has been minimized.

Show comment
Hide comment
@zerustech

zerustech Mar 4, 2016

Contributor

@fabpot Looks like it was caused by TwigBundle :

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

Time: 17.06 seconds, Memory: 13.25Mb
Contributor

zerustech commented Mar 4, 2016

@fabpot Looks like it was caused by TwigBundle :

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

Time: 17.06 seconds, Memory: 13.25Mb
@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Mar 7, 2016

Member

@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

Member

xabbuh commented Mar 7, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@zerustech

zerustech Mar 10, 2016

Contributor

@xabbuh @fabpot Fixed the following issues:

  1. When the 'value' element in an array is null, it does not replace its wrapper array, because isset() returns false on null element.
  2. 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.
Contributor

zerustech commented Mar 10, 2016

@xabbuh @fabpot Fixed the following issues:

  1. When the 'value' element in an array is null, it does not replace its wrapper array, because isset() returns false on null element.
  2. 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

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Sep 14, 2016

Member

👍 for merge in 2.7 after the small asked change.

Member

fabpot commented Sep 14, 2016

👍 for merge in 2.7 after the small asked change.

[config] Fix issue when key removed and left value only
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 | #15270
| License       | MIT
| Doc PR        | n/a

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

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Dec 14, 2016

Member

Thank you @zerustech.

Member

fabpot commented Dec 14, 2016

Thank you @zerustech.

fabpot added a commit that referenced this pull request Dec 14, 2016

bug #14082 [config] Fix issue when key removed and left value only (z…
…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

@fabpot fabpot closed this Dec 14, 2016

@zerustech

This comment has been minimized.

Show comment
Hide comment
@zerustech

zerustech Dec 14, 2016

Contributor

@fabpot It's my pleasure.

Contributor

zerustech commented Dec 14, 2016

@fabpot It's my pleasure.

@fabpot fabpot referenced this pull request Jan 12, 2017

Merged

Release v2.7.23 #21258

This was referenced Jan 12, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment