symfony-cmf / Routing

Routing component building on the Symfony Routing component
Other
288 stars 69 forks source link

add a post match event #93

Closed dawehner closed 10 years ago

dawehner commented 10 years ago
Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR TODO

Drupal would like to change the request based upon the route information directly after final matching the route.

Therefore this pull request adds POST_DYNAMIC_MATCH/POST_DYNAMIC_MATCH_REQUEST to have a good counterpart for the PRE events.

Reference the original drupal issue https://drupal.org/node/2026431

dbu commented 10 years ago

this makes kind of sense - but then again RouteEnhancers are triggered right after, so maybe not. the enhancers can do the same thing as a listener can, and there are priorities.

i added the PR template as per the contribution guide to the issue.

can you please add an entry CHANGELOG.md and do a PR on symfony-cmf-docs to document this as well?

lsmith77 commented 10 years ago

@dbu the Drupal folks do not like the idea of modifying the Request instance inside enhancers though imho it is legitimate to modify the Request inside enhancers.

dbu commented 10 years ago

@dawehner is also of the drupal folks. i changed my original positive comment above after the discussion, because @Crell pointed out that the event is about the same thing as the route enhancers.

i was going to say the Request object became irrelevant at that point anyways, but just checked and see we pass it to the enhancers as well. if we pass it in, i don't see why it could not be changed if it is legitimate at all to change the Request object.

Crell commented 10 years ago

In the broader sense, we're doing this in Drupal because we need a place to determine the one mime type that we're going to treat the request as thereafter; that must happen after matching (because the available routes will influence that decision) but before other route enahncers (because our enhancers in many cases depend on the request's mime type). There's currently nowhere to do so other than a high-priority enhancer; I don't like it, but can live with it. The Drupal 8 release manager doesn't want to live with it. Adding an extra hook doesn't seem to improve the situation, just move it around.

How does CMF or Symfony fullstack handle content negotiation? Vis, where does it call $request->setRequestFormat()?

lsmith77 commented 10 years ago

we have a listener after the routing is finished, so running after the enhancers. we are looking to switch to using the ExpressionLanguage to integrate content negotiation into the routing process for Symfony 2.5

lsmith77 commented 10 years ago

what is the status here?

dbu commented 10 years ago

i think it makes no sense, but if all of drupal is convinced its better for them and solves an issue, i could live with merging it. it seems an open discussion to me here.

@ drupalists: note that we will release the next minor version soonish (like in 2-3 weeks hopefully) so if this should be in there, we need to hear from you soon ;-)

Crell commented 10 years ago

I think we concluded in later follow-up discussion that we are going to move the _controller-defining enhancers to their own request listeners, post matching, and the conneg code as well. That makes this issue unnecessary, so closing.