8000 [Config] Remap XML before normalization by ro0NL · Pull Request #21052 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Config] Remap XML before normalization #21052

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

[Config] Remap XML before normalization #21052

wants to merge 2 commits into from

Conversation

ro0NL
Copy link
Contributor
@ro0NL ro0NL commented Dec 25, 2016 •
edited
Loading
Q A
Branch? master
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes (should add one for regression though)
Fixed tickets #21051 (comment)
License MIT
Doc PR symfony/symfony-docs#...

You never seem to get the remapped value with things like beforeNormalization() etc.

Meaning you never get the normalized form which makes deprecating for instance really hard ;-)

@ro0NL
Copy link
Contributor Author
ro0NL commented Dec 26, 2016 •
edited
Loading

Added failing test that proves we get a de-normalized form in normalization closures.

This PR fixes XML remapping, but setting the key attrribute actually still happens in PrototypedArrayNode::normalizeValue (ie. post normalization).

From a normalization closure;

1) Symfony\Component\Config\Tests\Definition\ArrayNodeTest::testPreNormalize
with data set #4 (array(array(array('foo', 'foo'), array('bar', 'bar'))), array(array(array('foo'), array('bar'))))
Failed asserting that Array &0 (
    'plural' => Array &1 (
        0 => Array &2 (
            'x' => 'foo'
            'bar' => 'foo'
        )
        1 => Array &3 (
            'x' => 'bar'
            'bar' => 'bar'
        )
    )
) is identical to Array &0 (
    'plural' => Array &1 (
        'foo' => Array &2 (
            'bar' => 'foo'
        )
        'bar' => Array &3 (
            'bar' => 'bar'
        )
    )
).

And without this PR you'd even get singular here.

Same for replacing equivalent values...

@ro0NL
Copy link
Contributor Author
ro0NL commented Dec 26, 2016 •
edited
Loading

If we agree with behavior change in master i propose to go with;

  • normalize
    • preNormalize
      • normlize keys
      • remap XML attributes
      • assign key attribute
      • replace equivalent values
    • run normalization closures (user normalization starts here)
    • normalizeValue (ie. post normalization)
      • normalize childs
      • check extra keys (actually validation?)

Basically this is about separating child normalization (post) from technical normalization (pre), where user normalization lives in between.

From a user normalization closure you can add a (de)normalized child, but you should not rely on existing children to be fully normalized as they are out of scope for the current node.
8000

@ro0NL ro0NL mentioned this pull request Dec 26, 2016
Closed
@nicolas-grekas
Copy link
Member

Just a confirmation: is this PR for master or for a lower branch?

@ro0NL
Copy link
Contributor Author
ro0NL commented Dec 26, 2016

Depends on SF's policy on behavioral changes :)

I think we should do the master+changelog approach on this. I already imagine people relying on certain keys.. or not expecting certain ones for that matter (to make things work perhaps).

@nicolas-grekas nicolas-grekas added this to the 3.x milestone Dec 27, 2016
@xabbuh
Copy link
Member
xabbuh commented Dec 30, 2016

How would you write code then that has to support different Symfony versions?

@ro0NL
Copy link
Contributor Author
ro0NL commented Jan 4, 2017

Maybe adding afterNormalization is the better approach.. im not sure where where pre stops, and post begins.

Imo. XML remapping and key normalization are kinda at the same level.. but maybe im wrong? I can understand before normalization actually runs before treatFalseLike etc. However practically you need a point where you just know $v['enabled'] is set/normalized. Hence afterNormalization could work.

@stof
Copy link
Member
stof commented Mar 30, 2017

this PR is a BC break, and then the name beforeNormalization does not make sense anymore, as it happens after.

Btw, if you need to know whether whether the feature is enabled or no, it probably means you need to run your logic after the merging of different files too (as it can be enabled in a different file). So use ->validate() instead of beforeNormalization.

@ro0NL
Copy link
Contributor Author
ro0NL commented Mar 30, 2017

i believe my issue was one where i got a different value depending on the format (YAML gives plural, XML singular) which i didnt really expected

👍 for validate.. that is practically afterNormalization already

@ro0NL
Copy link
Contributor Author
ro0NL commented Aug 31, 2017

Closing for now :) not sure anymmore what to do here. I believe moving things to validate() is the right approach.

@ro0NL ro0NL closed this Aug 31, 2017
@ro0NL ro0NL deleted the config/remap-xml branch August 31, 2017 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Bug Config Status: Needs Review
Projects
None yet
Milestone
3.4
Development

Successfully merging this pull request may close these issues.
5 participants
@ro0NL @nicolas-grekas @xabbuh @stof @carsonbot
0