This is a follow-up for #1888424: Make Drupal's URL generation logic available to HttpKernel, and minimize code repetition/divergence, and postponed on it.

Now that we have all link generation centralized into one of two methods on the generator, we need to make it faster. The obvious way to do that is to cache requests to generateFromRoute() and generateFromPath(), so that we catch all of the work done by the outbound path processors.

For path alias lookups, we did that with a wrapping object. I think it makes sense to do the same here, with an object that has the same interfaces as the generator but simply forwards requests to it and then caches them. The pattern established by the path alias cache should work fine here. (I think it's now using the Destructable interface? If not, we should do that.)

We may also want to consider removing the cache wrapper from path alias lookups once we're done. We probably shouldn't do that in this patch, but a follow up would be to benchmark and see if it's still useful, and if not, get rid of it to avoid the complexity and extra cache storage.

Files: 
CommentFileSizeAuthor
#70 interdiff-68-70.txt786 bytesmartin107
#70 url_generator-1965074-70.patch24.54 KBmartin107
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,328 pass(es), 0 fail(s), and 3 exception(s).
[ View ]
#68 url_generator-1965074-68.patch24.53 KBmartin107
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,129 pass(es), 0 fail(s), and 35 exception(s).
[ View ]
#68 interdiff-66-68.txt918 bytesmartin107
#66 url_generator-1965074-66.patch24.57 KBmartin107
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,042 pass(es), 84 fail(s), and 72 exception(s).
[ View ]
#66 interdiff-65-66.txt1.42 KBmartin107
#65 interdiff-62-65.txt634 bytesmartin107
#65 url_generator-1965074-65.patch24.61 KBmartin107
#62 interdiff-60-62.txt899 bytesmartin107
#62 url_generator-1965074-62.patch24.55 KBmartin107
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,847 pass(es), 0 fail(s), and 36 exception(s).
[ View ]
#60 url_generator-1965074-60.patch24.72 KBmartin107
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,788 pass(es), 1 fail(s), and 36 exception(s).
[ View ]
#60 interdiff-57-60.txt2.66 KBmartin107
#57 url_generator-1965074-57.patch24.63 KBmartin107
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,740 pass(es), 7 fail(s), and 36 exception(s).
[ View ]
#57 interdiff-51.57.txt784 bytesmartin107
#51 url_generator-1965074-50.patch24.64 KBmartin107
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#50 interdiff-47-50.txt2.96 KBmartin107
#50 url_generator-1965074-50.txt24.64 KBmartin107
#47 url_generator-1965074-47.patch23.88 KBmartin107
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,821 pass(es), 54 fail(s), and 2,136 exception(s).
[ View ]
#42 url_generator-1965074-42.patch25.45 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,700 pass(es).
[ View ]
#37 url_generator-1965074-37.patch25.47 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch url_generator-1965074-37.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#37 1965074-35-37.increment.txt617 bytespwolanin
#35 1965074-32-35.increment.txt735 bytespwolanin
#35 url_generator-1965074-35.patch25.47 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 58,451 pass(es).
[ View ]
#32 url_generator-1965074-32.patch25.35 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 54,837 pass(es), 1,663 fail(s), and 51,887 exception(s).
[ View ]
#32 interdiff.txt10.04 KBdawehner
#31 1965074-30-31.increment.txt885 bytespwolanin
#31 cache-url_generator-1965074-31.patch30.4 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 58,452 pass(es).
[ View ]
#30 cache-url_generator-1965074-30.patch29.54 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] 58,603 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#30 1965074-29-30.increment.txt519 bytespwolanin
#29 1965074-27-29.increment.txt10.57 KBpwolanin
#29 cache-url_generator-1965074-29.patch29.56 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] 58,550 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#27 cache-url_generator-1965074-27.patch21.12 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] 58,390 pass(es), 8 fail(s), and 0 exception(s).
[ View ]
#27 1965074-21-27.increment2.txt3.14 KBpwolanin
#21 cache-url_generator-1965074-21.patch20.91 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] 54,355 pass(es), 938 fail(s), and 482 exception(s).
[ View ]
#21 1965074-19-21.increment.txt10 KBpwolanin
#19 cache-url_generator-1965074-19.patch20.79 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 58,134 pass(es), 8 fail(s), and 0 exception(s).
[ View ]
#19 interdiff.txt4.03 KBdawehner
#17 cache-url_generator-1965074-17.patch17.22 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 58,142 pass(es), 10 fail(s), and 0 exception(s).
[ View ]
#10 drupal-1965074-10.patch15.61 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 57,855 pass(es), 10 fail(s), and 0 exception(s).
[ View ]
#10 interdiff.txt2.54 KBdawehner
#8 drupal-1965074-8.patch17.87 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 57,655 pass(es), 10 fail(s), and 0 exception(s).
[ View ]
#8 interdiff.txt2.95 KBdawehner
#6 drupal-1965074-6.patch15.73 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 57,380 pass(es), 13 fail(s), and 302 exception(s).
[ View ]
#4 1965074.cached-url-generator.patch10.26 KBkatbailey
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Comments

Assigned:Unassigned» katbailey
Status:Postponed» Active

I've made a bit of a start on this, will try to get a patch up at the weekend.

Can we also add a Drupal::generator() or something like that method here while we're at it.

And, let's not do something and something.cached as service names, because people end up using the wrong service. Maybe something and something.uncached , maybe the uncached one can even be a private service?

Drupal::urlGenerator() was already added in the previous issue.

StatusFileSize
new10.26 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Ugh, sorry - I have been very distracted lately. Here are the beginnings of a patch - it totally breaks the installer and I haven't yet figured out what exactly is going wrong. Hardly seems worth setting to needs review, though I wouldn't mind getting some eyes on it to see if I'm at least going in the right direction with this as I'm not totally sure I've understood what's required.

I don't know why that would be breaking the installer, other than "it's the installer *cries*". It does look like what I expected it to look, though, so it's on the right track if nothing else. (Or I'm completely wrong, which is always a possibility.)

+++ b/core/lib/Drupal/Core/Routing/CachedURLGenerator.php
@@ -0,0 +1,178 @@
+  /**
+   * Passes all unknown calls to the decorated object.
+   */
+  public function __call($method, $args) {
+    return call_user_func_array(array($this->urlGenerator, $method), $args);
+  }

In my experience, this sometimes works out well and sometimes causes all sorts of insanity. What methods is this useful for, or is it "just in case"?

Status:Active» Needs review
StatusFileSize
new15.73 KB
FAILED: [[SimpleTest]]: [MySQL] 57,380 pass(es), 13 fail(s), and 302 exception(s).
[ View ]

Let's remove that and implement the single method which is needed (getPathFromRoute).

Found one bug while writing some tests: You also have to add the parameters to the key.

Status:Needs review» Needs work

The last submitted patch, drupal-1965074-6.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.95 KB
new17.87 KB
FAILED: [[SimpleTest]]: [MySQL] 57,655 pass(es), 10 fail(s), and 0 exception(s).
[ View ]

This fixes a couple of the failures.

Status:Needs review» Needs work

The last submitted patch, drupal-1965074-8.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.54 KB
new15.61 KB
FAILED: [[SimpleTest]]: [MySQL] 57,855 pass(es), 10 fail(s), and 0 exception(s).
[ View ]

Let's at least remove the old test code from urlgeneratorTest.

I have a feeling that at least some of the remaining test failures are caused by to strict caching. For example we don't take the language into account, but just putting it into the cacheKey did not helped.

Status:Needs review» Needs work

The last submitted patch, drupal-1965074-10.patch, failed testing.

Assigned:katbailey» Unassigned

Unassigning myself from some issues as I have very little time for core these days as I prepare to move 5000kms across the country...

Status:Needs work» Needs review

#10: drupal-1965074-10.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, drupal-1965074-10.patch, failed testing.

Also needs work since PathBasedGeneratorInterface has been replaced.

Status:Needs work» Needs review
StatusFileSize
new17.22 KB
FAILED: [[SimpleTest]]: [MySQL] 58,142 pass(es), 10 fail(s), and 0 exception(s).
[ View ]

Rerolled and added tests for generateFromRoute.

Status:Needs review» Needs work

The last submitted patch, cache-url_generator-1965074-17.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new4.03 KB
new20.79 KB
FAILED: [[SimpleTest]]: [MySQL] 58,134 pass(es), 8 fail(s), and 0 exception(s).
[ View ]

I can't fix the language based failures now.

Status:Needs review» Needs work

The last submitted patch, cache-url_generator-1965074-19.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new10 KB
new20.91 KB
FAILED: [[SimpleTest]]: [MySQL] 54,355 pass(es), 938 fail(s), and 482 exception(s).
[ View ]

So, I see a couple bugs in there ('system_path' -> '_system_path', and the wrong prefix for some entries) . Added a pass at language handling and simplified the code a bit.

Also, why don't we consider moving the in-memory caching and the clear() method to the Drupal UrlGenerator and interface? Though at that point inheritance might make more sense than decorating.

Status:Needs review» Needs work

The last submitted patch, cache-url_generator-1965074-21.patch, failed testing.

Status:Needs work» Needs review

#21: cache-url_generator-1965074-21.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, cache-url_generator-1965074-21.patch, failed testing.

+++ b/core/tests/Drupal/Tests/Core/Routing/CachedUrlGeneratorTest.php
@@ -63,11 +71,11 @@ protected function setUp() {
   public function testGenerate() {
...
-    $this->assertEquals('test-route-1', $this->cachedUrlGenerator->generate('test_route'));
-    $this->assertEquals('test-route-1', $this->cachedUrlGenerator->generate('test_route'));
+    $this->assertEquals('test-route-1', $this->cachedUrlGenerator->generateFromRoute('test_route'));
+    $this->assertEquals('test-route-1', $this->cachedUrlGenerator->generateFromRoute('test_route'));
@@ -77,15 +85,15 @@ public function testGenerate() {
-    $this->assertEquals('test-route-1/value1', $this->cachedUrlGenerator->generate('test_route', array('key' => 'value1')));
-    $this->assertEquals('test-route-1/value1', $this->cachedUrlGenerator->generate('test_route', array('key' => 'value1')));
-    $this->assertEquals('test-route-1/value2', $this->cachedUrlGenerator->generate('test_route', array('key' => 'value2')));
-    $this->assertEquals('test-route-1/value2', $this->cachedUrlGenerator->generate('test_route', array('key' => 'value2')));
+    $this->assertEquals('test-route-1/value1', $this->cachedUrlGenerator->generateFromRoute('test_route', array('key' => 'value1')));
+    $this->assertEquals('test-route-1/value1', $this->cachedUrlGenerator->generateFromRoute('test_route', array('key' => 'value1')));
+    $this->assertEquals('test-route-1/value2', $this->cachedUrlGenerator->generateFromRoute('test_route', array('key' => 'value2')));
+    $this->assertEquals('test-route-1/value2', $this->cachedUrlGenerator->generateFromRoute('test_route', array('key' => 'value2')));
   }

We are testing the generate method, so we should call the generate method.

dawehner - in our implementation that basically delegates to generateFromRoute, and I inlined that delegation in this class now. That mean you could cache once calls to generate() or generateFromRoute(). Actually to do that right, we should actually not set $options unless $absolute is TRUE.

Status:Needs work» Needs review
StatusFileSize
new3.14 KB
new21.12 KB
FAILED: [[SimpleTest]]: [MySQL] 58,390 pass(es), 8 fail(s), and 0 exception(s).
[ View ]

puts the test mostly back, and add comments and a small change to generate(), fixes use of Languge

Status:Needs review» Needs work

The last submitted patch, cache-url_generator-1965074-27.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new29.56 KB
FAILED: [[SimpleTest]]: [MySQL] 58,550 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new10.57 KB

fix language stuff mostly

StatusFileSize
new519 bytes
new29.54 KB
FAILED: [[SimpleTest]]: [MySQL] 58,603 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

fix doxygen.

StatusFileSize
new30.4 KB
PASSED: [[SimpleTest]]: [MySQL] 58,452 pass(es).
[ View ]
new885 bytes

should fix the last test

StatusFileSize
new10.04 KB
new25.35 KB
FAILED: [[SimpleTest]]: [MySQL] 54,837 pass(es), 1,663 fail(s), and 51,887 exception(s).
[ View ]

Let's try to bring down the patch size and introduce a CachedUrlGeneratorInterface similar to CachedModuleHandler and CachedPathAliasManager etc.

Missing file for CachedUrlGeneratorInterface ?

Status:Needs review» Needs work

The last submitted patch, url_generator-1965074-32.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new25.47 KB
PASSED: [[SimpleTest]]: [MySQL] 58,451 pass(es).
[ View ]
new735 bytes

can't find dawehner, so adding the missing interface (not hard to guess).

I just reviewed the interdiff. The interface contains understandable basic documentation.

+++ b/core/lib/Drupal/Core/Routing/CachedUrlGeneratorInterface.php
@@ -0,0 +1,22 @@
+ * Provides additioanl methods for generators that cache the URLs.

additioanl should be additional

StatusFileSize
new617 bytes
new25.47 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch url_generator-1965074-37.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

thanks.

Title:Add cache wrapper to GeneratorAdd cache wrapper to the UrlGenerator

updating title

#37: url_generator-1965074-37.patch queued for re-testing.

#37: url_generator-1965074-37.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, url_generator-1965074-37.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+WSCCI
StatusFileSize
new25.45 KB
PASSED: [[SimpleTest]]: [MySQL] 58,700 pass(es).
[ View ]

reroll.

Status:Needs review» Needs work

I think this needs work in 1 of 2 directions.

A quick fix would be to cache just in memory, so we avoid repeating the work if the same link is generated more than once per page (not sure this is even worth doing).

Otherwise, we need to maybe put the cache set into a destructor so we can accumulate all the generated routes for a page?

From Berdir here, we can implement Drupal\Core\DestructableInterface - core will cal the destruct() method on these when shutting down the container so that's the opportunity to persist the data.

We could potentially move somehow the storage logic of AliasManagerCacheDecorator into here.

What happens here when we have dynamic query string parameters etc... like tokens?

Issue summary:View changes

Updated issue summary.

Issue summary:View changes
StatusFileSize
new23.88 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,821 pass(es), 54 fail(s), and 2,136 exception(s).
[ View ]

This issue is no longer blocked! This is just a reroll.

Last patch is from September 20, 2013 which in HEAD years this makes it an aged pensioner...

So no promises except it installs, UrlGenerator test pass locally .. lets see what testbot has to say

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 47: url_generator-1965074-47.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new24.64 KB
new2.96 KB

1) My reroll in #47 mangled the service definition of url_generator in core.services.yml.

2) Relaxed __construct of MenuLinkFormController to permit any object conforming to UrlGeneratorInterface
so we can feed it a caching version.

3) minor tidy now using the appropriate getter in CachedUrlGenerator.php

