Skip to content
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

argument should be rawurldecode, not urldecode. #2000

Closed
wants to merge 1 commit into from

Conversation

luzluna
Copy link

@luzluna luzluna commented Sep 27, 2016

in rfc2396.

The plus "+", dollar "$", and comma "," characters have been added to
those in the "reserved" set, since they are treated as reserved
within the query component.
So reserved chars should be bypassed to users, if in argument field.

…"$", and comma "," characters are treated as reserved within the query component.
@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.18% when pulling 01593e3 on luzluna:3.x into 7c33189 on slimphp:3.x.

@akrabat
Copy link
Member

akrabat commented Sep 27, 2016

What are the BC implications?

@luzluna
Copy link
Author

luzluna commented Sep 28, 2016

if route defined like this
$app->get('/hello/{name}', function (Request $request, Response $response) { $name = $request->getAttribute('name');
and request is '/hello/+apple'

now
$name is ' apple'

if apply this patch
$name is '+apple'

@johnhunt
Copy link

johnhunt commented Oct 4, 2016

I'd definitely say this is a BC break. It might be a better idea to add a second argument to getAttribute() to return the value using rawurldecode():

$app->get('/hello/{name}', function (Request $request, Response $response) { $name = $request->getAttribute('name', true);

However, this might be a bit too uglifying..

@akrabat
Copy link
Member

akrabat commented Oct 4, 2016

getAttribute() is part of PSR-7...

@johnhunt
Copy link

johnhunt commented Oct 4, 2016

I feel as though this belongs in slim 4, the implications of putting it in the 3.x branch could be pretty severe. Does anyone else agree?

@akrabat akrabat added this to the 4.0 milestone Oct 11, 2016
@akrabat akrabat added the Slim 4 label Oct 11, 2016
@akrabat akrabat removed this from the 4.0 milestone Oct 11, 2016
@geggleto
Copy link
Member

geggleto commented Mar 6, 2017

👍

@geggleto
Copy link
Member

@akrabat can we close this one and ask the author to target 4.x?

@akrabat
Copy link
Member

akrabat commented Mar 22, 2017

I hadn't noticed! @luzluna, please rebase against 4.x and edit the PR to be against 4.x too.

@akrabat
Copy link
Member

akrabat commented Jun 29, 2017

Can we raise a new PR for Slim-Http please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants