RestRouteCollection sorts routes in wrong order #379

Closed
kor3k opened this Issue Feb 21, 2013 · 9 comments

Comments

Projects
None yet
6 participants

kor3k commented Feb 21, 2013

let's say i have these routes:

get_emails : /emails -> cgetAction
get_emails_sent : /emails/sent -> cgetSentAction
get_email : /emails/{email} -> getAction

now the comparizon in RestRouteCollection line 90:

return strcmp($route1, $route2);

sorts routes by name ascendingly, so route get_emails_sent is placed after get_email , therefore it is never matched and url /emails/sent matches as get_email with email = sent.

we need to sort them descendigly to put get_email on the bottom by swapping routes in arguments as:

return strcmp($route2, $route1);
Owner

stof commented Feb 21, 2013

Well, as the order of patterns is meaningful for the router, sorting names in descending order could also lead to issues. What we bother about is not the order of names

kor3k commented Feb 21, 2013

yes, but these route names are generated by FOSRestBundle. so at the
current behavior you cannot use routes/action
like get_resources_something -> /resources/something because it always
evalutes as get_resource.

kor3k commented Feb 22, 2013

or could you please create something like RestRouteCollectionSorterInterface with sort( $route1 , $route2 ) method so users can customize the sorting mechanism?

Owner

lsmith77 commented Mar 13, 2013

@Elexy another ticket you could have a look at.

Elexy commented Mar 13, 2013

I can't until next week. on vacation now.

zefrog commented Mar 15, 2013

Just a quick note as I am also impacted, following a recent upgrade to sf2.2 and friendsofsymfony/rest-bundle 0.11.

I have two functions in the same controller, one is more a service than a resource, but anyway it was working using 0.9 and it is not using 0.11 :

public function getContactsSearchAction()
{
    // whatever
}

public function getContactsAction($id) 
{
     // whatever
}

This is causing the router:debug command to output the routes in the wrong order, thus matching /contacts/search.json to the wrong route :

api_contacts_get_contacts GET ANY /contacts/{id}.{_format}
api_contacts_get_contacts_search GET ANY /contacts/search.{_format}

I agree with @stof that we do not bother the name of the routes, but the order in which they come in the controller, aren't we ? Returning 0 in the uksort callable function is not helping either. I am missing something ?

Any updates or workarounds for this issue? Just upgraded an application to Symfony 2.2 and I'm running into this problem. The issue that I'm running into is the same described above by @zefrog.

Owner

lsmith77 commented Jun 4, 2013

someone needs to dig in and come up with a solution.

zefrog commented Jun 12, 2013

I am currently digging into this issue because sf2.3 is causing the same problem.

I am tempted to think this is a Symfony issue, because the routes order is the same using 2.1 and 2.3. But whereas Symfony2.1 matches the second route, the most specific one, Symfony2.3 matches the first one. And this is correct, as it is clearly stated in the Router documentation :

Earlier Routes always Win

So I believe this is a Symfony2.1 "feature"... That was removed for some reason (I will look further).

A quick fix I came up with is to change the all() function of RestRouteCollection. I don't get the point sorting the routes by HTTP method, rather than letting the developer chose the best order. Is there one ?

If this feature is used, I suggest taking the custom methods routes out of the $routes array, and moving them at the top, without touching the others. I am preparing a pull request for this.

Regards.

@zefrog zefrog pushed a commit to zefrog/FOSRestBundle that referenced this issue Jun 12, 2013

@mdouailin mdouailin fixing issue #379 - let custom method routes first, developer order f…
…or standard methods
07f22c3

@zefrog zefrog pushed a commit to zefrog/FOSRestBundle that referenced this issue Jun 12, 2013

@mdouailin mdouailin added tests for #379 - route order is kept if the method is not a cus…
…tom one
d1e2fb2

lsmith77 closed this Jun 13, 2013

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