drop DynamicRouter::match

[dbu]

This method is redundant to matchRequest but prone to information loss. See discussion in #105. The ChainRouter still has the match() method.

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	yes
Deprecations?	no
Tests pass?	yes
Fixed tickets	#105
License	MIT
Doc PR	-

fix #105

TODO:

• UPGRADE.md file

[dbu]

this change has actually a higher chance of biting people, if they relied on DynamicRouter implementing RouterInterface.

[dbu]

ChainedRouterInterface extends the RouterInterface. this is a problem as we want to drop the UrlMatcherInterface. we could change ChainedRouterInterface to use RequestMatcherInterface and UrlGeneratorInterface instead of RouterInterface.

[lsmith77]

there is a build failure

[dbu]

@dawehner @Crell does drupal use matchRequest or match? if you use match, do you agree it would make more sense to use matchRequest?

we are trying to get rid of some legacy things here.

[znerol]

Drupal is using the RouterListener from Symfony and thus matchRequest() is called on incoming requests. However, there are cases where calling match() on a path is useful. For example when generating breadcrumbs based on request path components (PathBasedBreadcrumbBuilder) or when checking access for user supplied paths (PathValidator).

Access-checking recently has been integrated with the router (AccessAwareRouter) in order to guard against security issues due to naive custom/contrib code. Also URL construction is currently being worked on for the same reason.

There is also an effort going on to separate access-checks which need to run on the currently incoming request (CSRF token validation) from those which are operating on (upcasted) controller arguments. The latter always need to be executed, even when generating links on a page. In such cases a fake request is currently being populated, with the only purpose to pass on the arguments to the access checks. This is indeed cumbersome and error-prone.

I just discussed this with @dawehner, and we think that we might want to determine whether to run all access-checks or only the ones not requiring the request based on whether the access-checker was invoked from matchRequest() or match(). In other words, use matchRequest() on incoming requests only, but use match() when it is necessary to render links based on (user-supplied) paths.

The same could be done with route-enhancers: If an enhancer requires the incoming request, it only should be run from within matchRequest().

[dbu]

thanks for the input. a few questions/comments:

• is drupal using the chain router, or does it use DynamicRouter directly? this PR does not touch the ChainRouter, so that would still convert a url to a request object.
• are you sure you need to match a route for breadcrumbs? i would have expected that this uses the generate method, which always operates on a route name / object and always returns a string that is the url, never a request.
• for the access control needs: did you see how symfony does this in the Security component? i think they manage to do that completely independent of the routing process - but with the RequestMatcher concept which is quite neat. if you could better decouple the access control from routing, you probably also would not need fake requests to check if links should be output or not.

it seems to me like you would need some custom component that does your internal access-checking (where you seem to use match now) in a routing-agnostic way (maybe interacting directly with the things that also access check incoming requests - but those "things" should be decoupled from the routing process).

can you explain in more detail why you would need to match on url in some situations? to me it seems that having inconsistent information for access checks is a dangerous idea. you know that you could also copy the original request and just change the url, if you really need the request to determine access to potential urls?

[znerol]

Drupal is using the ChainRouter and it is indeed unfortunate that doMatch() also instantiates a fake request. The way that path based breadcrumbs work is that path info is split into path components and then every parent path is run through the router in order to retrieve the route, arguments and the page title and also run the access checks.

I think that just making up a fake request and running it through an infrastructure which expects to act on a real request is even more dangerous. What we are after is to clearly separate the two cases (handle a request on input vs. checking a path when generating output).

Checking a path is actually quite similar to handling the request: It is necessary to retrieve the route, to upcast the arguments in order to check access and finally generate the link. However, in that case we do not want to run the csrf-access checks for example. Keeping the code paths of matchRequest() and match() would help us a lot.

[dbu]

Hm, why not using a request attribute that says this request is a fake request to trigger resolution? Then you can make things you do not want to execute check for that attribute. Would sound better to me than this very implicit relying on one codepath vs the other.

[dbu]

