"Dangerous" code in DynamicRouter?

[fabpot]

While diving into the Drupal code, I stumbled upon this line https://github.com/symfony-cmf/Routing/blob/master/DynamicRouter.php#L205 where a Request is created with only the path info. That looks like a bad idea as everything except the path info falls back to default values.

[dbu]

hi fabien,

thanks for the feedback! we only use that request object in the call to applyRouteEnhancers and users are supposed to use matchRequest if their application has enhancers that depend on more than the url. in the symfony context, the entry point will always be matchRequest as symfony discovers the RequestMatcher interface on ChainRouter. we wanted to avoid an additional infrastructure for enhancers that can work without a request object. the enhancers add information to the match, e.g. "upcasting" a drupal node id to a Node object or in the cmf context determining the controller based on the content object class. they usually do not use the request, but we wanted to give access to it in case there is need for it.

we have similar code in the ChainRouter as well. the idea here is to support routers that implement the RequestMatcherInterface even when the ChainRouter is called with match instead of matchRequest. at that point, no other information is available.

what would you suggest instead? we could

• provide two variants of the ChainRouter for UrlMatcher and RequestMatcher,
• throw some exception when match is called but we have a RequestMatcher registered
• silently ignore the router in that case. but an application will usually only use one of match and matchRequest so this option is most likely not helpful.

[lsmith77]

so what do we want to do here?

[dbu]

@fabpot do we need to do anything here? i somehow wonder if we just should drop all codepaths that are not based on the Request in the DynamicRouter. it would simplify quite some places and avoid creating Request objects in random places.

for ChainRouter we probably want to keep them, but its a lot less fiddly there.

[fabpot]

@dbu I agree with you (dropping all codepaths not based on the request).

[lsmith77]

@dbu: can you do a PR for this so we can put it into 1.3.0?

[lsmith77]

@dbu / @fabpot so basically remove DynamicRouter::match()?

[dbu]

In full stack symfony this will have no effect as the matchRequest is preferred. ChainRouter can continue to create a request when needed, i think. So it would only affect users using DynamicRouter standalone, they would have to create the Request to call DynamicRouter with it.

----- Reply message ----- Von: "Lukas Kahwe Smith" notifications@github.com An: "symfony-cmf/Routing" Routing@noreply.github.com Cc: "David Buchmann" david@liip.ch Betreff: [Routing] "Dangerous" code in DynamicRouter? (#105) Datum: Fr., Sep. 5, 2014 05:19 @dbu / @fabpot so basically remove DynamicRouter::match()?

— Reply to this email directly or view it on GitHub.