Merge AliasManagerCacheDecorator into AliasManager

1. +++ b/core/core.services.yml
   @@ -375,9 +375,6 @@ services:
   -  path.alias_manager.cached:
   -    class: Drupal\Core\CacheDecorator\AliasManagerCacheDecorator
   -    arguments: ['@path.alias_manager', '@cache.data']
   

   +1 as you can easily forget to use the cached version.

2. +++ b/core/lib/Drupal/Core/Path/AliasManager.php
   @@ -80,11 +103,39 @@ class AliasManager implements AliasManagerInterface {
   +  public function writeCache() {
   

   It is a bit odd that writeCache is part of the public API but yeah this is not the fault of the patch itself.

3. +++ b/core/lib/Drupal/Core/Path/AliasManager.php
   @@ -202,26 +268,19 @@ public function getPathLookups() {
   +   * @param $path
   

   If we change this line we can use @param string directly. In general this could be seen as somehow out of scope.

My comment sounded maybe a bit pessimistic, but I really like this refactoring.

Noticed during profiling that something is going wrong right now with alias caching and languages.

Looks like the AliasManager right now can only write back caches of a single language, the first in the lookup map?

In my case, I was importing nodes with langcode not specificed (und), they now explicitly pass along the language context. The result is that they remain uncached. I was displayed 100 of them in a view and I have 100 queries due to that right now.

And even after changing them to en, caching still doesn't seem to be working, because it never ends up in the cache... this seems to be because there are no aliases, so they're added to noAlias, which isn't added to the cache.

This might not be the correct issue, but with all the refactoring, we could possibly fix at least some of the issues here? What about caching the alias hits and misses separately in the cache, so that we don't have to attempt to load the alias for them? Why are we not caching the aliases btw, should be possible to do that by language?

That said, merging this seems to make a lot of sense and it can go even further, the cache write method still uses getPathLookups(), which is a public method on the interface, the only reason for that seems to be that the cache wrapper can do the caching? The method is very limited, because it is not possible to solve the problems above while still using it as it has no understanding of language and if an alias existed. So we can probably just remove that and use $this->lookupMap and $this->noAlias directly in writeCache() ?

Why we cache only paths (as also mentioned by @berdir). It seems that we could cache entire path-alias mapping instead of preloading it fro DB.

Caching only the paths means that it's not necessary to invalidate the cache when path aliases are added/updated. That decision pre-dates having cache tags available so we could maybe look at that again.

Why are we preloading only in getAliasByPath()? Wouldn't also getPathByAlias() benefit from preloaded mappings? - I suspect that we want to prevent preloading SQL queries in case only one "path from alias" is needed (for current url/page).

getAliasByPath() is needed for URL generation, getPathByAlias() is needed only for the incoming request - so there's no real saving on that side.

Haven't reviewed the latest patch yet.

Also looks like this needs more discussion, or a follow-up.

Discussed with berdir in irc, let's fix the caching here. This is completely broken atm so bumping to critical.

Going to try and work a bit on this.

(Title needs a correction too)

Ok, this should be a much better and has a ton of unit test coverage :)

Relevant changes:

- cacheNeedsWriting defaults to FALSE, no need to set it to FALSE then when loading from cache.
- Killed getPathLookups(), it is useless now (it was only necessary when the caching was in the decorator) and it was way too limit for the other improvements
- path lookup cache is now keyed by language and also includes noAlias misses.. the logic was there in preload but they were not cached :(

Unit tests can possibly be refactored a bit, I focussed on covering all scenarios that I could think of and not to optimize lines of code ;)

Did also manual testing of a page of nodes in 3 different languages, everything went through preload, with and without aliases.

I think we can look into caching actual aliases in a follow-up because that will then be an internal implementation detail.

What I still don't like is that whole CacheDecoratorInterface business: It's a weird split of methods, writeCache() is on decorator, clearCache() on aliasManager and has very generic/misleading method names but I can possibly live with it. Don't really see the point in that, but so be it.

Generic method names: setCacheKey() is actually the path, and this should be more like a cache key suffix, because there's no longer a cache_path bin that is only used for this, but the generic cache_data bin. This means that you could access site/some-existing-other-cid and that would then seriously mess up things :) Another bug that needs to be fixed.

What I didn't change is that preloading happens in getPathFromAlias(), the reason is that otherwise we can't expect that the cache key is set as that happens at the same time (Which is *not* checked and another example while the generic naming is problematic).

Yeah, that needs to change, not sure why it worked for me. I'm actually wondering now if I tested it before I moved it back into getPathFromAlias() ;)

Not sure how exactly we want to do that? Move it back and make it conditional on having a cache Key? That would also avoid loading the cache in case no aliases URL's are displayed?

Can you check this? also fixed the cache key prefix problem.

This should fix the test fails.

If you want to be nitpicky, then you could just put that inline now, a separate variable for that seems unnecessary and not what we usually do?

What makes you think I do? (Just care about this getting in) :)

Here's a patch with my suggestion, feel free to RTBC whatever you prefer ;)

Actually, you have to RTBC #29 or re-roll the previous patch, because it didn't apply anymore :p

+++ b/core/lib/Drupal/Core/Path/AliasManager.php
@@ -78,13 +102,49 @@ class AliasManager implements AliasManagerInterface {
+    if ($this->cacheNeedsWriting && !empty($path_lookups) && !empty($this->cacheKey)) {

Couldn't se do the $this->cacheNeedsWritingCheck at the start of the method? No point building the array otherwise.

Looks good apart from this.

Reroll for #2247991: [May 27] Move all module code from …/lib/Drupal/… to …/src/… for PSR-4.

Committed/pushed to 8.x, thanks!

Yay, thanks for this!

Urks, what's up with those variable names...

Wow, why did my unit tests not catch this?

Double-Urks.

:)

Confirmed the tests catch the bug

phpunit --filter AliasManagerTest
PHPUnit 3.7.21 by Sebastian Bergmann.

Configuration read from /Volumes/devdisk/dev/sites/drupal8alt.dev/core/phpunit.xml.dist

....EE.E.EE

Time: 2 seconds, Memory: 69.00Mb

There were 5 errors:

1) Drupal\Tests\Core\AliasManagerTest::testGetAliasByPathNoMatch
Undefined variable: twenty_four_hours

/Volumes/devdisk/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/Path/AliasManager.php:148
/Volumes/devdisk/dev/sites/drupal8alt.dev/core/tests/Drupal/Tests/Core/Path/AliasManagerTest.php:229
/usr/local/php5-5.5.8-20140112-185654/lib/php/PHPUnit/TextUI/Command.php:176
/usr/local/php5-5.5.8-20140112-185654/lib/php/PHPUnit/TextUI/Command.php:129

2) Drupal\Tests\Core\AliasManagerTest::testGetAliasByPathMatch
Undefined variable: twenty_four_hours