-      $this->cacheKey .= '::' . $this->languageManager->getLanguage(Language::TYPE_URL)->id;
+      $this->cacheKey .= '::' . $this->languageManager->getLanguage(Language::TYPE_URL)->getId();

StatusFileSize
new24.64 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

butter fingers

Status:Needs review» Needs work

The last submitted patch, 51: url_generator-1965074-50.patch, failed testing.

just looking at the commit log 7 commits between submission of patch and testing of patch

good o'le sprint time at NYC camp... will try again shortly

Browser install worked locally for me at #51, works for me now after commit sprint .. retesting

Status:Needs work» Needs review

51: url_generator-1965074-50.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, 51: url_generator-1965074-50.patch, failed testing.

StatusFileSize
new784 bytes
new24.63 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,740 pass(es), 7 fail(s), and 36 exception(s).
[ View ]

More careful inspection of logs shows I need to backout "minor tidy" #50.3.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 57: url_generator-1965074-57.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.66 KB
new24.72 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,788 pass(es), 1 fail(s), and 36 exception(s).
[ View ]

Trivial reroll + fixed a single issue.

Constructor of CachedUrlGenerator now takes LanguageManagerInterface

-  public function __construct(UrlGeneratorInterface $url_generator, CacheBackendInterface $cache, LanguageManager $language_manager) {
+  public function __construct(UrlGeneratorInterface $url_generator, CacheBackendInterface $cache, LanguageManagerInterface $language_manager) {

Status:Needs review» Needs work

The last submitted patch, 60: url_generator-1965074-60.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new24.55 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,847 pass(es), 0 fail(s), and 36 exception(s).
[ View ]
new899 bytes

Recent patches reintroduced assertions that HEAD removed ...

LanguageListTest should pass now this has been resolved.

I am still trying to track down those pesky exceptions ... they all seem to have a common form.

Status:Needs review» Needs work

The last submitted patch, 62: url_generator-1965074-62.patch, failed testing.

+++ b/core/core.services.yml
@@ -307,12 +314,15 @@ services:
+  url_generator:
+    class: Drupal\Core\Routing\CachedUrlGenerator
+    arguments: ['@url_generator.uncached', '@cache.url_generator', '@language_manager']

I think it would make sense to call this "url_generator.cached" and then have "url_generator" simply be an alias for the former. That makes it easier to swap from cached to uncached.

StatusFileSize
new24.61 KB
new634 bytes

#64 .. This is how it looks

  url_generator.uncached:
    class: Drupal\Core\Routing\UrlGenerator
    arguments: ['@router.route_provider', '@path_processor_manager', '@route_processor_manager', '@config.factory', '@settings']
    calls:
      - [setRequest, ['@?request']]
      - [setContext, ['@?router.request_context']]
  url_generator.cached:
    class: Drupal\Core\Routing\CachedUrlGenerator
    arguments: ['@url_generator.uncached', '@cache.url_generator', '@language_manager']
  url_generator:
    alias: url_generator.cached

looking at the other uses of alias in HEAD, and looking for conformity. The obvious set of names would be

url_generator
url_generator.cached,
url_generator.active

but I am happy to go with tstoeckler's suggestion as that way we hide a implementation details away from other
devs.

Status:Needs work» Needs review
StatusFileSize
new1.42 KB
new24.57 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,042 pass(es), 84 fail(s), and 72 exception(s).
[ View ]

Still hunting those 36 exceptions ... but in the mean time I have found 2 nitpicks.

Status:Needs review» Needs work

The last submitted patch, 66: url_generator-1965074-66.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new918 bytes
new24.53 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,129 pass(es), 0 fail(s), and 35 exception(s).
[ View ]

That blew up because I did not subject #66 to the testbot..... and there was a problem with my aliasing.

I retried to get the alias to work but failed

using http://symfony.com/doc/current/components/dependency_injection/advanced.... as a reference I tried the alternate form of

url_generator:
  alias: '@url_generator'

but they introduced a different class of problem..
Unnervingly I tried swapping the order of url_generator and url_generator.uncached service definition in core.services.yml which if I understand the multiple pass dependency resolution nature of setting up services should make no difference.. and yet the error messages changed. [ and yes I was clearing the cache :-) ]

I tried for extra protection making the url_generator.uncached. private, so no application code can make use of it but no success..

So in the end while muttering under my breath about alpha level software I concluded that all this was out of the scope of this issues and backout the alias changes in
#65

PS making 'url_generator.uncached' private seems sensible.. so that remained.

Status:Needs review» Needs work

The last submitted patch, 68: url_generator-1965074-68.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new24.54 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,328 pass(es), 0 fail(s), and 3 exception(s).
[ View ]
new786 bytes

Removed some exceptions.

Status:Needs review» Needs work

The last submitted patch, 70: url_generator-1965074-70.patch, failed testing.