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

[WIP] accept header based selection of the controller #561

Closed
wants to merge 1 commit into from

Conversation

lsmith77
Copy link
Member

Essentially the approach here is to register a listener after the core RouterListener that tries to map the controller based on the media type. The media type map is configured inside the route.

Example route:

hello_version:
    pattern:  /liip/version
    defaults:
        _controller_map:
            fallback: '1.0'
            config:
                '1.0': 'liip_hello.world.controller:v1Action'
                '2.0': 'liip_hello.world.controller:v2Action'
        _format: json

Todos:

  • don't really like the MediaType* naming. we already have a MimeTypeListener
  • make it possible to configure a different mapper, should it be possible to configure mappers via the route definition? if so if we support services, we would need a factory that has access to the DIC
  • integrate with the routing generation
  • make it possible to configure version ranges in the default mapper (lift code from https://github.com/composer/composer/blob/master/src/Composer/Package/Version/VersionParser.php or https://github.com/vierbergenlars/php-semver)
  • potentially support a default config by defining some naming convention (potentially allow to configure one naming convention that uses a different controller and another one that uses a different method) so that not all routes need to have an actual _controller_map defined (instead one can just use true to enable the feature)
  • add tests

@newLoki: if you have time to work on any of the above items please let me know via a comment in this ticket.

@willdurand
Copy link
Member

Shouldn't we wait the ExpressionLanguage component?

@lsmith77
Copy link
Member Author

why wait? i mean its here already. are you talking about being able to embed the relevant logic into the routing via the EL?

@willdurand
Copy link
Member

Ja. I would like to create different route definitions rather than a single one with a looooong configuration section.

@adrienbrault
Copy link
Contributor

What about using version constraints like in composer instead ?

EDIT: missed the description, sorry

@stof
Copy link
Member

stof commented Sep 23, 2013

@willdurand Would it be really longer than 2 route definitions duplicating everything except the controller ? I don't expect the controller map to contain tens of entries

@adrienbrault
Copy link
Contributor

Hm, why isnt the version set as a request attributes, and then matched as a regular route requirement ? (Do you think it is possible to translate a version constraint to a regex ?)

@stof
Copy link
Member

stof commented Sep 23, 2013

@adrienbrault this would work if you have the version as a placeholder in the url (and you don't need anything special for it as the routing component alone can handle it). the goal of this PR is to select the version based on the Accept header, not based on some URL placeholder.

@lsmith77
Copy link
Member Author

@adrienbrault see also the discussion after this post #136 (comment)

furthermore .. the regexp based version thingi is just an example. i think it would make sense to provide several and of course the matching could be done by full media type without any regexp at all.

@adrienbrault
Copy link
Contributor

So now my understanding is:

  • We cannot define multiple routes, because only the first one will be matched by the router ?
  • @willdurand suggested to use the RequestMatcherInterface, however it seems that almost everything would have to be rewritten in the Routing component (compiler/optimized dumpers, the matcher itself)

@willdurand
Copy link
Member

We cannot define multiple routes, because only the first one will be matched by the router?

With an UrlMatcher, yes.
The RequestMatcher would fix the issue, but it is only used by the Drupal guys, no one else, so... not a good idea.

@stof sure but they are not the same routes after all, so having two different definitions would make sense. It is already the case with the HTTP method..

@stof
Copy link
Member

stof commented Sep 23, 2013

@willdurand If you want to use multiple routes, you don't need this PR at all.
And btw, Symfony 2.4 is using the RequestMatcher in the core. It was needed to implement the matching of expressions. However, there is no built-in way to register custom functions in the expression engine used by the router yet, so using the expressions to restrict the matching of the route to a specific version is not possible yet, except by defining the whole parsing logic in the expression itself with basic operators.

@willdurand
Copy link
Member

@willdurand If you want to use multiple routes, you don't need this PR at all.

Care to elaborate?

And, is 2.4 released yet?

@stof
Copy link
Member

stof commented Sep 23, 2013

no, it is not released yet.

@adrienbrault
Copy link
Contributor

Then we would just need a way to add negotiation functions to the routing expression language right ?

hello_version_v1:
    pattern:  /liip/version
    defaults:
        _controller: 'liip_hello.world.controller:v1Action'
        _format: json
    condition: 'matchesVersion(request, "1.*")'


hello_version_v2:
    pattern:  /liip/version
    defaults:
        _controller: 'liip_hello.world.controller:v2Action'
        _format: json
    condition: 'matchesVersion(request, "2.*")'

Though I would not like having multiple routes that would generate the same path.

Right now it does not seem easy add functions because : https://github.com/symfony/symfony/blob/5ebaad33e6b6e01b46afb86a9370dfe7e5d136c0/src/Symfony/Component/Routing/Matcher/UrlMatcher.php#L235-L245

@lsmith77
Copy link
Member Author

lsmith77 commented Dec 2, 2013

for more information about conditions in the routing layer see http://symfony.com/doc/current/book/routing.html#book-routing-conditions

@willdurand
Copy link
Member

So, would it be possible to leverage the EL instance part of the routing layer to negotiate the right controller/action now?

I am a bit lost, but it looks like Symfony 2.4 now relies on the RequestMatcher which should solve a few issues. At least my VersionListener could work..

@lsmith77 lsmith77 closed this Oct 20, 2014
@lsmith77 lsmith77 mentioned this pull request Oct 20, 2014
@dbu dbu deleted the media_type_listener branch May 29, 2017 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants