PathAliasManager could not find the alias by the internal path

Confirmed that this issue is returning the wrong internal path instead of its alias, even thought its alias is present. Also confirmed that using array_diff_key is incorrect and should be array_diff.

Applying the provided patch fixes it with no other side effect.

This needs regression test coverage.

Adding a test that should fail with current implementation that is wrongly using array_diff_key instead of array_diff (please note that this test failing is the expected behavior).

Current test for Drupal\Tests\path_alias\Unit\AliasManagerTest::testGetAliasByPathCachedMatch() searches for a given path inside an array of cached paths. That cached array only contains one item (and the path we are searching for is in the first position), so array_diff_key works by simple coincidence of positions in the array.

This patch modifies that test so the cached array contains more that one item, and the path we are searching for is NOT in the first position. This makes array_diff_key fails because it is wrongly using the array key instead of its value.

If this patch fails (the expected behavior), I'll send another patch with both the test and its fix, to confirm the problem is fixed.

As expected, test from #8 failed, so I am posting again the same test along its fix. This patch is expected to pass as it fixes the problem.

Regression test coverage added.

Making issue unassigned so it can be reviewed.

We also experienced this issue, and I can confirm that the proposed change fixes the issue in our case.

This patch fixes another set of symptoms. I confirm this on Drupal 10.0.1. Here are the steps to reproduce the problem that the patch in #9 fixes:

1. Install Drupal 10 on Simplytest.me with standard profile
2. Log in as user "admin" with password "admin"
3. Create a new basic page at /node/add/page.
4. Enter text for the title.
5. Set to published (in "Save as" field).
6. Do not set a URL alias.
7. Save.
8. Notice that you end up at URL path /node/1.
9. Edit this node.
10. This time, do set a URL alias, e.g., "/alias-test".
11. Save.
12. Notice that you end up at URL path /alias-test.
13. Edit this node again.
14. Change nothing.
15. Save.
16. Repeat the above steps (#3 - #15) to create a second node (with a different alias).

After updating the second node for the second time, notice that you land on URL path /node/2. This seems to be a bug. You should have been sent to this node's alias. In fact, the "View" tab links to the aliased path.

The problem seems to be caused by the un-aliased paths cache in \Drupal\path_alias\AliasManager::getAliasByPath.

However, the patch in comment #9 fixes this on Drupal 10.

I've retested patches from #8 and #9 with Drupal 9.5, 10 and 10.1. All patches work as expected (#8 should fail, while #9 should pass).

Hence, I'm marking this as RTBC thanks to feedback from #13 and #15

@albertosilva, in general, you should not RTBC your own patch. While others' manual testing can help confirm your fix is valid, it is not the same as a code review of the patch.

I suggest checking out the #needs-review-queue channel in the Drupal community Slack if you have an issue that's been waiting for a code review for a very long time.

So we are essentially using array_diff_key() when only array_diff() should be used. This is one of those issues that makes me ask, "Why is it this way in the first place?" Are the array keys important? Were they ever? So, I checked.

Finding out why array_diff_key() was ever used.

[ayrton:drupal | Sun 17:10:25] $ git log -L224,225:core/modules/path_alias/src/AliasManager.php
commit 474a6902993ebc200cdda6b8c717ea4bb7368cf4
Author: catch <catch@35733.no-reply.drupal.org>
Date:   Mon Jan 27 15:46:24 2020 +0000

    Issue #3092090 by Berdir, Wim Leers, plach, amateescu: Remove legacy Path Alias subsystem

diff --git a/core/modules/path_alias/src/AliasManager.php b/core/modules/path_alias/src/AliasManager.php
--- a/core/modules/path_alias/src/AliasManager.php
+++ b/core/modules/path_alias/src/AliasManager.php
@@ -10,1 +224,2 @@
-class AliasManager extends CoreAliasManager implements AliasManagerInterface {}
+        $this->noAlias[$langcode] = array_flip(array_diff_key($this->preloadedPathLookups[$langcode], array_keys($this->lookupMap[$langcode])));
+      }

This led me to:
#3092090: Remove legacy Path Alias subsystem

In that issue, the key line is just moved from one namespace to another. So, back further we go:

[ayrton:drupal | Sun 17:15:42] $ git checkout 474a690299^

[ayrton:drupal | Sun 17:17:33] $ git log -L267,268:core/lib/Drupal/Core/Path/AliasManager.php
commit 09702f841389ab2c4e93062d621fb40f1c3959f0
Author: Nathaniel Catchpole <catch@35733.no-reply.drupal.org>
Date:   Wed May 28 10:44:50 2014 +0100

    Issue #2233623 by Berdir, slashrsm, xjm: Fixed Merge AliasManagerCacheDecorator into AliasManager.

diff --git a/core/lib/Drupal/Core/Path/AliasManager.php b/core/lib/Drupal/Core/Path/AliasManager.php
--- a/core/lib/Drupal/Core/Path/AliasManager.php
+++ b/core/lib/Drupal/Core/Path/AliasManager.php
@@ -147,2 +222,2 @@
         // Keep a record of paths with no alias to avoid querying twice.
-        $this->noAlias[$langcode] = array_flip(array_diff_key($this->preloadedPathLookups, array_keys($this->lookupMap[$langcode])));
+        $this->noAlias[$langcode] = array_flip(array_diff_key($this->preloadedPathLookups[$langcode], array_keys($this->lookupMap[$langcode])));

