Fixed incorrect replacement of route elements beginning with same string #2991

Merged
merged 9 commits into from Mar 11, 2014

Conversation

Projects
None yet
4 participants
@mikegibson

Where a route contains a named element which starts with the name of a previous named element, the router is matching these incorrectly.

For example:

Router::connect('/:product/:productsPart', array('controller' => 'products', 'action' => 'view'));
echo Router::url(array('controller' => 'products', 'action' => 'view', 'product' => 'widget', 'productsPart' => 2));

gives /widget/widgetsPart when it should give /widget/2

I have fixed this by simply ordering the params in reverse length order before checking for matches.

@jippi

This comment has been minimized.

Show comment
Hide comment
@jippi

jippi Mar 10, 2014

Member

Can you please provide a test case for this issue?

Member

jippi commented Mar 10, 2014

Can you please provide a test case for this issue?

@mikegibson

This comment has been minimized.

Show comment
Hide comment

Done

@lorenzo

View changes

lib/Cake/Routing/Route/CakeRoute.php
+ $keys = $this->keys;
+
+ // Sort the keys in reverse order by length to prevent mismatches
+ uasort($keys, array($this, '_sortKeys'));

This comment has been minimized.

@lorenzo

lorenzo Mar 10, 2014

Member

maybe just use uasort($keys, 'strlen'); $keys = reverse($keys) to avoid the extra sorting method

@lorenzo

lorenzo Mar 10, 2014

Member

maybe just use uasort($keys, 'strlen'); $keys = reverse($keys) to avoid the extra sorting method

This comment has been minimized.

@markstory

markstory Mar 10, 2014

Member

Fewer function calls is important here.

@markstory

markstory Mar 10, 2014

Member

Fewer function calls is important here.

This comment has been minimized.

@mikegibson

mikegibson Mar 10, 2014

The example you provided doesn't work, not sure if this is possible without a callback?

@mikegibson

mikegibson Mar 10, 2014

The example you provided doesn't work, not sure if this is possible without a callback?

This comment has been minimized.

@lorenzo

lorenzo Mar 10, 2014

Member

it should work, what is the result you are getting? Btw with reverse I meant array_reverse

@lorenzo

lorenzo Mar 10, 2014

Member

it should work, what is the result you are getting? Btw with reverse I meant array_reverse

This comment has been minimized.

@mikegibson

mikegibson Mar 10, 2014

AFAIK strlen returns the length of the string where uasort expects either 0, 1 or -1 in return? All examples I've found with strlen have to use a separate comparison function. I've pushed a new commit using an alternate sorting method.

@mikegibson

mikegibson Mar 10, 2014

AFAIK strlen returns the length of the string where uasort expects either 0, 1 or -1 in return? All examples I've found with strlen have to use a separate comparison function. I've pushed a new commit using an alternate sorting method.

@markstory markstory added this to the 2.4.7 milestone Mar 10, 2014

@mikegibson

This comment has been minimized.

Show comment
Hide comment
@mikegibson

mikegibson Mar 10, 2014

I have sorted the keys in a different way eliminating the need for an additional method in the CakeRoute class

I have sorted the keys in a different way eliminating the need for an additional method in the CakeRoute class

@mikegibson

This comment has been minimized.

Show comment
Hide comment
@mikegibson

mikegibson Mar 10, 2014

Sorry that's not going to work either as the lengths may be the same... I'll think about how to do this without a comparison method...

Sorry that's not going to work either as the lengths may be the same... I'll think about how to do this without a comparison method...

@mikegibson

This comment has been minimized.

Show comment
Hide comment
@mikegibson

mikegibson Mar 10, 2014

Just added another commit which should improve things

Just added another commit which should improve things

@markstory

This comment has been minimized.

Show comment
Hide comment
@markstory

markstory Mar 10, 2014

Member

There are numerous test failures on the 5.2 and 5.3 builds.

Member

markstory commented Mar 10, 2014

There are numerous test failures on the 5.2 and 5.3 builds.

@mikegibson

This comment has been minimized.

Show comment
Hide comment
@mikegibson

mikegibson Mar 10, 2014

Failures were due to different behaviour of array_combine with empty arrays in PHP <5.4. I'm now testing if $keys are empty before sorting and all tests are passing.

Failures were due to different behaviour of array_combine with empty arrays in PHP <5.4. I'm now testing if $keys are empty before sorting and all tests are passing.

@markstory markstory self-assigned this Mar 10, 2014

@mikegibson

This comment has been minimized.

Show comment
Hide comment
@mikegibson

mikegibson Mar 10, 2014

I believe it's Travis CI that failed here not my code, this is all working now.

I believe it's Travis CI that failed here not my code, this is all working now.

markstory added a commit that referenced this pull request Mar 11, 2014

Merge pull request #2991 from mikegibson/route_params
Fixed incorrect replacement of route elements beginning with same string

@markstory markstory merged commit 0c207db into cakephp:master Mar 11, 2014

1 check passed

default The Travis CI build passed
Details

markstory added a commit that referenced this pull request Mar 11, 2014

Only sort the keys once per request instead of on each match.
Sorting the keys property by value sorts keys with the same prefix for
free. This does change the order of the keys, but I don't think that is
actually a large issue as it is just a list.

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