Problem/Motivation

The NestedMatcher architecture was deliberately intended as a step along a road, but not the final implementation. It has some known issues (e.g. #1784040: NestedMatcher may be too slow and non-deterministic), and a lot of moving parts. It also doesn't inherit much functionality from Symfony, just the interface models. But that's OK, because that's all we wanted it to be.

Proposed resolution

In collaboration with David Buckmann (dbu) of Symfony CMF, we have overhauled the Symfony CMF Routing component to build it around the NestedMatcher design. That has been greatly refined, however. Most of the heavy lifting is now done by the CMF Routing component, and we're just filling in a few pieces.

This patch adds the CMF Routing component via Composer, and then leverages it. In short, the new design is:

ChainRouter: Chains together multiple routers (a matcher and a generator combined).
NestedMatcher: Evolved version of the old NestedMatcher
RouteProvider: More generalized version of InitialMatcher, now can be used by the Generator, too.
RouteFilter: Formerly Partial Matcher. Same basic idea.
FinalMatcher: same as before.
Route Enhancers: A new thing from CMF, lets us process the attributes array before it gets added to the request attributes Bag. I'm not using it yet, but this seems like it would be an even better place to put parameter upcasting than what we're working on in #1798214: Upcast request arguments/attributes to full objects.
Generator: Right now we're just using the vanilla RouteProvider-backed Generator from CMF, which actually works fine.

Also, CMF (and therefore we) are using a thin wrapper around Symfony's UrlMatcher as the final matcher. What that means is most of our partial matchers go away, because they just duplicate what is already in that class. In fact, UrlMatcher already did more than we were, which means this patch immediately gets us:

- Matching by scheme (http vs. https)
- Matching by domain

Neither of which we had before. It also eliminates the need for the separate partial matcher for HTTP method. We still have a route filter for mime type, but if mime type-based matching ever integrates into UrlMatcher (as various people are, slowly, working on), then we can drop ours and inherit the Symfony one with no extra effort.

I also ended up merging our route compiler with Symfony's route compiler. It's a big ugly, but that's because Symfony's route compiler is not designed to be extended. That could be made cleaner if someone wants to tidy up Symfony's route compiler, but it's not a priority at the moment.

The patch looks large, but that's mostly because of removing lots of files and adding the CMF component. The net impact is about half as much Drupal-specific code in /core, which is a good thing.

Remaining tasks

- I need to add some caching and integrate with the path alias system, although this is stable as is.

- I don't know if we want to go all the way to integrating url and l into the generator now, or save for another patch. Input from committers welcome.

- New version of #1798214: Upcast request arguments/attributes to full objects that uses Route Enhancers, as a separate patch.

User interface changes

None. This is mostly just code reorganization.

API changes

Very minor. The only one of note for most people is that there are no more "partial matchers". They're now called Route Filters, and we need fewer of them.

Files: 
CommentFileSizeAuthor
#31 1874500-routing-take-2.patch92.96 KBCrell
PASSED: [[SimpleTest]]: [MySQL] 50,607 pass(es).
[ View ]
#25 1874500-routing-take-2.patch86.39 KBCrell
PASSED: [[SimpleTest]]: [MySQL] 49,684 pass(es).
[ View ]
#21 1874500-routing-take-2.patch85.4 KBCrell
PASSED: [[SimpleTest]]: [MySQL] 50,536 pass(es).
[ View ]
#19 1874500-routing-take-2.patch85.27 KBCrell
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1874500-routing-take-2_4.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#14 1874500-routing-take-2.patch85.75 KBCrell
PASSED: [[SimpleTest]]: [MySQL] 50,506 pass(es).
[ View ]
#7 1874500-routing-take-2.patch224.04 KBCrell
FAILED: [[SimpleTest]]: [MySQL] 49,967 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#5 1874500-routing-take-2.patch223.27 KBCrell
FAILED: [[SimpleTest]]: [MySQL] 49,517 pass(es), 166 fail(s), and 36 exception(s).
[ View ]
#2 1874500-routing-take-2.patch223.26 KBCrell
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1874500-routing-take-2_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#1 1874500-routing-take-2.patch83.83 KBCrell
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1874500-routing-take-2.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new83.83 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1874500-routing-take-2.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

And patch. I needed a nid. :-) There's also a nice clean branch in the WSCCI sandbox named 1874500-routing-take-2 for those who would rather review that. I think we should merge that, too, rather than just committing the patch.

