[WiP] NestedMatcher support

[Crell]

This in-progress branch represents a new model for the Matcher, called NestedMatcher. It was developed by the Drupal project, but deliberately with the intent of it coming back upstream. All of the code in this PR is (c) Larry Garfield and not Drupal-specific, so it's all license-safe for MIT licensing.

The basic idea is that rather than a matcher that finds a single route, we allow a matcher to find multiple possible routes. Then we allow N separate objects to filter that list until it either becomes empty (and then throw a 404 or 405 or 415 or whatever the appropriate error is) or we end up with just a single route left and that's the found route. Win!

This code is not commit ready; at the moment it's still using Drupal code formatting standards. Sorry. ;-) But I want to open the discussion of merging this model back into CMF cleanly, so that Drupal can just drop our version and use the CMF Routing library directly.

This definitely needs some revision yet, and needs to get integrated properly rather than just be a bunch of extra classes.

For background, there's some good background discussion here: http://drupal.org/node/1784040

Discuss. :-)

[lsmith77]

@Tobion can you have a look?

[Tobion]

Yes I'll look at it.

[dbu]

i wonder if it is not over-engineering to have an interface for this. i don't see why people would want to implement their own nested matcher that is different from the default one, and if they really need to why they do not just do it. do we use the interface anywhere else than in the NestedMatcher extends ... line?

[Crell]

I included an interface more out of habit than anything else. I always default to having an interface. In this case, though, you're right that it may not be necessary, since the important parts of the interface are already defined by RequestMatcherInterface. I'm fine with removing it.

[dbu]

so the DynamicRouter (or actually any router) can use the NestedMatcher if needed. makes sense. note that the Symfony\Component\Routing\Router currently does not allow to inject a matcher service but only has a 'matcher_class' => 'Symfony\Component\Routing\Matcher\UrlMatcher', option. to use a matcher service configured to our liking, you would need to extend that class. or we could refactor the core router to optionally accept an injected matcher.

i like the concept. this is yet missing a NestedMatcher for the normal route mapping as discussed in symfony/symfony#5014 - my PR symfony/symfony#5351 was not approved, we should find a new solution for this.

lets fix the code style once its clear we agree on the exact functionality and all.

[Crell]

@dbu: I'm fine with fixing Router upstream if we can convince Fabien. Specifying classes statically like that causes more trouble than it's worth, IMO. I'm also fine with just having a generic RequestRouter object in the CMF routing lib that takes objects and works off of Request, not RequestContext. That would be nicely reusable for any type of Matcher or Generator, and would be totally easy to wire up via the DIC.

[Tobion]

wouldn't the correct name be matchRequestPartially?!

[Crell]

I suppose that depends on your naming convention. I went with matchRequest-plus-suffix, but if Symfony convention is otherwise I'm fine with renaming it. Note that we likely will want to rename it in light of symfony/symfony#5895 if that goes in, which I hope it does.

[Tobion]

I don't think it has to do with naming convention, but more with correct English. partially refers to match so must be an adverb, not an adjective.
What do you want to rename it to? I don't see what my PR has to do with this method.

[Crell]

That PR renames matchRequest() back to just match(). Presumably then this method would get renamed as well.

[Tobion]

To sum up: @dbu 's idea is to execute the symfony matcher as a partial matcher within the nestedmatcher. That's because otherwise the NestedMatcher would need to duplicate much logic as individual filters like http method (implemented in this PR) and the scheme requirement (not yet implemented here). This is the reasoning behind his PR symfony/symfony#5351.
I agree that the duplication is not good. But I also think that it's part of the idea to make it possible to configure the filters any way you want. Like removing the method requirement (filter). If all standard logic is executed anyway, you cannot configure it flexibly anymore. But to make that possible one would really need to reimplement all rules, which I think goes overboard. @Crell: Do you have some specific use-cases for partial matcher filters that are useful but are not handled by smyfony? I'm afraid the nested matcher adds flexibility that is not needed for the cost of reimplementing most of symfony's matcher.

[dbu]

well we /could/ provide one PartialMatcher based on the core matcher that does it all in one go, but also the separate matchers and then the user could choose what to use. so we could offer efficiency and flexibility.

but i agree with @Tobion we should understand the use case a bit more.

but if there are valid use cases, i think we can have it in this component but the DynamicRouter could by default still use the core matcher if the cmf does not need those more complex use cases.

[lsmith77]

@Crell ping ..?

[Crell]

Thinking aloud...

My concern with wrapping Symfony\Component\Routing\Matcher\UrlMatcher (which is I assume what's being referred to here) into a PartialMatcher is that it by design would be a FinalMatcher only (since it's returning not a collection but an array for attributes). But then you're likely repeating work you've already done in discrete partial matchers.

Unless you only use Partial Matchers for things that are not already covered by UrlMatcher, which means in most use cases HttpMethodManager goes away. You'd still use InitialMatcher to get your source based on path, but then UrlMatcher is repeating only the path logic. Which means in the typical case, you have no partial matchers!

The downside there is that any additional system-specific matchers you add would have to happen before method matching (and format matching if that gets into that class), which mean you're potentially doing the expensive ones first instead of after all of the cheap stuff has run. (Think routing based on the type of object in a parameter.)

[Tobion]

Which means in the typical case, you have no partial matchers!

@Crell: That was basically my question. What are your use-cases for offering partial matchers that can be tied together any way you want? Because you usually need all anyway, which is what Symfony\Component\Routing\Matcher\UrlMatcher does. So why do you need the flexiblity? And which partial matchers do you want to add that the Symfony UrlMatcher does not handle? If I understand your desires, I would see why you want to reimplement all of the logic (like the method and scheme requirement, and also the hostname requirement that landed in symfony master -> all of them would need a reimplementation as partial matcher, isn'it?).

[Crell]

The initial one was format-based matching, which Symfony doesn't have yet and is still in development. We're working on a temporary implementation that is not a full-on negotiation implementation but at least handles basic HTML vs. JSON type requests. A second consideration is size. UrlMatcher requires a RouteCollection to match against. Drupal has 1000+ routes (yes, seriously), which makes doing it all in memory infeasible. We have to match first in the database on path, and then filter from there.

Third, although we've not settled on it we want to leave open the opportunity for matching on non-request attributes, like the type of a node (Drupal entity data object). There are other lines of work working on that now, and I'm not certain where they will land, but the ability to do Drupal-specific routing is something we wanted to leave open since those use cases have existed for some time in Drupal, with various ugly hacks. We want to eliminate those hacks.

[dbu]

i tend to trust the judgement of crell that there are cases where you need something like this. we have to think how to make people understand they usually don't need to get into this unless they really do have a special use case.

what i still don't like is that we would end up duplicating all the matching logic the core already has. if we could manage to refactor the core matcher in a way we could use it to filter routes instead of returning the first match, that would feel so much better. or should we just wrap it and call it repeatedly with the remaining collection each time we have a match?

or crell are you saying we really need to split the core matcher into smaller chunks and people actually need to insert their custom stuff in the middle of those steps? guess not, adding to the end or maybe the beginning should be enough, no?

[Crell]

Well, here's the thing. If we say that UrlMatcher can be a FinalMatcher, which IMO makes sense, then that can handle most cases. Method, domain, scheme, soon format I hope, can all be handled there. Conceptually I like the idea of being able to leave out method matching entirely, for instance, but as a practical matter I understand that's potentially excessively idealistic.

So if hypothetically we used UrlMatcher as FinalMatcher, and then reserved PartialMatchers for "fancy" options (like object type, day of week, or other wacky runtime considerations), I see three issues, which may be resolvable:

1. Drupal can't provide UrlMatcher with a RouteCollection, because it's just too big. We must keep the separate DB-backed path matcher, even if we then pass that to UrlMatcher.

2. If we do a first-pass externally and then pass in the reduced collection, the path part of UrlMatcher is still going to run, and we still have to deal with the fact that Drupal doesn't use pathInfo as the path to match against but a processed route based on it. If there were some way to disable the path-matching part of UrlMatcher, it would make the class a misnomer but would make that problem go away. (Perhaps by making everything except path matching into a separate object that UrlMatcher takes as a dependency, which then we can take as well?)

3. Both CMF and Drupal want to return the matched route object as _route, whereas UrlMatcher wants to return the route name.

So, I think if we could break up UrlMatcher into 3 parts:

1. Path matching
2. All other direct-HTTP matching (method, format, scheme, domain, etc.)
3. Deriving the attribute data from the finally selected route

Then Drupal, CMF, and Symfony full stack could all share #2, Drupal and CMF could share #3, and Drupal and CMF could go their own ways for #1. At that point, NestedMatcher becomes very simple in the typical case, with a path initial matcher, and some combination of 2 and 3 for the final matcher. The Partial Matchers are then there just for esoteria and unusual cases, but the typical case wouldn't even use them. And DynamicRouter could easily leverage NestedMatcher to replace its current hard-coded-two-step process with a multi-step process with little extra overhead, but the opportunity for people to inject other filters between those two steps if they felt like it.

Think we can sell Fabien on that? :-)

