added ability to determine route "resources" from the Controller

[lsmith77]

still needs some tests (especially with subresources), documentation and review

please also see https://github.com/liip/LiipHelloBundle/compare/rest_class for an example

@everzet @beberlei

[stof]

The default value looks weird here. Shouldn't it be an empty string (or null) ?

[lsmith77]

why? 'rest' is the default/current behavior

[stof]

it is the default for the type. but this is not what the RestActionReader receives. It receives a resource (this does not affect your example as the default value is not really used anyway: the RestControllerReader always pass it explicitly as an empty string for the new behavior).

[lsmith77]

ah .. i see what you mean now .. yup that code messed up during refactoring. will have a look

[stof]

isn't it a BC break ? From what I see, the current behavior is achieved by using rest_class as type whereas the current type is rest

[stof]

nvm, I misunderstood the behavior before looking at the example

[everzet]

@lsmith77 could you give me some reasoning behind this, cuz i'm totally lost (but still interested in understanding) :)

[stof]

@everzet Allowing to omit the resource name from the methods, thus allowing to implement these methods using traits (or using a base class) for different controllers.

[asm89]

• What will happen if you have a ArticleController#getArticleAction() in this new setup?
• Can't we keep only type: rest and add an annotation no indicate that the name of the resource should be guessed from the class (or configured in that annotation)? Something like: @REST\Resource()? I think the responsibility for this should be at class level, not configuration (how you import the routes).

Apart from these two questions +1 from me. Having getAction() in a base rest class or trait would be very nice.

[lsmith77]

@everzet what @stof said .. this way you can also have a generic base class if you are not yet on 5.4.

for subresources our concept was to require separate controllers.

ie. UserController for /users, UserTopicController for /users/topics etc.

@asm89 I discussed this option with @beberlei but we didnt want to force the use of annotations. that being said, i think i now know a bit more about routing and will make another attempt at trying to add a new parameter to the routes to be able configure this new behavior.

[beberlei]

@everzet

Several reasons from my side:

• type: rest - forced lots of duplication in method names, that the controller already had.
• no reusibility of actions through traits or a base controller possible, since resource name is hardcoded in action names.

[asm89]

My take on the issue is injecting "User" or "Users" depending on the amount of parameters on the method, but always doing this for all the methods in the class. Code! Mind it is in PoC state. So hardcoded class with resource and adjusted the original test.
Plus:

• Easy to imagine how the method will get mapped. The resource will always be inserted.
• No ambiguous mapping (e.g. getAction and getUserAction getting will never mapped to the same route)

Minus:

• You can't have getFooBarsAction mapping to /foos/{foo}/bars in your UserController anymore, but I think that's a plus.

What is still an issue though is how to map the following:

# works                                                                                                               
getAction()    -> getUsersAction()                                                                                    
getAction($id) -> getUserAction($id)                                                                                  

These methods can't co-exist in the same class in php.

A more explicit solution might be adding a 'collection' prefix to the predefined keywords? For example cget, cpost, ...? This way we can avoid having the getList special case.

getAction($id)  GET  /users/{id}                                                                                      
cgetAction      GET  /users                                                                                           
postAction($id) POST /users/{id}                                                                                      
cpostAction()   POST /users                                                                                           

[lsmith77]

@asm89 i have done some refactoring to radically simplify the code base. i am fairly happy with things now .. except for the logic for how to determine if to pluralize or not inside getHttpMethodAndResourcesFromMethod().

Right now I am using "List" as well as the conventional http methods to trigger the pluralization. Switching to instead prefixing the http method with "c" seems less intuitive.

Also I am not sure about the special rule for conventional http methods. Right now one needs to write newArticlesAction() and imho it would be nicer to be able to write newArticleAction(). I haven't changed this, but it would be easy to do. But what I did make possible is to write newAction() instead of newListAction().

[lsmith77]

ok .. i decided i didnt like "List" as a special resource, since its not a standard term in the REST world. switching to "Collection" seemed better .. but that seemed a bit long and stuff like "postCollectionAction" still sounded stupid.

as @asm89 pointed out we already have special handling for the http methods, so it makes more sense to do such stuff there as well. for now i went with the "c" prefix.

[asm89]

For what it's worth, I have adjusted my "fork" of this PR (which was not behaving the same as the implementation of @lsmith77 before) to also include the cget changes and I added tests for the new behaviour ().

@lsmith77 I have a separate commit with the tests that you can cherry-pick if you want: asm89/FOSRestBundle@798d5a2.

[lsmith77]

@asm89 can you summarize the behavior or implementation advantages of your approach?

[asm89]

I don't think it differs much from your code anymore since we gradually worked to the point where we do the same. :)

• It always injects the resource in each method of the controller
• Based on the amount of arguments in the method User or Users will be injected
• if Users in injected , thehttpMethods cget etc are aliases to get

In the implementation I didn't change any existing tests (no BC breaks?), I only added new tests to verify the new behaviour (you can cherry-pick them :) asm89/FOSRestBundle@798d5a2).

[lsmith77]

ok .. i cherry picked your tests and made them work with my implementation. so from my POV the main thing missing now is docs .. for now see the tests and the LiipHelloBundle example

one area where i am still a bit unsure if it should be possible to configure sub resources via the controller name because we already support this via the "parent" .. then again if i want to split up my controller into a UserController and a UserCommentController .. i might also want to have a CommentController .. so leaving out the parent resource from the controller name could lead to collisions. so i think its fine the way it is.

@everzet can you have a look?

[asm89]

👍 Let's see what @everzet has to say about it.

[javiern]

it seems nice,
im trying to implent something like this by abstracting the Reosurce and Representation concept to a common interface but not being able so far to get results... i made a post in symfony user group to see if some one can give me a hand or if im totally wrong in my aproach.

https://groups.google.com/forum/?fromgroups#!topic/symfony2/7uKWq9OX6S0%5B1-25%5D

[asm89]

ping @everzet

[lsmith77]

oops .. didnt realize the PR didnt include docs .. any volunteers?

[pierrre]

How to create a POST route with this patch?
I'd like to create "POST /users/register".

I've created a function named postRegisterAction() in my UsersController,
but it creates "POST /users/registers"... :(

[stof]

@pierrre if you don't like the generated pattern, you can always change it by adding the @Post annotation to give a pattern explicitly.