Routing Issue with Implicit Naming and an Uncountable Word #351

Closed
Bendihossan opened this Issue Jan 3, 2013 · 12 comments

Comments

Projects
None yet
2 participants
@Bendihossan
Contributor

Bendihossan commented Jan 3, 2013

Where I have the following in my controller:

/**
* @Prefix("{_locale}")
* @RouteResource("Content")
 */
class ContentController extends Controller
    {

    /**
     * Returns all content
     *
     * @param Request $request
     *
     * @return View
     */
    public function cgetAction(Request $request)
    {
         // ...
    }

    /**
     * Returns a single item of content
     *
     * @param Request $request
     *
     * @return View
     */
    public function getAction(Request $request)
    {
        // ...
    }
}

... the bundle generates the route for a single item of content but not for the resource collection:

get_content                    GET      /{_locale}/content/{id}.{_format}

If I don't use the implicit naming, and use the method signatures getContentAction and getContentsAction action then a route is generated for the collection but it is:

get_content                    GET      /{_locale}/content/{id}.{_format}
get_contents                   GET      /{_locale}/contents.{_format}

I then end up with inconsistent resource URLs because of the name of the action.

What I'm aiming for is:

get_content                    GET      /{_locale}/content/{id}.{_format}
get_contents                   GET      /{_locale}/content.{_format}

I believe the reason for this is that 'Content' is an uncountable word. I get the same results with 'Media' and 'Sheep' as the RouteResource too. If I use a word which is countable like 'Mug' or 'Phone' then routing works:

get_phone                      GET      /{_locale}/phones/{id}.{_format}
get_phones                     GET      /{_locale}/phones.{_format}
@Bendihossan

This comment has been minimized.

Show comment Hide comment
@Bendihossan

Bendihossan Jan 3, 2013

Contributor

I think the reason the bundle doesn't generate a route for the collection resource is that it tries to generate two routes with the same name (get_content) for both the collection and the individual resource URLs. I think to fix this issue we want to check to the uncountable static array in Util\Pluralization and modify the name of the route if we're trying to get a collection (cgetAction) rather than the singular resource.

I'm still new to Symfony and haven't developed a bundle myself so I don't quite understand how information flows around the FosRestBundle so I don't know if what I'm suggesting is that easy.

Contributor

Bendihossan commented Jan 3, 2013

I think the reason the bundle doesn't generate a route for the collection resource is that it tries to generate two routes with the same name (get_content) for both the collection and the individual resource URLs. I think to fix this issue we want to check to the uncountable static array in Util\Pluralization and modify the name of the route if we're trying to get a collection (cgetAction) rather than the singular resource.

I'm still new to Symfony and haven't developed a bundle myself so I don't quite understand how information flows around the FosRestBundle so I don't know if what I'm suggesting is that easy.

@lsmith77

This comment has been minimized.

Show comment Hide comment
@lsmith77

lsmith77 Jan 3, 2013

Owner

your analysis sounds correct. imho the routing part of this bundle should probably be rewritten from the ground up eventually to better deal with all of these edge cases.

Owner

lsmith77 commented Jan 3, 2013

your analysis sounds correct. imho the routing part of this bundle should probably be rewritten from the ground up eventually to better deal with all of these edge cases.

@Bendihossan

This comment has been minimized.

Show comment Hide comment
@Bendihossan

Bendihossan Jan 3, 2013

Contributor

Am I right in saying that the function at fault is generateRouteName in RestActionReader? https://github.com/FriendsOfSymfony/FOSRestBundle/blob/master/Routing/Loader/Reader/RestActionReader.php#L296

Contributor

Bendihossan commented Jan 3, 2013

Am I right in saying that the function at fault is generateRouteName in RestActionReader? https://github.com/FriendsOfSymfony/FOSRestBundle/blob/master/Routing/Loader/Reader/RestActionReader.php#L296

@lsmith77

This comment has been minimized.

Show comment Hide comment
@lsmith77

lsmith77 Jan 3, 2013

Owner

hmm can't say from the top of my head .. but its definitely in that class

Owner

lsmith77 commented Jan 3, 2013

hmm can't say from the top of my head .. but its definitely in that class

@Bendihossan

This comment has been minimized.

Show comment Hide comment
@Bendihossan

Bendihossan Jan 3, 2013

Contributor

@lsmith77 I have a (hacky) fix for this: https://gist.github.com/bd08ec2488eb816c561d