StatusFileSize
new223.26 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1874500-routing-take-2_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Stupid git and it's not committing the CMF component when I tell it to-ness...

I split the CMF component off to a separate issue, after which rerolling here will be much easier: #1874530: Add Symfony CMF Routing component

Status:Needs review» Needs work

The last submitted patch, 1874500-routing-take-2.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new223.27 KB
FAILED: [[SimpleTest]]: [MySQL] 49,517 pass(es), 166 fail(s), and 36 exception(s).
[ View ]

Oh come on, git...

Status:Needs review» Needs work

The last submitted patch, 1874500-routing-take-2.patch, failed testing.

StatusFileSize
new224.04 KB
FAILED: [[SimpleTest]]: [MySQL] 49,967 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

OK, this should bring the fail count down some...

Status:Needs work» Needs review

Bah.

Status:Needs review» Needs work

The last submitted patch, 1874500-routing-take-2.patch, failed testing.

The test fail in REST module occurs because the wrong HTTP status code is returned. 405 means Method not Allowed, but the HTTP request method GET is allowed in this case. 415 Unsupported Media Type would be correct in this case, because an invalid format (application/wrongformat) is sent in the test.

So there seems to be a bug in the routing system?

When I look at what the code is doing, it's filtering exactly right. There are several legal routes that have no format requirement specified, so they should pass the mime type filter. My guess is that because we're now letting UrlMatcher handle method filtering we've reversed the order. That is, we used to have method filtering first, then mime filtering. Now we have mime filtering first, then method filtering. That means whichever filter happens second is going to "win" and throw its exception.

Honestly that sounds to me like we should be updating the test.

FYI, I just committed #1874530: Add Symfony CMF Routing component. Should unblock this issue, I believe.

That won't impact the one remaining bug here, but it should make the next patch much smaller and easier to review. :-) Thanks. Hopefully coming this weekend.

Assigned:Unassigned» Crell
Status:Needs work» Needs review
StatusFileSize
new85.75 KB
PASSED: [[SimpleTest]]: [MySQL] 50,506 pass(es).
[ View ]

OK, new patch! This has also been rebased and repushed to the repo. I would still favor a merge over a patch commit.

Changes:

1) The CMF Routing component is already in core so is not included here.

2) I changed the REST module tests to account for the changes in this patch, and included a comment to explain why they're still valid.

3) I added a Drupal-specific UrlGenerator subclass that handles path aliases, including tests.

4) RouteProvider now caches route lookups for the lifetime of the object.

Things not done:

A) With the RouteProvider, we could refactor some of the other tests to not even need a database at all. I probably won't deal with that here.

B) I have not yet done any l() or url() integration, as I don't know if that belongs here or in a separate patch. Given that I'm sure he has strong opinions on the subject, I will not work on that here unless catch says to.

C) I don't know if the caching here is sufficient, or if we need some deeper integration with the alias caching. Again, catch, this is your area of interest so you tell me the minimum you need done in order to commit this and I'll write that.

I think this should pass all tests, which means get to reviewing, people!

Here are a couple of things I noticed on a first pass:

+++ b/core/lib/Drupal/Core/Routing/CompiledRoute.phpundefined
@@ -60,15 +59,30 @@ class CompiledRoute {
+  public function __construct(Route $route, $fit, $pattern_outline, $num_parts, $staticPrefix, $regex, array $tokens, array $pathVariables, $hostnameRegex = null, array $hostnameTokens = array(), array $hostnameVariables = array(), array $variables = array()) {
+    parent::__construct($staticPrefix, $regex, $tokens, $pathVariables);

The last 4 constructor params don't get passed to the parent constructor.

+++ b/core/lib/Drupal/Core/Routing/RouteCompiler.phpundefined
@@ -26,8 +27,31 @@ class RouteCompiler implements RouteCompilerInterface {
+   * Compiler for the Symfony route compilation data.
+   *
+   * @var \Symfony\Component\Routing\RouteCompiler
+   */
+  protected $symfonyCompiler;
+
+  /**
+   * Constructs a new RouteCompiler.
+   *
+   * Normally putting a dependent object in here like this would be a no-no,
+   * but Symfony's route compiler insists on class injection rather than object
+   * injection so we are given no choice.
+   */
+  public function __construct() {
+    //$this->symfonyCompiler = new SymfonyRouteCompiler();
+  }

Looks like this is leftover cruft..?

C) I don't know if the caching here is sufficient, or if we need some deeper integration with the alias caching. Again, catch, this is your area of interest so you tell me the minimum you need done in order to commit this and I'll write that.

The way to find out would be to set up a realistic test case then profile it, rather than asking me...

On l() and url() integration, I don't see any reason to add integration here. This won't be generically useful url() is using it (or partially replaced by it) but that's also going to be tricky to get right.

Also why is the generator being added here and not in the generator issue?

url()/l() integration/replacement is necessary before it becomes generically useful, if the plan is to do that before code freeze then it needs a dedicated issue to discuss it (could be a follow-up) but there's not much point adding a full featured generator unless we plan to do that.

For ESI, we only need to subclass and override one method per #1830854: [meta] The ESI pipeline battle plan and I'd much rather do that than add a complicated system with extra overhead just to get the route for one path which is going to be the same 100% of the time.

Re url(): I did a little further poking last night, and bringing in url() is highly non-trivial. It references 3 globals and I believe 3 additional utility functions, which themselves reference things. That's going to be ugly to try and merge, so let's not do that here. Follow-up.

Re the generator: Because adding an actually functional generator without the rest of the stuff in this patch is not feasible. And adding this stuff without a generator is not possible, since it's all built up together. I did pull in a bit of code from the generator issue. We should probably just close that in favor of this one.

As I have said repeatedly, I firmly believe we should be using the generator more than url() when the dust settles, even if it's only strictly required for ESI, so getting it in place now is a good thing. That also will allow us to do #1798296: Integrate CSRF link token directly into routing system, because we can just flag a route with an _csrf_token = TRUE requirement and then the generator does its thing.

Fabpot is also in the middle of gutting HttpKernel, so the one function you're thinking of won't exist very soon: https://github.com/symfony/symfony/pull/6459

StatusFileSize
new85.27 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1874500-routing-take-2_4.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

New patch, takes care of comments in #15. No other changes.

Status:Needs review» Needs work

The last submitted patch, 1874500-routing-take-2.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new85.4 KB
PASSED: [[SimpleTest]]: [MySQL] 50,536 pass(es).
[ View ]

Damn that fast moving Drop...

I like this. It's nice and tidy. Feels like Crell is getting rid of half of core. :)

Here's only a few minor findings:

+/**
+ * @file
+ * Definition of Drupal\system\Tests\Routing\RouteProviderTest.
+ * Contains of Drupal\Core\Routing\UrlGenerator.
+ * Contains of Drupal\system\Tests\Routing\MockAliasManager.
+ * Contains of Drupal\system\Tests\Routing\MockRouteProvider.
+ * Contains of Drupal\system\Tests\Routing\UrlGeneratorTest.

Once oldschool and four times copy'n waste.

+++ b/core/lib/Drupal/Core/Routing/UrlMatcher.php
+  public function __construct() {}

Empty constuctor?

The empty constructor is necessary to override the parent constructor. I think there's a comment to that effect... If not I'll add one. I'll fix the docblocks later tonight.

StatusFileSize
new86.39 KB
PASSED: [[SimpleTest]]: [MySQL] 49,684 pass(es).
[ View ]

Minor reroll covering the docs comments from #23. I also moved the routing bits of CoreBundle to a utility method to make them a bit tidier to keep track of, after discussing with Kat that it was probably a good thing to do in general.

I also spent some time looking into #1845402: Update menu link access checks for new router to see if that made sense to fold in here. However, as chx reminded me menu links are about to become entities per #916388: Convert menu links into entities, so anything we do here would collide with that rather badly. I therefore recommend we not touch that yet until this patch and that one are both in, and then we can see where the dust has settled.

I therefore consider this patch complete for the time being and commitable, unless someone else has pushback (or testbot decides it hates me).

Just had a very quick of the patch and I think there is one big problem: the router service is in the request scope (that's because is depends on router.dynamic which depends on router.request_context which needs the request service when it is created). It means that a new router will need to be created each time you handle a new request. Why not just let the Symfony Router listener (or your own) update a "global" (in the DIC sense of course) request context object whenever a new Request is handled?

With the generator, if we're going to add a 'real' implementation that's supposed to work, then it'll either need to take into account hook_url_inbound_alter() (which I know Crell won't like), or we need to remove hook_url_outbound_alter() (which I'm sure he will).

With the current status of this patch it looks like url() and the generator will return completely different things if a module implements that hook. It ought to be possible to replace the path alias manager service to mimic the behaviour of that hook as it stands. And if we do end up actually using the generator for anything by release then it's providing much the same functionality anyway.

catch: The next step (which at this point I'd rather do in a separate patch to keep things simple) is to merge url() into our generator. That includes merging the caching of path lookups, generator included, and unifying hook_url_outbound_alter() into an event that we can cache *after* lookup, not before. That in turn means that path aliases become just another hook_url_outbound_alter()-equivalent listener, which is fine because we're caching it. That would also parallel the inbound-alter logic, which has already been converted to the request listener.

That does lose us the ability to do uncached runtime outbound url manipulation, but I think that's a fairly niche case and an acceptable trade off to a unified outbound URL cache.

Because that involves dissecting url(), however, which is going to be a task and a half itself, as well as refactoring the path and path cache classes, I'd rather push that to a follow-up so that we can unblock upcasting and such. I've opened that up here: #1888424: Make Drupal's URL generation logic available to HttpKernel, and minimize code repetition/divergence

Until that happens, url() and generate() would only give differing results if you have hook_url_outbound_alter() active, which I don't think core even uses. I think a known follow-up is fine.

fabpot: Well, I was trying to avoid changing the router listener, since we just got rid of a custom one recently. :-) Can you clarify what you're suggesting? I'm not entirely sure how to do what you're talking about. (Code snippet would be appreciated.)

All right, I tried to address fabpot's point from #26, but ran into a delightfully thorny issue with Symfony's container scoping that I've documented here: https://github.com/symfony/symfony/issues/6756

In light of that, I recommend we leave it be for the time being as the way it's coded now has no leakage, it's just regenerating more objects than we'd like. If any of the solutions suggested in that issue come to pass, we can adopt them at that time without any change to the developer API, just changes to the internal wiring of the router to the container, so my recommendation is to move ahead with this issue as is and then adjust later once this question is figured out upstream.

Status:Needs review» Needs work

The patch seems like a fairly straightforward cleanup to better leverage the CMF component. I'm +1 for committing it fairly quickly to keep momentum going on issues either hard or soft blocked on this one. Seems like catch's 2 main concerns are:

there's not much point adding a full featured generator unless we plan to do [url()/l() integration]

This patch doesn't really add a full-featured generator though. It just provides a NULL implementation and a minimal Drupal UrlGenerator implementation to comply with needed interfaces. The Drupal one is extending the Symfony CMF one, which maybe is full-featured, but I don't see why that should be held against this patch.

With the current status of this patch it looks like url() and the generator will return completely different things if a module implements [hook_url_outbound_alter()]

I'm satisfied with #28's answer to that.

Unless catch or others have additional concerns or aren't satisfied with the resolutions/punting of these ones, I'm tempted to RTBC, except there's some missing docs, so "needs work" on that:

+++ b/core/lib/Drupal/Core/Routing/MimeTypeMatcher.php
@@ -10,46 +10,41 @@
   /**
-   * Matches a request against multiple routes.
-   *
-   * @param \Symfony\Component\HttpFoundation\Request $request
-   *   A Request object against which to match.
-   *
-   * @return \Symfony\Component\Routing\RouteCollection
-   *   A RouteCollection of matched routes.
+   * {@inheritDoc}
    */
-  public function matchRequestPartial(Request $request) {
-
+  public function filter(RouteCollection $collection, Request $request) {

Is {@inheritDoc} allowed by Drupal coding standards? Elsewhere, we do something more like Implements \Symfony\Cmf\Component\Routing\NestedMatcher\RouteFilterInterface::filter().

+++ b/core/lib/Drupal/Core/Routing/NullGenerator.php
@@ -0,0 +1,27 @@
+  public function generate($name, $parameters = array(), $absolute = FALSE) {
+    throw new RouteNotFoundException();
+  }
+
+  public function setContext(RequestContext $context) {
+  }
+
+  public function getContext() {
+  }

These all need PHPDoc.

+++ b/core/lib/Drupal/Core/Routing/UrlGenerator.php
@@ -0,0 +1,43 @@
+/**
+ * Description of UrlGenerator
+ */
+class UrlGenerator extends ProviderBasedGenerator {

Better docs please.

+++ b/core/lib/Drupal/Core/Routing/UrlGenerator.php
@@ -0,0 +1,43 @@
+  public function __construct(RouteProviderInterface $provider, AliasManagerInterface $alias_manager, LoggerInterface $logger = NULL) {
+    parent::__construct($provider, $logger);
+
+    $this->aliasManager = $alias_manager;
+  }
+
+  public function generate($name, $parameters = array(), $absolute = FALSE) {
+    $path = parent::generate($name, $parameters, $absolute);
+
+    // This method is expected to return a path with a leading /, whereas
+    // the alias manager has no leading /.
+    $path = '/' . $this->aliasManager->getPathAlias(trim($path, '/'));
+
+    return $path;
+  }

Need PHPDoc.

+++ b/core/lib/Drupal/Core/Routing/UrlMatcher.php
@@ -0,0 +1,36 @@
+  public function finalMatch(RouteCollection $collection, Request $request) {

Needs PHPDoc.

Also, a bunch of class methods used by tests are missing docs, but I don't know how strict we are with those.

Status:Needs work» Needs review
StatusFileSize
new92.96 KB
PASSED: [[SimpleTest]]: [MySQL] 50,607 pass(es).
[ View ]

I think requiring full docblocks on mock testing classes is excessively pedantic and anal. I have therefore added them.

Attached patch is just docs fixes and removing a few unused "use" statements I found in the process. No functional change.

Committers: There is a freshly rebased and clean branch in the WSCCI sandbox called 1874500-routing-take-2. Please commit this issue by merging from that branch. See:

http://drupal.org/project/1260830/git-instructions

Status:Needs review» Needs work
Issue tags:-WSCCI, -symfony

The last submitted patch, 1874500-routing-take-2.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+WSCCI, +symfony

#31: 1874500-routing-take-2.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community

Thanks.

Looked it over and it looks good to me, but I'll leave it open for 24 more hours or so as catch seem to have an interest in it.

I'm OK with the follow-up Crell opened. I couldn't find anything else to complain about which is always disturbing so Dries go ahead (or I'll get it late tomorrow if I'm around and it's not in yet).

Status:Reviewed & tested by the community» Fixed

This got merged by Dries. Marking fixed.

Automatically closed -- issue fixed for 2 weeks with no activity.