So for breadcrumb you are using fragments of the Route to identify the hierarchy? What if that hierarchy is not reflected in the url? And could you trigger that parsing directly, by having the nestedmatcher instead of going through the router? Its not like you actually have to select from potential routes when you just need to split a route on tokens.

[znerol]

So for breadcrumb you are using fragments of the Route to identify the hierarchy?

No, even worse: The breadcrumb builder relies on the path (i.e. it traverses all the prefixes of e.g. admin/structure/block/manage/bartik_search/translate/de/edit and calls match() on every single item. This results in the breadcrumb Administration » Structure » Block layout » Configure block » Translate. But in fact it would be probably less cumbersome to just adhere to a uniform scheme for the route keys and derive the ancestors of a page by traversing route key fragments.

And could you trigger that parsing directly, by having the nestedmatcher instead of going through the router?

Yes, indeed. I thought the same on the train back home. It would also be easier to sidestep filters (currently used for content negotiation, thus for the outgoing url-matching case irrelevant).

Another point worth considering is that the Symfony UrlMatcherInterface is request-context aware, i.e. it declares a dependency on the incoming request. For our use-case it might be better to introduce a PathMatcherInterface in order to clearly differentiate it from the routing component.

[dawehner]

Hm, why not using a request attribute that says this request is a fake request to trigger resolution?

Mh, well this is actually what we do at the moment, but we try to avoid request attributes, due to their
magicness and problematic discoverability for developers. Especially things like _route_name caused a lot of confusion for many people.
Using match() vs. matchRequest() could have nicely transport that semantic difference.

Regarding breadcrumbs: In general we do have a pluggable breadcrumb implementation:

interface BreadcrumbBuilderInterface {
    public function applies(RouteMatchInterface $route_match);
    public function build(RouteMatchInterface $route_match);
}

with route match pretty much being a value object of the request attributes.
This allows people to swap out the breadcrumb but there is one implementation which is path based,
but for sure we do also have one which is based on taxonomy terms hierarchy only.

it seems to me like you would need some custom component that does your internal access-checking (where you seem to use match now) in a routing-agnostic way (maybe interacting directly with the things that also access check incoming requests - but those "things" should be decoupled from the routing process).

Well, there are so called access checkers which require the request
This is exactly what we do already in our access component:

interface AccessManagerInterface {
    public function checkNamedRoute($route_name, array $parameters = array(), AccountInterface $account, Request $route_request = NULL);
    public function check(Route $route, Request $request, AccountInterface $account);
}

as you see we already have splitted up request specific ones and not request specific.

[dbu]

using match vs matchRequest as semantics of internal check for partial resolution vs incoming request is really abusing things, its not the semantics of the router at all. both are supposed to find the route for the request, but originally, only the url string was used instead of the full request object, hence the two methods.

if going through the router is really what makes most sense in drupal for your case, i would recommend that you extend the router and add a validatePath function that does the behaviour you need. from the explanations i think you would not even need the route provider. the only thing you are reusing is the route enhancers (or some of them) and the regular expression from the compiled route to split your path. applyRouteEnhancers being a protected method, you can still call that from your custom method if you need it.

i don't want to complicate your live, and i will wait merging until we solved this discussion, don't want to break your code. but i don't want to keep bad code around to support a hacky solution, so i am challenging your design :-P if online does not work out, i expect to see @Crell next week in madison for madisonphp. larry, we could sit together friday afternoon if you are already in madison then, wdyt?

[Crell]

I'll be getting into Madison Friday evening. If you don't mine a late dinner we can talk then. I'll need to have @znerol and @dawehner bring me up to speed on the discussion here since I only just saw this thread now. 😄

[znerol]

Regrettably there currently is code in Drupal calling the router with other input than the incoming request. However, as pointed out by @dbu re-purposing match() wouldn't be any better, and therefore I'm okay with removing this from cmf.

[lsmith77]

so do I understand it properly, that we can likely expect a final version of this PR over the weekend?

[dbu]

from what i understood, drupal should be ok with us removing match. the remaining question is whether i really should change the ChainedRouterInterface to no longer implement RouterInterface. this would mean that DynamicRouter really does no longer implement RouterInterface. technically this is all correct, but i have seen it often that people where expecting RouterInterface instead of UrlGeneratorInterface in their code. those people will get bitten - unless they use the ChainRouter which stays as it currently is.

[dbu]

not sure if anybody is using this interface. but it was too restrictive. can we do this? otherwise we would need to keep match and throw an exception on dynamic router.

should this go into changelog as well?

[lsmith77]

the ChainRouterInterface was added in one of the RCs at the request of @dawehner (aka Drupal), so I assume we can easily change it now ..

[dbu]

added another commit. this is creating quite some BC breaks in the details i fear. should we go on? or simply deprecate the match method, or start throwing exceptions in match that its not meant to be used, and keep the bc breaks for 2.0?

[lsmith77]

puh .. that is a very tricky if statement ..
seems like there should be some additional parenthesis in there.

also in the phpdoc for ChainRouterInterface you specify RouterInterface|RequestMatcherInterface which would be quite a lot simpler to express than the above ..

also shouldn't we say what a valid router is?

[dbu]

you mean paranthesis around the inner &&? i can do that if you think it helps.
where would you say what a valid router is? in the interface phpdoc? or in the exception message?

[lsmith77]

yeah, I think we should just deprecate for now. that being said, is there a reason not to just go to 2.0?

[lsmith77]

ping

[dbu]

ping

agreed to only deprecate for now. will do this week.

[dbu]

so ftr, we decided to avoid the BC break in the 1.x branch to respect semver and avoid unexpected side effects (like people typehinting RouterInterface getting errors). see #120 for the PR that only deprecates DynamicRouter::match but still cleans up the chain router.

i will update this PR once #120 is merged, to be the start for the 2.0 adaption.

[lsmith77]

needs a rebase

[dawehner]

Mh, so one potential usecase we have for ::match() is to implement caching of path only based routing.
For example for menu links you pass along a path, entered by the user, which then has to be converted to a routing result.

A generic routing is hard to cache, given that you have the full request object as context, but for path only based routing, only the path matters, and you should be able to cache the result.

[dbu]

@dawehner if you do request caching, i would strongly recommend to look at the symfony HttpCache that is based on the full request, rather than trying to build your own thing. there is no guarantee that only the path matters.

for this topic in general, we should see what symfony 3 brings in terms of routing, whether they sorted out UrlMatcher against RequestMatcher and be sure that if we have to do any BC breaks, we align to handle symfony 3 without further BC break.

[dawehner]

@dbu
No, we want to cache the routing and just the routing, as this is the first step before we do access checking. This is really not about caching of a full thing but really just about a decorator for specific calls to the routing system

[dbu]

ah i see. to me this really is a symfony 3 topic in the end, regarding what they want to do with the UrlMatcher vs RequestMatcher interface. was that already discussed somewhere? can you start the discussion on symfony/symfony if its not discussed yet?

[lsmith77]

@fabpot do you have any plans here?

[dbu]

@Tobion do you have an idea what symfony 3.0 thinks about router match vs matchRequest? is symfony going to drop one of them or keep them both?

[Tobion]

I guess if we start transitioning to PSR-7 in symfony 3.0 we would also more likely (only) support matching against RequestInterface from PSR-7. For everything else, I'm not sure it's worth to break BC so hard just to remove one of the existing.

[dbu]

@Tobion well, to me its very confusing that the RouterInterface mandates match and then there is a RequestMatcherInterface with a different signature. in the end we do in this bundle re-create a Request when match is called so that we have one consistent code. fabien is even afraid this could lead to security issues: #105

[wouterj]

@dbu can you please rebase and finish this PR?

[dbu]

symfony did not make up their mind about the future of match(). i suggest we leave this for version 2 and see what happens until we do version 3. there is PSR-7 and another psr about server requests, maybe that will replace the symfony request anyways at some point.

[ElectricMaxxx]

pls reopen if still interessted.