| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | routing system |
| Category: | task |
| Priority: | major |
| Assigned: | catch |
| Status: | closed (duplicate) |
| Issue tags: | Needs issue summary update, Needs profiling, WSCCI |
Issue Summary
Blocker for
#1808080: Add an _internal route
#1597696: Switch page caching to HttpCache
Why this is a blocker
One of the goals for WSCCI is to enable ESI-based subrequests. These could be used for all blocks if SCOTCH comes to fruition, or just in selected cases if not. Symfony already offers support for doing so in a clean fashion, iff we support it properly.
The HttpKernel we're using (borrowed from Symfony's Framework Bundle), contains a method, render(). That method is called with the name of a controller. render() then determines what the best strategy is based on the request information: Render an ESI tag, render an hInclude tag, or fire a subrequest immediately and just inline the result. That distinction is, by design, completely transparent to the caller. It must be.
In order to generate a URI for the ESI tag or hInclude tag, render() calls HttpKernel::generateInternalUrl(). It looks like so:
<?php
public function generateInternalUri($controller, array $attributes = array(), array $query = array(), $secure = true)
{
if (0 === strpos($controller, '/')) {
return $controller;
}
$path = http_build_query($attributes, '', '&');
$uri = $this->container->get('router')->generate($secure ? '_internal' : '_internal_public', array(
'controller' => $controller,
'path' => $path ?: 'none',
'_format' => $this->container->get('request')->getRequestFormat(),
));
if ($queryString = http_build_query($query, '', '&')) {
$uri .= '?'.$queryString;
}
return $uri;
}
?>That is, it builds a URI to a magic route, named _internal, that contains all the information it needs to then route the subsequent ESI or hInclude request to the right controller to generate the appropriate portion of the page. It does so using the generate() method of the "router" service.
Drupal has no router service yet. What is a router service? It is a router, ie, an object that conforms to the RouterInterface:
http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/vendo...
That is, it is a combination of a matcher and a generator. We already have a matcher. We do not have a generator. This issue is adding a generator.
What is a generator? A generator is the opposite of a matcher. Whereas a Matcher takes a Request and locates a Route, a Generator takes a Route and parameters and returns a URL. A Router, in Symfony speak, is simply both of those coupled together. Adding a Matcher and Generator has been on the WSCC roadmap since DrupalCon Denver in March: http://groups.drupal.org/node/22026
So then what's the chain of events here?
1) Add a Generator. Without a Generator, we cannot use HttpKernel::render() to generate internal requests.
2) Add an _internal route, and its corresponding _internal_public. Those routes are what all ESI/hInclude requests go through: #1808080: Add an _internal route
3) Add a Router service, by simply combining our existing matcher and Generator.
That enables the use of render(). That, in turn, allows for ESI anywhere.
That "ESI" is the same for a real Varnish server or for using Symfony's HttpCache: #1597696: Switch page caching to HttpCache. That is by design. Even if you're not running Varnish, you can benefit from the resulting "Block Caching TNG". And that could be implemented even on our current Drupal 7-era crappy blocks system, although it would certainly be better with the new-hotness SCOTCH blocks system.
In addition, I have been talking to the Symfony CMF team for some time and we are planning to merge our routing libraries. That would offer them our more robust and flexible matcher, and us the more robust chain handling and other tools in the CMF Routing component. (This was discussed back in the original router/matcher issue, too: #1606794: Implement new routing system). However, doing so and then switching to the CMF Routing component also requires using a Router.
Symfony uses the generator for all URL generation. While I think we should, there is a performance question there. Because we have enough routes that we cannot load them all into memory at once, we have a single simple DB hit per route to load the route object. (On the level of a url alias lookup hit.) The immediate intention of this patch is enabling the render() method, which means only a very few calls per page (one per subrequest/ESI). That is, negligible, given that this is on the critical path to enabling partial page caching. If we extend it and start using it for all URL generation, it will need to have some performance tuning done to it, likely with caching. However, if we are to do that then it would be logical to also combine in url(), path alias lookup, and hook_url_outbound_alter(), since those all mess with outgoing paths in disparate ways. Doing all of those at once, in the generator, and then caching the result would offer considerably more flexibility and performance.
However, that is out of scope for this issue. It is also blocked on #1269742: Make path lookup code into a pluggable class, which turns path aliasing into an injectable class that could be passed into the generator.
Even if none of that happens, this is a blocking step to enabling ESI requests, and the limited usage of it there is not an appreciable performance hit.
So why critical?
I am working on the assumption that the feature set described above qualifies as a "new feature", and thus has to be in before 1 December. If I am wrong, and the committers agree that the entire pipeline and issue set described above can happen post-feature-freeze, then we can demote this to major and postpone it on #1269742: Make path lookup code into a pluggable class, after which we can do the entire generator/url/alias/hook_url_outbound_alter conversion at once, with whatever caching we end up with for performance. That will take a while, though, which means we don't even get to having the possibility of doing ESI until sometime in February, probably.
Absent such assurances that a February timeline for adding the new feature of "ESI/hInclude/partial page caching out of the box" is acceptable, I consider this issue critical, RTBC, and request that it be committed before BADCamp so that we can work on the next steps at BADCamp.
Original
Goal: a class that implements UrlGeneratorInterface and handles Drupal routes as well as actual paths or external URLs
We should make the current url a wrapper on this so that we can get it into core without changing all the url calls.
The Drupal generator should ideally have the same constructor as the symfony one - hence we need to have a route collection that taks to the Drupal DB?
On the performance front, the current url() function doesn't actually touch the DB other than to find the path alias. this makes me worry.
A possible approach to moderate the impact is that the menu links could reference both the instantiated path as well as the route machine name? In other words, a menu link entry is effectively a cache for the output of the generator?
Or should the generator or routecollection cache routes that have already been seen in this request?
Consider drupal.org with e.g. 10k menu links used by the book module. They are all node paths, so ideally we don't need to hit the DB more than once when generating the output links.
Comments
#1
Started working on this yesterday - my initial conclusions:
we should implement a read-only route collection that pulls the desired route from the DB.
We should extend the existing symfony generator class rather than re-implementing or copying most of the code there which is needed for generating a URL from a route. The child class should just add a method to do what url() does now.
I pushed some code, but it's not working yet.
#2
Moving to Core, since the router is about to go in which means this can/should be done directly on core.
#3
#4
Tagging
#5
Ok, I'll try to migrate partial code to here.
#6
This is a blocker for a number of other issues now, so critical:
#1808080: Add an _internal route
#1597696: Switch page caching to HttpCache
And a few others.
#7
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:
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.
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.
Next steps:
PS: The patch contains the whole hacked Symfony\Component\Routing, that's why it's so big.
#8
#9
The last submitted patch, 0001-1705488-prototype-of-DrupalUrlGenerator.patch, failed testing.
#10
Outch. My patch is against Crell's ancient wscci sandbox. Back to the laboratory...
#11
Ok. This one is against 8.x. Let's see...
#12
The last submitted patch, 0001-1705488-prototype-of-DrupalUrlGenerator.patch, failed testing.
#13
g.oechsler and I talked on Skype. He's going to take a second crack at this with more background context. Please stand by. :-)
#14
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.
#15
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.
#16
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.
#17
This is g.oechsler's work, I'm just posting the patch here for review purposes.
#18
Correction, this one.
#19
+++ 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.
#20
Thank you for coaching, Larry! Here is another shot on this.
#21
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.
#22
+++ 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.
#23
#24
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?
#25
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.
#26
Here we go with another try.
#27
#28
+++ 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.
#29
I'm racing a turtle here. Half way there! :)
#30
+++ 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
#31
Ah yes, and never use private properties. Those should be protected.
#32
Now UrlGenerator is registered to the DIC, a test for this exists and all of it got more docs and less stray whitespace.
#33
#34
The last submitted patch, 1705488-generator.patch, failed testing.
#35
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.
#36
Fixed last quirks and rebased against upstream/8.x.
#37
Rocking! Let's get this committed and move on to the next steps.
#38
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
#39
Quickly addressing #38.
#40
Here it is.
#41
+++ 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
#42
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
#43
Lots of traffic here! aspilicous: #42 is fixing these as well!
#44
I'm afraid the last patch had one backslash too much... I wonder how that one sneaked in.
#45
oopps sorry. Just saw a quick fix needed, and assigned anonymous.
Hope this gets in soon. Good luck.
#46
Just skimmed it over again, and all the \ look right, I think.
#47
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.
#48
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.
#49
"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.
#50
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.
#51
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.)
#52
Looks like the branch you created the patch in #51 from needs a rebase/merge against 8.x.... That's a bit too big ;)
#53
Ahem... One of these days I'll learn how to use Git...
#54
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!
#55
+ 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.
#56
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.
#57
Re-assigning to catch.
#58
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: Switch page caching to HttpCache 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.
#59
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.
#60
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".
#61
Talked this over with catch at BADCamp. New plan is here: #1830854: [meta] The ESI pipeline battle plan
#62
This is now folded into #1874500: CMF-based Routing system.