Skip to content

Commit

Permalink
Fix issue where named parameters would not be urldecoded.
Browse files Browse the repository at this point in the history 
Fixes #2155
  • Loading branch information
@markstory
markstory committed Oct 26, 2011
1 parent 7547d2c commit 477c492
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 0 deletions.
1 change: 1 addition & 0 deletions lib/Cake/Routing/Route/CakeRoute.php
View file
Expand Up @@ -272,6 +272,7 @@ protected function _parseArgs($args, $context) {
$separatorIsPresent = strpos($param, $namedConfig['separator']) !== false;
if ((!isset($this->options['named']) || !empty($this->options['named'])) && $separatorIsPresent) {
list($key, $val) = explode($namedConfig['separator'], $param, 2);
$val = urldecode($val);
$hasRule = isset($rules[$key]);
$passIt = (!$hasRule && !$greedy) || ($hasRule && !$this->_matchNamed($val, $rules[$key], $context));
if ($passIt) {
Expand Down
11 changes: 11 additions & 0 deletions lib/Cake/Test/Case/Routing/Route/CakeRouteTest.php
View file
Expand Up @@ -381,6 +381,17 @@ public function testMatchWithNamedParametersAndPassedArgs() {
$this->assertFalse($result);
}

public function testParseNamedParametersUrlDecode() {
Router::connectNamed(true);
$route = new CakeRoute('/:controller/:action/*', array('plugin' => null));

$result = $route->parse('/posts/index/page:%CE%98');
$this->assertEquals('Θ', $result['named']['page']);

$result = $route->parse('/posts/index/page[]:%CE%98');
$this->assertEquals('Θ', $result['named']['page'][0]);
}

/**
* test that named params with null/false are excluded
*
Expand Down

9 comments on commit 477c492

@basuke
Copy link
Contributor

@basuke basuke commented on 477c492 Oct 26, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mark,

Last month, I sent a same kind of patch that was rejected because of no more new feature in 1.3.

https://github.com/cakephp/cakephp/pull/214/files

If you include this urldecode thing, you should do the opposite. I think you should urlencode the named args in _writeUrl, don't you?

@markstory
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps. I kind of hate named parameters and want them to die in a fire, I could see an argument for urlencoding outbound named parameters in 2.0. Its still early enough in that release cycle that a change like that could be done.

@basuke
Copy link
Contributor

@basuke basuke commented on 477c492 Oct 26, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hate them too. Anyway, please add urlencode or rawurlencode in _writeUrl(). If you decode it, you must encode it again.

@ADmad
Copy link
Member

@ADmad ADmad commented on 477c492 Oct 26, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mark, how about we at-least dig its grave in 2.1? Removing it totally at this point might not go down too well with the community so keep it for BC but remove core's dependency on it and by default use regular query string wherever possible (eg. pagination).

@jeremyharris
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think they should go away as well, but I also think this is a much bigger discussion. Perhaps an RFC on lighthouse? Have to deal with things like merging the two (if we keep both), perhaps giving users an option to choose 1 or the other, etc.

@ADmad
Copy link
Member

@ADmad ADmad commented on 477c492 Oct 26, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah an RFC on LH and/or topic on cakephp-core google group would be a start.

@markstory
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we can remove them in 2.1, but for 3.0 they are totally going to die. The biggest outstanding problem with removing them for me is how to properly handle querystring parameters. Do we stick with the '?' => array(...) system currently in use, or just use all non routing keys in an array as query string arguments? If we do that there are some other issues around handling passed arguments as well. Unfortunately, removing them isn't as simple as it looks at first glance :(.

@jeremyharris
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the users should explicitly define what should be passed in the query string. Since we're taking some of the magic that makes life miserable out anyway, I think using all extra keys as query args is on that falls in that category of "too much magic." What if you want to pass an action arg? Maybe that's just my opinion though. I'm interested to see what everyone else thinks.

@markstory
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess both named params and passed arguments should be urlencoded, as the both could easily contain spaces or non-ascii characters. I'll write a patch for this.

Please sign in to comment.