Wit this change the bundle generates this output:

get_collection_content         GET      /{_locale}/content.{_format}
get_content                    GET      /{_locale}/content/{id}.{_format}

I've hard coded 'cgetAction' for now, I'll try and find a way around this. If you're otherwise OK with that I will add a test and make a PR.

Contributor

Bendihossan commented Jan 3, 2013

@lsmith77 I have a (hacky) fix for this: https://gist.github.com/bd08ec2488eb816c561d

Wit this change the bundle generates this output:

get_collection_content         GET      /{_locale}/content.{_format}
get_content                    GET      /{_locale}/content/{id}.{_format}

I've hard coded 'cgetAction' for now, I'll try and find a way around this. If you're otherwise OK with that I will add a test and make a PR.

@lsmith77

This comment has been minimized.

Show comment Hide comment
@lsmith77

lsmith77 Jan 3, 2013

Owner

imho we should then always use such a prefix for collections, not just when the c prefix is used. this would of course be a big BC break but it might indeed be the best solution.

Owner

lsmith77 commented Jan 3, 2013

imho we should then always use such a prefix for collections, not just when the c prefix is used. this would of course be a big BC break but it might indeed be the best solution.

@Bendihossan

This comment has been minimized.

Show comment Hide comment
@Bendihossan

Bendihossan Jan 3, 2013

Contributor

How do you detect if the action is a GET for a collection without looking up the method name? I can change the code to do this if you know.

Contributor

Bendihossan commented Jan 3, 2013

How do you detect if the action is a GET for a collection without looking up the method name? I can change the code to do this if you know.

@lsmith77

This comment has been minimized.

Show comment Hide comment
@lsmith77

lsmith77 Jan 3, 2013

Owner

i would need to dig in too .. we do have fairly good tests for the core feature set .. so refactorings are possible without risking gigantic accidental breakage .. but there is the big risk of edge case behavior changes :-/

Owner

lsmith77 commented Jan 3, 2013

i would need to dig in too .. we do have fairly good tests for the core feature set .. so refactorings are possible without risking gigantic accidental breakage .. but there is the big risk of edge case behavior changes :-/

@Bendihossan

This comment has been minimized.

Show comment Hide comment
@Bendihossan

Bendihossan Jan 3, 2013

Contributor

What do you think is the best thing to do? Using the code in the gist as it is with the hard coded 'cgetAction' would limit any potential edge case behaviour changes as it'd be targeted to functions with that method signature? I realise that's not ideal so I don't mind if you don't like that.

This code seems to detect if the route is for a collection or not but it looks messy to me: https://github.com/FriendsOfSymfony/FOSRestBundle/blob/master/Routing/Loader/Reader/RestActionReader.php#L334

Contributor

Bendihossan commented Jan 3, 2013

What do you think is the best thing to do? Using the code in the gist as it is with the hard coded 'cgetAction' would limit any potential edge case behaviour changes as it'd be targeted to functions with that method signature? I realise that's not ideal so I don't mind if you don't like that.

This code seems to detect if the route is for a collection or not but it looks messy to me: https://github.com/FriendsOfSymfony/FOSRestBundle/blob/master/Routing/Loader/Reader/RestActionReader.php#L334

@lsmith77

This comment has been minimized.

Show comment Hide comment
@lsmith77

lsmith77 Jan 4, 2013

Owner

yes generateUrlParts() has become increasingly complex and could definitely use some refactoring .. though i dont have an idea how to improve it really. the only "idea" i currently have is just making generateUrlParts() even messier by returning both the $urlParts as well as a flag if its a collection or not.

Owner

lsmith77 commented Jan 4, 2013

yes generateUrlParts() has become increasingly complex and could definitely use some refactoring .. though i dont have an idea how to improve it really. the only "idea" i currently have is just making generateUrlParts() even messier by returning both the $urlParts as well as a flag if its a collection or not.

@lsmith77

This comment has been minimized.

Show comment Hide comment
@lsmith77

lsmith77 Mar 13, 2013

Owner

@Elexy another ticket you could have a look at.

Owner

lsmith77 commented Mar 13, 2013

@Elexy another ticket you could have a look at.

@lsmith77

This comment has been minimized.

Show comment Hide comment
@lsmith77

lsmith77 May 15, 2014

Owner

fixed by #761

Owner

lsmith77 commented May 15, 2014

fixed by #761

@lsmith77 lsmith77 closed this May 15, 2014

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