8000 Fix % parameter escaping by lsmith77 · Pull Request #3241 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

Fix % parameter escaping #3241

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 1 commit into from
Closed

Fix % parameter escaping #3241

wants to merge 1 commit into from

Conversation

lsmith77
Copy link
Contributor
@lsmith77 lsmith77 commented Feb 1, 2012

Some how the tests do not match what I would expect in terms of behavior.

With my changes the following:

parameters:
    lala: huhu
    foo:
        bar: %%lala%%1 %ding
        foo:
            bar: %%lala%%1 %ding
        huhu: %kernel.root_dir%

Gives me what I would expect:

// var_dump($foo->getParameter('foo')); die;
array(3) { ["bar"]=> string(13) "%lala%1 %ding" ["foo"]=> array(1) { ["bar"]=> string(13) "%lala%1 %ding" } ["huhu"]=> string(41) "/Users/lsmith/htdocs/symfony-standard/app" }

But as you can see the test suite fails quite miserably:
http://travis-ci.org/#!/lsmith77/symfony/builds/611912

@vicb
Copy link
Contributor
vicb commented Feb 1, 2012

@lsmith77 I remember that this behavior has been changed not so long ago, you might want to check the history, was it related to 99011ca ?

@vicb
Copy link
Contributor
vicb commented Feb 2, 2012

@ericclemmons can you help here ? (85ca8e3)

@ericclemmons
Copy link
Contributor

@vicb @lsmith77 I noticed the same escaping issues with %%, where previously they were not replaced with a single % in some situations.

My patch was ensuring that parameters names were immediately surrounded by % with no spaces.

I'll take a look at your example above on the 2.0 branch since yes, your build fails spectacularly :)

@stof
Copy link
Member
stof commented Feb 2, 2012

@lsmith77 could it be possible that you try to resolve the same parameter twice when resolving the parameter bag ? The second time, it would break things if the escaping has already been replaced.

The parameter which fails in the tests look like file%%link%%format so the resolving would lead to file%link%format with your patch, which indeed looks like a string containing a parameter

@stof stof mentioned this pull request Feb 2, 2012
Merged
@stof stof closed this Feb 2, 2012
fabpot added a commit that referenced this pull request Feb 4, 2012
@fabpot
merged branch stof/unescape_paramaters (PR #3260)
c9e8749 
Commits
-------

a7b48c0 Renamed the method
8e13095 Fixed the unescaping of parameters to handle arrays
045f936 Changed the testcase to expect the unescaping only after the resolution
a1b6d4c Added a failing testcase for escaped % in array parameters

Discussion
----------

Unescape paramaters

This fixes the unescaping of % in parameters when it is used in an array.

It is a replacement for @lsmith77's work done in #3241 but with a working fix this time :)
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
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lsmith77 @vicb @ericclemmons @stof
0