/Volumes/devdisk/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/Path/AliasManager.php:148
/Volumes/devdisk/dev/sites/drupal8alt.dev/core/tests/Drupal/Tests/Core/Path/AliasManagerTest.php:267
/usr/local/php5-5.5.8-20140112-185654/lib/php/PHPUnit/TextUI/Command.php:176
/usr/local/php5-5.5.8-20140112-185654/lib/php/PHPUnit/TextUI/Command.php:129

3) Drupal\Tests\Core\AliasManagerTest::testGetAliasByPathCachedMissLanguage
Undefined variable: twenty_four_hours

/Volumes/devdisk/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/Path/AliasManager.php:148
/Volumes/devdisk/dev/sites/drupal8alt.dev/core/tests/Drupal/Tests/Core/Path/AliasManagerTest.php:367
/usr/local/php5-5.5.8-20140112-185654/lib/php/PHPUnit/TextUI/Command.php:176
/usr/local/php5-5.5.8-20140112-185654/lib/php/PHPUnit/TextUI/Command.php:129

4) Drupal\Tests\Core\AliasManagerTest::testGetAliasByPathUncachedMissNoAlias
Undefined variable: twenty_four_hours

/Volumes/devdisk/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/Path/AliasManager.php:148
/Volumes/devdisk/dev/sites/drupal8alt.dev/core/tests/Drupal/Tests/Core/Path/AliasManagerTest.php:468
/usr/local/php5-5.5.8-20140112-185654/lib/php/PHPUnit/TextUI/Command.php:176
/usr/local/php5-5.5.8-20140112-185654/lib/php/PHPUnit/TextUI/Command.php:129

5) Drupal\Tests\Core\AliasManagerTest::testGetAliasByPathUncachedMissWithAlias
Undefined variable: twenty_four_hours

/Volumes/devdisk/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/Path/AliasManager.php:148
/Volumes/devdisk/dev/sites/drupal8alt.dev/core/tests/Drupal/Tests/Core/Path/AliasManagerTest.php:523
/usr/local/php5-5.5.8-20140112-185654/lib/php/PHPUnit/TextUI/Command.php:176
/usr/local/php5-5.5.8-20140112-185654/lib/php/PHPUnit/TextUI/Command.php:129

FAILURES!
Tests: 11, Assertions: 38, Errors: 5.

Extra tests look great, not sure how we wall missed $twenty_four_hours_four_hours.

Committed/pushed to 8.x, thanks!

From the #35 patch committed in #36:

+++ /dev/null
@@ -1,128 +0,0 @@
-  public function getAliasByPath($path, $langcode = NULL) {
-    return $this->aliasManager->getAliasByPath($path, $langcode);
-  }
...
+++ b/core/lib/Drupal/Core/Path/AliasManager.php
@@ -162,12 +237,14 @@ public function getAliasByPath($path, $langcode = NULL) {
     // Try to load alias from storage.
     if ($alias = $this->storage->lookupPathAlias($path, $langcode)) {
       $this->lookupMap[$langcode][$path] = $alias;
+      $this->cacheNeedsWriting = TRUE;
       return $alias;
     }

This does not look like a no-op. Looks to me like this meant that prior to this patch, the decorator's getAliasByPath() didn't trigger a new cache write, but after this patch, we started triggering that. Patch in comment #3 also has this hunk.

#2582219: Preload Cache in AliasManager can get huge is now trying to revert that, so if anyone here has feedback about that, please comment on that issue.