commit 1299c3afcf6529e31e8f818978459aa2191f67a6
Author: Alex Pott <alex.a.pott@googlemail.com>
Date:   Mon May 5 16:14:38 2014 +0100

    Issue #2233619 by slashrsm, Jalandhar: Merge lookup functions in AliasManager.

diff --git a/core/lib/Drupal/Core/Path/AliasManager.php b/core/lib/Drupal/Core/Path/AliasManager.php
--- a/core/lib/Drupal/Core/Path/AliasManager.php
+++ b/core/lib/Drupal/Core/Path/AliasManager.php
@@ -120,1 +147,2 @@
-    return $result;
+        // Keep a record of paths with no alias to avoid querying twice.
+        $this->noAlias[$langcode] = array_flip(array_diff_key($this->preloadedPathLookups, array_keys($this->lookupMap[$langcode])));

Rinse and repeat until I finally get back to:

commit 61f4dfc982fc6c1c20377691e2adfb3e4c5f68f9
Author: Dries Buytaert <dries@buytaert.net>
Date:   Sat May 16 19:07:02 2009 +0000

    - Patch #456824 by catch: add better caching to drupal_lookup_path().

diff --git a/includes/path.inc b/includes/path.inc
--- a/includes/path.inc
+++ b/includes/path.inc
@@ -67,0 +88,2 @@
+          // Keep a record of paths with no alias to avoid querying twice.
+          $no_aliases[$path_language] = array_flip(array_diff_key($system_paths, array_keys($map[$path_language])));

In that original issue, the first prototype had:

+  $cids = array_keys(array_diff_key(array_flip($cids), $cache));

...and it sort of kept on in the issue from there.

Caching with $system_paths later introduced and was set to the cache with:

+    if ($paths = current($map)) {
+      $data = array_keys($paths);
+      $expire = REQUEST_TIME + (60 * 60 * 24);
+      cache_set($cid, $data, 'cache_path', $expire);
+    }

So they both had integer keys at that point, and it continued to mostly work by accident

Also make note of comment #36 on that issue:

Drupal 6 port. I had to change array_diff_key into array_diff though?

:)

Back to 10.1.x

TLDR, the issue summary explains why this is wrong; we want to compare path values to path values regardless of integer index keys. The IS explains why it's definitely a bug now, and the test coverage proves it.

I could mark this RTBC based on my own review, but then I couldn't commit it, so reaching out to the #need-review-queue channel for a peer code review.

Tagging for a followup to better document the array structures of the various protected properties in the class.

This issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge require as a guide.

Ran just the test case from #9 without the fix and got

[docker-compose://[/Users/smustgrave/Sites/Drupal-Demos/drupal-10.x/.ddev/.ddev-docker-compose-full.yaml]:web/]:php8.1 /var/www/html/vendor/phpunit/phpunit/phpunit --configuration /var/www/html/phpunit.xml --filter "/(Drupal\\Tests\\path_alias\\Unit\\AliasManagerTest::testGetAliasByPathCachedMatch)( .*)?$/" --test-suffix AliasManagerTest.php /var/www/html/web/core/modules/path_alias/tests/src/Unit --teamcity
Testing started at 7:45 PM ...
PHPUnit 9.5.26 by Sebastian Bergmann and contributors.

Warning:       Your XML configuration validates against a deprecated schema.
Suggestion:    Migrate your XML configuration using "--migrate-configuration"!

Testing /var/www/html/web/core/modules/path_alias/tests/src/Unit

Failed asserting that two strings are equal.
Expected :'iRgKYMeq'
Actual   :'/um0hyKhY/pw76XM0L'
<Click to see difference>

All green with the fix as we can see.

Thanks to @xjm for deep investigative work it appears changing this is the correct solution all along.

Opened https://www.drupal.org/project/drupal/issues/3331889 as the follow up

Oh! One very small thing I was reminded of based on @smustgrave's test result there.

+++ b/core/modules/path_alias/tests/src/Unit/AliasManagerTest.php
@@ -259,7 +259,14 @@ public function testGetAliasByPathCachedMatch() {
+      $language->getId() => [
+        $this->randomMachineName(),
+        $path,
+      ],

Instead of randomMachineName(), we should use a static value. There's a small chance the random machine name could end up being the same as the path if other parts of the test are refactored, or it could lead to other regressions. I'd suggest something self-explanatory like:
/another/path

@xjm thanks for the investigative work, and sorry for marking it RTBC. The original patch was provided by @pauger, to which I later added the missing test. My intent was to validate @pauger's solution, not mine. Lesson learned.

I've fixed the test, removing the randomMachineName you mentioned, replacing it with /another/path

Moving back to RTBC with the update.

Just a note @albertosilva for future reference, you should always provide an interdiff when you upload a changed patch to an issue:
https://www.drupal.org/docs/develop/git/using-git-to-contribute-to-drupa...
(Only necessary when using patches, not with merge requests.)

In this case the patch is small enough that I can easily see what was changed, so I went ahead with reviewing it anyway. :) Committed to 10.1.x, and cherry-picked to 10.0.x and 9.5.x as a patch-eligible, non-disruptive bugfix. Thanks!

Removing duplicate tag.

Code makes sense and archeology from xjm helps understand that this has always been a bug.

confused thought I did open the follow up?

@smustgrave You did; @larowlan and I just had a bit of a cross-post-a-thon. :)