[dbu]

thanks crell

1. this should really be handled by using the DynamicRouter and RouteRepositoryInterface. the repository is queried at runtime to provide a list of viable route options. there is a PR open to make this based on request rather than url. this is somehow the same as what you call initial matcher (i think RouteSource or RouteProvider would be a better name than both RouteRepository or InitialMatcher). this would be something only used by drupal and the cmf, not by core symfony.

2. totally agree here.

3. yes, though this is no big issue, the DynamicRouter solves this easily.

so yes, the question really is how we can refactor what is called UrlMatcher to provide various matching functions that can be used as filters too. fabien was saying he is ready to refactor as long as there is no big performance loss and there is this issue symfony/symfony#5014 but no news on it. @Tobion do you have ideas how to solve this? maybe if we have another concrete proposal for splitting UrlMatcher better, we could see some advancement?

[lsmith77]

/cc @lolautruche @fabpot

[Tobion]

Thanks @Crell for this explanation. It's a good summary of the problems we face.

In my opinion it would be best if the Symfony matcher exposed some clean extension points for each requirement (e.g. path, scheme, method) within the matching process. So a subclass could apply specific logic in there. For example when matching the path, a custom implementation could query the database and could also use $request->attributes('_drupal_path') instead of $request->getPathInfo().

The problem is, that such filters can not be applied simutaneously but only one after the other. This is the difference to the current core matcher. This is by design slower as the (filtered!) collection would need to be traversed multiple times instead of one. It also depends on the sequence of filters (path -> method -> domain or scheme -> domain -> path ...).

[Crell]

I wonder if UrlMatcher could be rewritten as a PartialMatcher. Then we have InitialMatcher/RouteRepository/RouteSource/Whatever it's called, which takes a path/request and returns a collection. We have UrlMatcherTNG which does 90% of what it does now, minus path matching (maybe) and generating the attributes. And then we have a FinalMatcher that does the attribute generation off of the one remaining route.

Main Symfony behavior: RouteSource is a no-op, or nearly so. FinalMatcher is just the last few lines from UrlMatcher. UrlMatcher is mostly what it is today. But you can still plug in another partial matcher if you want.

Drupal: we provide our own RouteSource, and tweak the FinalMatcher to return a route object, not route name.

CMF: It has its own RouteSource, and the same FinalMatcher as Drupal.

Typical case, the only partial matcher anyone uses is UrlMatcherTNG. But you can slip in others if you want.

Total added cost for Symfony fullstack: Somewhere less than 10 stack calls, probably. My only concern is the regex inside UrlMatcher, which I think is doing both matching and attribute extraction at the same time. Maybe that's OK, though.

[dbu]

this sounds good to me. for the meantime, shall we get this PR through with a FinalMatcher implementation that uses the current UrlMatcher and merge RouteRepositoryInterface with InitialMatcherInterface? that way i could change the DynamicRouter to use that and we would be all ready for improvements.

i would really love to change the name of that thing to RouteProviderInterface and call the method something like getRouteCollectionForRequest. the other difference is that we have a getRouteByName - which is needed for the generator. can we have that too or do you think we need 2 interfaces here so that a pure matcher route provider does not need to be able to fetch a route by name?