CMF-based Routing system

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?

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

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.

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?

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.

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.

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).

This got merged by Dries. Marking fixed.