Implement a generator for Drupal paths

Moving to Core, since the router is about to go in which means this can/should be done directly on core.

Tagging

This is a blocker for a number of other issues now, so critical:

#1808080: Add an _internal route
#1597696: Consider whether HttpCache offers any significant benefit over the existing page cache

And a few others.

Good news: I was working on this issue and at least got anywhere.
Bad news: It's a horrible hack. I's not meant to be commited, it's meant to be discussed.

What I did:

I implemented the class DrupalUrlGenerator extending Symfony's UrlGenerator.
DrupalUrlGenerator has a method url() which is returning the url for the path requested.
This is basically achieved by calling the original url() function from common.inc, which I simply moved to original_url().
The function url() in common.inc is now glue code to call DrupalUrlGenerator::url();
Every Route which is discovered within a request is cached in a RouteCollection.

There are some problems with this:

1. RouteCollection::add() won't allow slashes in names, so adding a route for "admin/content" would not work. (Only names matching /^[a-z0-9A-Z_.]+$/ are allowed.)
   I resolved this by extending it to DrupalRouteCollection which allows adding anything as name.
   To make this work I had to hack Symfony's RouteCollection, making the attribute $routes protected to be accessible from the subclass.
2. DrupalUrlGenerator::doGenerate(), which is actually meant to generate the pass, is now simply returning the output generated by original_url().
   So adding complex routes with parameters, variables, requirements a.s.o. is impossible. This especially hurts for routes like "node/42" or "user/foo", which could be handled by route definitions with variables defined in node.module or user.module respectively.
3. Calling the original url code totally misses the point.

Next steps:

1. Get feedback to make sure I'm not running in a dead end here.
2. Refactoring and moving the code from original_url() to DrupalUrlGenerator::doGenerate(). This would resolve mostly problems 2 and 3.
3. Find a smarter way to add routes with slashes in their names, without hacking Symfony. There is a good chance that I got it totally wrong! Please let me know!
4. Provide a way to add Route definitions in modules. (Probably something like drupal_add_route()?)

PS: The patch contains the whole hacked Symfony\Component\Routing, that's why it's so big.

Outch. My patch is against Crell's ancient wscci sandbox. Back to the laboratory...

Ok. This one is against 8.x. Let's see...

g.oechsler and I talked on Skype. He's going to take a second crack at this with more background context. Please stand by. :-)

Can someone clarify why this is a critical task, worth holding up other features? The OP doesn't make a whole lot of sense to me. Demoting to major until that is identified.

This is a blocker for the _internal route support from Symfony, which in turn blocks all of the sub-requests-that-cache-independently-so-we-can-get-all-the-macro-level-performance-benefits-we've-been-talking-about work. SCOTCH will need to do work after that happens too, to leverage it.

Ok, let's do this then. Cos I seriously do not understand what problem this issue is trying to solve and how all of these things can possibly be blocked on it.

This is g.oechsler's work, I'm just posting the patch here for review purposes.

Correction, this one.

+++ b/core/lib/Drupal/Core/Routing/DrupalUrlGenerator.php
@@ -0,0 +1,46 @@
+class DrupalUrlGenerator extends UrlGenerator {

Class names should not have "Drupal" in them. The preferred approach in this case is to alias UrlGenerator in the use statement to BaseUrlGenerator or SymfonyUrlGenerator, and then name ours just "UrlGenerator".

+++ b/core/lib/Drupal/Core/Routing/DrupalUrlGenerator.php
@@ -0,0 +1,46 @@
+  public function __construct() {
+    //I need to pass some RouteCollection and RequestContext.
+    //Empty ones will do for now.
+    parent::__construct(new RouteCollection(), new RequestContext(), null);
+  }

I'm not sure we need the parent constructor at all, do we? If not, we can skip the RouteCollection and just accept a RequestContext as a constructor parameter to our generator.

I think an empty RequestContext is going to fail tests, because it's needed for things like the base path, scheme, etc.

+++ b/core/lib/Drupal/Core/Routing/DrupalUrlGenerator.php
@@ -0,0 +1,46 @@
+    $result = db_query("select route from {router} where name = :name", array(':name' => $name));
+    $matches = $result->fetchAll();

This should take a DB connection as a constructor argument, which gets assigned to a protected $connection property. Then here, we can call $this->connection->query().

We also don't need to select multiple routes here. All route names are unique, so we can do a ->fetchObject() call rather than fetchAll() to just get back the first (only) result.

+++ b/core/lib/Drupal/Core/Routing/DrupalUrlGenerator.php
@@ -0,0 +1,46 @@
+    //Test fixtures set compiler_class to Drupal\Core\Routing\RouteCompiler
+    //which produces compiled routes without a getVariables method
+    //fixing it hardcoded for now.
+    $route->setOption('compiler_class', '\Symfony\Component\Routing\RouteCompiler');

We may want to combine the compilers at some point. Not sure yet.

+++ b/core/lib/Drupal/Core/Routing/DrupalUrlGenerator.php
@@ -0,0 +1,46 @@
+    // the Route has a cache of its own and is not recompiled as long as it does not get modified
+    $compiledRoute = $route->compile();

I don't understand this comment. The compiled route is not stored in the database, because it breaks if we do that. :-)

+++ b/core/modules/system/lib/Drupal/system/Tests/Routing/UrlGeneratorTest.php
@@ -0,0 +1,128 @@
+  function __construct($test_id = NULL) {
+    parent::__construct($test_id);
+
+     $this->fixtures = new MyRoutingFixtures();
+  }

Why the extra Fixtures class? If you need to change something, let's just push the changes back on the Fixtures class.

+++ b/core/modules/system/lib/Drupal/system/Tests/Routing/UrlGeneratorTest.php
@@ -0,0 +1,128 @@
+    $generator = new DrupalUrlGenerator();

This is where we can pass in a DB connection and RequestContext for testing.

+++ b/core/modules/system/lib/Drupal/system/Tests/Routing/UrlGeneratorTest.php
@@ -0,0 +1,128 @@
+      $this->assertEqual($route->getPattern(), $url, 'Checking path generation for '. $name);

By convention, the message string should, I believe, be something like "Correct path generated for route $name". That way it reads correctly if the test comes back green.

Thanks, George! Let's see what the bot things.

I also rebased your branch against upstream/8.x (that's why the original patch above was so huge). You'll need to delete your local branch and pull the new one with the same name.

Thank you for coaching, Larry! Here is another shot on this.

I tried to implement all of what was suggested in #19.

Right now I still use an empty RequestContext object in the UrlGenerator as it apparently does not break the tests. However, this might be because of the tests: I'm expecting "URLs" like "/node/add/article" right now, which works perfectly fine, if the $baseUrl is an empty string coming from an empty context.
Is this alright? If the UrlGenerator should be able to produce something like https://www.example.com:8081/node/add/article or other fancy stuff some context could be really helpful, though.

On the other hand UrlGenerator is a lonely object doing no harm. I wonder how this should be integrated to actually do something useful - not even daring to ask how it relates to this _internal URL thingy.

More coaching is very welcome. Cheers!

PS: I dropped the "Route has a cache of it's own" comment. It was simply copy and waste from Symfony's UrlGenerator code.

+++ b/core/lib/Drupal/Core/Routing/UrlGenerator.php
@@ -0,0 +1,45 @@
+    //Test fixtures set compiler_class to Drupal\Core\Routing\RouteCompiler
+    //which produces compiled routes without a getVariables method
+    //fixing it hardcoded for now.
+    $route->setOption('compiler_class', '\Symfony\Component\Routing\RouteCompiler');

We need to fix this before committing, although I'm not sure if we fix it by adjusting the text fixtures or setting a compiler explicitly. Or merging compilers, although that may have unpleasant performance implications. Hm. Opinions welcome.

+++ b/core/modules/system/lib/Drupal/system/Tests/Routing/MatcherDumperTest.php
@@ -117,7 +117,7 @@ function testAddAdditionalRoutes() {
   public function testDump() {
     $connection = Database::getConnection();
-    $dumper= new MatcherDumper($connection, 'test_routes');
+    $dumper= new MatcherDumper($connection);
 
     $route = new Route('/test/{my}/path');
     $route->setOption('compiler_class', 'Drupal\Core\Routing\RouteCompiler');
@@ -130,7 +130,7 @@ public function testDump() {

@@ -130,7 +130,7 @@ public function testDump() {
 
     $dumper->dump(array('route_set' => 'test'));
 
-    $record = $connection->query("SELECT * FROM {test_routes} WHERE name= :name", array(':name' => 'test_route'))->fetchObject();
+    $record = $connection->query("SELECT * FROM {router} WHERE name= :name", array(':name' => 'test_route'))->fetchObject();
 

Why the change to the dumper tests? We're not using the default table deliberately to avoid any potential collision with the host site, and to demonstrate that we can. Let's remove this change.

Instead, the generator should take a table name as a parameter, too, and default to the same table name as the dumper. That way, all key components in the system can use any table name we want.

+++ b/core/modules/system/lib/Drupal/system/Tests/Routing/UrlGeneratorTest.php
@@ -0,0 +1,125 @@
+    $generator = new UrlGenerator($connection, new RequestContext());

We definitely need to test this with multiple RequestContexts. As you note, the base path is empty so a lot of things are, I think, working just kinda by accident. Let's make a fake request, use that to make a request context, and then test with that.

I saw that MatcherDumper::dump() is writing the route objects and data from the compiled routes to the database.

I think I could join the route compilers and write the data from Symfony's CompiledRoute (that is variable and routes) in extra columns to the database as well.

This way I could skip the compilation step in UrlGenerator::generate(), which should rather speed up things, than slowing it down.

Could this be a way to go?

If we can reasonably combine our compilers, fine. However, since we're running compile for routes in the matching process, we don't want to make that too expensive. Refactoring that is out of scope for now, so I'm inclined to punt on it. Part of the difficulty there is that Symfony's compiled route object is not at all designed to be extended. It's rather poorly architected, frankly. :-)

I think for now let's just get it working. We have another 4 months to optimize the compilation time code after we get the functionality in. Maybe for now we just explicitly set the compiler class before compiling, and ignore the fact that it is sometimes redundant? (That's what we're doing in the dumper, basically.) Alternatively, we can remove setting the compiler in the fixtures and move that into each of the tests. I'm fine either way.

Here we go with another try.

+++ b/core/lib/Drupal/Core/Routing/UrlGenerator.php
@@ -0,0 +1,48 @@
+    $result = $this->connection->query('select route from {' . $this->table . '} where name = :name', array(':name' => $name));
+    $row = $result->fetchObject(); ¶

$this->table needs to be wrapped in $this->connection->escapeTable().

Since there's only a single field coming back, we can skip fetchObject() and call fetchField(). That will return either the value of the route column, or NULL. Then the unserialize() call changes to $route = unserialize($route);

Just slightly cleaner.

And holy crap those tests are thorough! :-) Let's see how bot likes 'em. If it passes, then I think we can wire this into the DIC and move forward.

I'm racing a turtle here. Half way there! :)

+++ b/core/lib/Drupal/Core/Routing/UrlGenerator.phpundefined
@@ -0,0 +1,48 @@
+ * DrupalUrlGenerator generates a URL based on definitons in the router table.

I think the class name is not prefixed with Drupal anymore

+++ b/core/lib/Drupal/Core/Routing/UrlGenerator.phpundefined
@@ -0,0 +1,48 @@
+  private $connection;
+  private $table;

Don't forget to add docs

+++ b/core/lib/Drupal/Core/Routing/UrlGenerator.phpundefined
@@ -0,0 +1,48 @@
+    $route = $result->fetchField(); ¶

trailing whitespace

Inline comments in the tests would be helpfull

Ah yes, and never use private properties. Those should be protected.

Now UrlGenerator is registered to the DIC, a test for this exists and all of it got more docs and less stray whitespace.

All I have left is minor nitpicks. Fix these and I think we're done!

+++ b/core/lib/Drupal/Core/Routing/UrlGenerator.php
@@ -0,0 +1,88 @@
+    $this->context = new RequestContext();

This needs to be a declared property on the object.

+++ b/core/lib/Drupal/Core/Routing/UrlGenerator.php
@@ -0,0 +1,88 @@
+   public function generate($name, $parameters = array(), $absolute = false) {

false should be FALSE.

+++ b/core/modules/system/lib/Drupal/system/Tests/Routing/UrlGeneratorTest.php
@@ -0,0 +1,164 @@
+use Exception;

You don't need to use Exception. Just reference it as \Exception inline. (Newly revised coding standards.)

+++ b/core/modules/system/tests/modules/router_test/lib/Drupal/router_test/TestControllers.php
@@ -30,4 +30,7 @@ public function test4($value) {
+  public function test5() {
+    return drupal_container()->get('router.generator')->generate('router_test_2');
+  }

I know this is just a test, but let's set a good example. :-) Make the TestControllers class extend from Symfony's ContainerAware class, and then you can use $this->container->get() instead of drupal_container(). That's cleaner.

Fixed last quirks and rebased against upstream/8.x.

Rocking! Let's get this committed and move on to the next steps.

I'm so srry.... Let's fix this quickly...

+++ b/core/lib/Drupal/Core/Routing/UrlGenerator.phpundefined
@@ -0,0 +1,95 @@
+   * @param Symfony\Component\HttpFoundation\Request $request

This should start with a \ too

+++ b/core/lib/Drupal/Core/Routing/UrlGenerator.phpundefined
@@ -0,0 +1,95 @@
+   * @throws RouteNotFoundException

Full path please

Quickly addressing #38.

Here it is.

+++ b/core/lib/Drupal/Core/Routing/UrlGenerator.phpundefined
@@ -0,0 +1,95 @@
+   * @var Drupal\Core\Database\Connection

The name standard is to prefix these paths with a \to please editors. Please do that in this patch, now I have your attention ;)

+++ b/core/lib/Drupal/Core/Routing/UrlGenerator.phpundefined
@@ -0,0 +1,95 @@
+   * @var Symfony\Component\Routing\RequestContext

Same

+++ b/core/lib/Drupal/Core/Routing/UrlGenerator.phpundefined
@@ -0,0 +1,95 @@
+   * @throws Symfony\Component\Routing\Exception\RouteNotFoundException

Same

Wow. Two people working on this in parallel :) I was working on this just the second I saw you volunteering, mbrett5062. Thanks anyway! On the way I found a few other namespace issues in the docs... fixing them with this

Lots of traffic here! aspilicous: #42 is fixing these as well!

I'm afraid the last patch had one backslash too much... I wonder how that one sneaked in.

oopps sorry. Just saw a quick fix needed, and assigned anonymous.

Hope this gets in soon. Good luck.

Just skimmed it over again, and all the \ look right, I think.

So I asked for an issue summary here because I have no idea what this patch is doing, and reading the docs and tests in this patch, I still don't. So most of my feedback is docs-related.

+++ b/core/lib/Drupal/Core/Routing/UrlGenerator.phpundefined
@@ -0,0 +1,95 @@
+/**
+ * UrlGenerator generates a URL based on definitons in a database table.
+ */
+class UrlGenerator extends SymfonyUrlGenerator {

Let's expand the PHPDoc here to explain this concept. How do I know when I need to use UrlGenerator in my module? What's the use case for why it exists?

Also, (nitpick) "definitions"

+++ b/core/lib/Drupal/Core/Routing/UrlGenerator.phpundefined
@@ -0,0 +1,95 @@
+    $result = $this->connection->query('select route from {' . $this->connection->escapeTable($this->table) . '} where name = :name', array(':name' => $name));

Should be SELECT ... FROM ... WHERE for consistency with what we do elsewhere.

+++ b/core/lib/Drupal/Core/Routing/UrlGenerator.phpundefined
@@ -0,0 +1,95 @@
+    // We cannot know if and what compiler class is set in the route coming from
+    // the database. To make Symfony's URL generation code work we have to set
+    // it here explicitly.
+    // See http://drupal.org/node/1705488#comment-6645232
+    $route->setOption('compiler_class', '\Symfony\Component\Routing\RouteCompiler');

Rather than putting a link to a specific comment here (which we tend not to do), let's re-formulate what's being said there into an actual @todo in the source code.

+++ b/core/modules/system/lib/Drupal/system/Tests/Routing/RouterTest.phpundefined
@@ -86,4 +86,14 @@ public function testControllerPlaceholdersDefaultValues() {
+  /**
+   * Confirms we can generate URLs.
+   */
+  public function testUrlGenerator() {
+    // The controller behind test5 returns a generated URL for test2. As we
+    // know how it should look like, we can test if the UrlGenerator registered
+    // to the container is actually working.
+    $this->drupalGet('router_test/test5');
+    $this->assertRaw('router_test/test2', 'The correct URL was returned because it was generated correctly.');
+  }

Sorry, what? How does this test show that generation is working properly..?

+++ b/core/modules/system/lib/Drupal/system/Tests/Routing/UrlGeneratorTest.phpundefined
@@ -0,0 +1,162 @@
+      'description' => 'Confirm that the url generation code is working correctly.',

(nitpick) URL

+++ b/core/modules/system/lib/Drupal/system/Tests/Routing/UrlGeneratorTest.phpundefined
@@ -0,0 +1,162 @@
+    $this->requests = array(
+      Request::create('http://test.com/foo?bar=baz'),
+      Request::create('https://test.com/foo?bar=baz'),
+      Request::create('test.com:90/foo'),
+      Request::create('https://test.com:90/foo'),
+      Request::create('https://127.0.0.1:90/foo'),
+      Request::create('https://[::1]:90/foo'),
+      Request::create('http://example.com/jsonrpc', 'POST', array(), array(), array(), array(), '{"jsonrpc":"2.0","method":"echo","id":7,"params":["Hello World"]}'),
+      Request::create('http://test.com'),
+      Request::create('http://test.com:90/?test=1'),
+      Request::create('http://test:test@test.com'),
+    );

Can we get a one or two-line comment above this array that explains the rationale for these particular requests?

+++ b/core/modules/system/lib/Drupal/system/Tests/Routing/UrlGeneratorTest.phpundefined
@@ -0,0 +1,162 @@
+   * Confirms correct URL generation for a sample RouteCollection without any
+   * parameters.
...
+   * Confirms correct URL generation for a more complex RouteCollection with
+   * parameters.

(nitpick) Please re-word so fits into 80 characters (or 80 characters and then additional sentences below).

+++ b/core/modules/system/lib/Drupal/system/Tests/Routing/UrlGeneratorTest.phpundefined
@@ -0,0 +1,162 @@
+        $this->assertEqual($expected_url, $url, "Correct path generated for route $name.");

This assertion method should use format_string() rather than direct interpolation of variables.

+++ b/core/modules/system/tests/modules/router_test/lib/Drupal/router_test/TestControllers.phpundefined
@@ -30,4 +31,7 @@ public function test4($value) {
+  public function test5() {
+    return $this->container->get('router.generator')->generate('router_test_2');
+  }

This function needs PHPdoc, hopefully which helps explain what the heck is going on here.

What is a Generator: http://symfony.com/doc/current/components/routing.html

See the section "Generate a URL".

This is for making URLs that point to a Drupal-defined route. (We've only been talking about it in public in WSCCI postings since DrupalCon Denver...) It's critical because Symfony's HttpKernel relies on a generator in order to make links for ESI and hInclude. No generator, no partial page caching, no performance wins.

"Symfony's HttpKernel relies on a generator in order to make links for ESI and hInclude. No generator, no partial page caching, no performance wins."

Great, so include some wording to that effect in the UrlGenerator PHPDoc to help orient people a bit.

"(We've only been talking about it in public in WSCCI postings since DrupalCon Denver...)"

That sort of snark isn't appreciated, thanks. People reading D8 source code 3 years from now are not going to even know there was a DrupalCon Denver. It needs to be clear from the docs in the code what is happening here.

Here's my shot on most in #47.

I got a bit chatty in the comments, I'm afraid. Especially with RouterTest:testUrlGenerator(), where the whole setup of the test is scattered over three files. While trying to point out how this all fits together, I ran in a quite unusual problem: The path where to find a certain file, like core/modules/system/tests/modules/router_test/lib/Drupal/router_test/TestControllers.php won't fit in 80 characters. I went with a backslash break those extra long lines, but I'm not sure if that's ok and I could not find a hint to handle this in the coding standards.

One thing remains:

"Let's expand the PHPDoc here to explain this concept. How do I know when I need to use UrlGenerator in my module? What's the use case for why it exists?"

I don't know when and why to use an UrlGenerator in a module. In the issue summary it says something about replacing url(). If this happend the use case would be clear immediately, I guess. Until then, I'll have to take Crell's word, that it's somehow important for some weird stuff I had to google before I knew what he was talking about. :) I'm pretty sure I do not want to elaborate on ESI et al. in the docs of UrlGenerator - this innocent little class simply does not care about webservers or caching subrequests. My problem is that I honestly have no clue what to write there apart of the obvious: It generates URLs.

Revised version of #50 that includes an improved class docblock to explain what it does.

If this is still bot-happy, let's get it committed so we can move on to the next steps. (And close this critical.)

Looks like the branch you created the patch in #51 from needs a rebase/merge against 8.x.... That's a bit too big ;)

Ahem... One of these days I'll learn how to use Git...

Ah, that's some smart wording for the class docs. The code was already agreed upon and documentation is quite insightful now. Looks like we're done!

+   public function generate($name, $parameters = array(), $absolute = FALSE) {
+    $result = $this->connection->query('select route from {' . $this->connection->escapeTable($this->table) . '} where name = :name', array(':name' => $name));
+    $route = $result->fetchField();

This is supposed to be used instead of url() right? If that's the case (or even if it isn't, supposing sufficient usage), then adding a database query for every call is completely unacceptable. I said this very clearly in the router issue (when it had a stub generator implementation) and that bit hasn't changed here.

Haven't reviewed the rest of the patch yet.

It is not (yet) a complete replacement for url(). url() if nothing else will still be available for arbitrary URLs. But this is a requirement to link to routes by route name, which is mandatory if we want to use the render() method of HttpKernel.

We can add caching around this if we need to, but that can be done post-feature-freeze. There's other enhancements we can make post-freeze, too. Right now this is a feature blocker for the ESI-all-the-things pipeline that we've been trying to get to since forever.

Re-assigning to catch.

No I'm not committing patches with obvious performance regressions and no discussion on how to fix them because "we can fix that after feature freeze", there's already a long list of issues at #1744302: [meta] Resolve known performance regressions in Drupal 8 that also need to be fixed after feature freeze and it's not getting any shorter.

Blocks at their own URI is already a critical task, as is this issue, so it can go in after feature freeze, without introducing yet another performance regression just for the promise of ESI (which is not going to improve out of the box core performance at all without solving a tonne of far more complicated issues than this one).

Also I don't see how this possibly blocks #1597696: Consider whether HttpCache offers any significant benefit over the existing page cache since that's for the page cache, doesn't need sub-requests or anything like that. There's no discussion in that issue at all of how this one is a blocker, it's been blocked on quite unrelated stuff for a while now.

I think the closest solution here would be to implement "static" caching along the lines of menu_get_item(), which retrieves, translates, and checks each requested path/route for access, depending on the passed arguments, in HEAD already.

Of course, every l() and/or url() is a completely different dimension, but at least I think that comes closest.

Updated issue summary. Add blockers

Updated the summary with a detailed review of the work this issue is blocking and why it is not a performance issue in its current form. There is no performance regression in this patch. There is "work to be finished later that is blocked on other issues still".

Talked this over with catch at BADCamp. New plan is here: #1830854: [meta] The ESI pipeline battle plan

This is now folded into #1874500: CMF-based Routing system.

Elaborate on the full dependency chain that this patch is blocking.