Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] [Routing] Allow multiple controllers per HTTP method #35973

Closed
kunicmarko20 opened this issue Mar 5, 2020 · 17 comments
Closed

[RFC] [Routing] Allow multiple controllers per HTTP method #35973

kunicmarko20 opened this issue Mar 5, 2020 · 17 comments

Comments

@kunicmarko20
Copy link

Description
Instead of a need to write multiple route mappings for different method and controller combination, I would like to expand routing configuration to allow mapping multiple controllers per HTTP method.

Example

Before

test.save:
  path: '/test'
  controller: My\Action\TestSave
  methods: [POST]

test.get:
  path: '/test'
  controller: My\Action\TestGet
  methods: [GET]

test.delete:
  path: '/test'
  controller: My\Action\TestDelete
  methods: [DELETE]

After

test:
  path: '/test'
  controller:
    get: My\Action\TestGet
    post: My\Action\TestSave
    delete: My\Action\TestDelete
@Pierstoval
Copy link
Contributor

Totally okay with this 👍
It would be great to define API endpoints in a really straightforward way and still define classic controllers as responders 😄

@galeaspablo
Copy link
Contributor

What's the proposal for this case?

test:
  path: '/test'
  methods: [DELETE]
  controller:
    get: My\Action\TestGet
    post: My\Action\TestSave

@kunicmarko20
Copy link
Author

What's the proposal for this case?

test:
  path: '/test'
  methods: [DELETE]
  controller:
    get: My\Action\TestGet
    post: My\Action\TestSave

My idea right now would be, if you are using multiple controllers you can't add methods key, so probably an exception, I am open to any other idea/proposal?

@Pierstoval
Copy link
Contributor

Exception at compile-time seems like the best compromise to me

@Tobion
Copy link
Contributor

Tobion commented Mar 7, 2020

-1 as it undermines the principle that a route is a combination of method, uri and defaults (e.g. controller). Also this creates discrepancies between yaml/xml and annotations. For annotation this is irrelevant but widens the differences in declarations in different formats.

@stephanvierkant
Copy link
Contributor

What if you want to have a controller for GET and another controller for both POST and PATCH?

@Pierstoval
Copy link
Contributor

@stephanvierkant Well, instead of duplicating a whole route block, you'll duplicate one single controller name 😄

@kunicmarko20
Copy link
Author

-1 as it undermines the principle that a route is a combination of method, uri and defaults (e.g. controller).

You still have this, methods are just moved under controller.

Also this creates discrepancies between yaml/xml and annotations. For annotation this is irrelevant but widens the differences in declarations in different formats.

If I implement this, it would probably be both for xml and yaml, that would avoid discrepancies?

What if you want to have a controller for GET and another controller for both POST and PATCH?

You can register same controller under different methods, no reason to not allow this.

@Tobion
Copy link
Contributor

Tobion commented Mar 8, 2020

You still have this, methods are just moved under controller.

No, now one route definition maps to several real routes. And if you want to add other conditions to a certain method like expression, you have to revert back to the real route definition again becaues it's not supported in this new style.

@kunicmarko20
Copy link
Author

You still have this, methods are just moved under controller.

No, now one route definition maps to several real routes. And if you want to add other conditions to a certain method like expression, you have to revert back to the real route definition again becaues it's not supported in this new style.

That is true, but my example was a simple one, and in a lot of my cases I don't need any additional conditions or configurations, but I still think this is a nice DX improvement and is worth it.

@Pierstoval
Copy link
Contributor

@Tobion

No, now one route definition maps to several real routes.

Yes, and the same currently applies to localized routes, I don't see any issue with that.

@robwasripped
Copy link

This is a good idea for methods. Would you also want to allow the same definition grouping for other values such as param requirements? eg for routing /foo/{bar} send to one controller for {bar} = 'new' and another for {bar} = int.

Either way it would be nice to be explicit in the syntax that this grouping works only with request methods or that it works with other request details too.

@kunicmarko20
Copy link
Author

kunicmarko20 commented Mar 11, 2020

This is a good idea for methods. Would you also want to allow the same definition grouping for other values such as param requirements? eg for routing /foo/{bar} send to one controller for {bar} = 'new' and another for {bar} = int.

Either way it would be nice to be explicit in the syntax that this grouping works only with request methods or that it works with other request details too.

I get your point and like the idea, maybe it would be a good additional feature if this one gets accepted, for now, I think as you said it would be enough to add methods key under controller so we know grouping is based on methods:

test:
  path: '/test'
  controller:
-    get: My\Action\TestGet
-    post: My\Action\TestSave
-    delete: My\Action\TestDelete     
+    methods:
+        get: My\Action\TestGet
+        post: My\Action\TestSave
+        delete: My\Action\TestDelete

@tsantos84
Copy link
Contributor

The result of this would to have three real routes with different names? (e.g test.get, test.post, test.delete) If yes, you are assuming that your code should rely to http verbs when generating routes (eg. {{ path("test.get") }}). I don't know if this is good for application's design.

@kunicmarko20
Copy link
Author

kunicmarko20 commented Mar 12, 2020

The result of this would to have three real routes with different names? (e.g test.get, test.post, test.delete) If yes, you are assuming that your code should rely to http verbs when generating routes (eg. {{ path("test.get") }}). I don't know if this is good for application's design.

Well, if you are gonna set it like:

test:
  path: '/test'
  controller:
    methods:
        get: My\Action\TestGet
        post: My\Action\TestSave
        delete: My\Action\TestDelete

I would say that you are already using http verbs for methods? I don't see how generating a route test.get or test.post is different than you creating your own route and defining that method is gonna be post or get?

I get your concern about {{ path("test.get") } that probably isn't good for application design, but I have a feeling that people that are gonna use this feature wouldn't use it for twig.

@tsantos84
Copy link
Contributor

tsantos84 commented Mar 12, 2020

I would say that you are already using http verbs for methods? I don't see how generating a route test.get or test.post is different than you creating your own route and defining that method is gonna be post or get?

The point is that we have control on the generated route name and it isn't necessarily related to http verbs when using multiples route definitions. That's my point.

But I need to agree with you that for CRUD applications it could save time and is a concise way to group resource's controllers. My point is just on side effects by using this notation.

@Pierstoval
Copy link
Contributor

@kunicmarko20 why did you close? You had pretty good overall feedback 🤔

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

No branches or pull requests

8 participants