AliasStorage::preloadPathAlias() incorrectly prioritizes und aliases

In particular, the behaviour now will be unexpected if url_alias.source is the same for two paths, which is a legitimate thing to do on multilingual sites.

This looks good except that calling if (!empty($preloaded)) is redundant after adding empty($preloaded) above for performance.

Fixing status

1. --- a/core/lib/Drupal/Core/Path/AliasStorage.php
   +++ b/core/lib/Drupal/Core/Path/AliasStorage.php
   

   This is modifying AliasStorage.

2. +++ b/core/tests/Drupal/KernelTests/Core/Path/AliasTest.php
   @@ -216,4 +217,67 @@ public function testWhitelist() {
   +  public function testPreloadPathAlias() {
   

   This is expanding the functional test coverage.

3. But the patch is not updating the unit test coverage in \Drupal\KernelTests\Core\Path\AliasStorageTest.

Impressive!

Discussed this at Baltimore. We figured that changing the order does not only change the result order of the specific vs. general alias but also the preferred alias if there are multiple aliases for the same path. So this could result in a backwards incompatible change (other than for the bug being fixed).

The patch on #19 was already merged:

When upgrading from 8.7.6 to 8.8 I got this message:

https://www.drupal.org/files/issues/2745755-19.patch (AliasStorage::preloadPathAlias() incorrectly prioritizes UND aliases; https://www.drupal.org/project/drupal/issues/2745755)
Could not apply patch! Skipping. The error was: Cannot apply patch https://www.drupal.org/files/issues/2745755-19.patch

Reviewing the code it already contains the patch so we don't need this patch in 8.8!!.

@efrainh no, the changes in the latest patch is not already in Drupal core, the patch just doesn't apply any more because the affected files changed. There is now a Drupal\Core\Path\AliasStorage::addLanguageFallback() method in D8.8, which is where the change would be needed instead. In D9.1 though, that class has been almost completely gutted, and the fallback change would need applying to Drupal\path_alias\AliasRepository::addLanguageFallback(). The tests would no doubt need changing up accordingly too.

So I'm adding the re-roll tag too, but I suspect this will not be a trivial re-roll.

As Gábor pointed out, this change might fix a bug, but would be a BC break. So I think it now has to wait for D10 :-/

Coming here from #3135440 where language hierarchy needs to solve basically the same problem/bug that core has, but for multiple languages instead of two.

Moving the target version to 11, and will open a issue fork to work on this.

So as mentioned in #27 the code has moved on a bit, so I've focused on bringing over the tests from #16 that tested out \Drupal\path_alias\AliasRepository::preloadPathAlias.

And then I've implemented a fix.

The real problem here is that preloadPathAlias processes the result set and prefers aliases at the end, whereas all the other methods use the first row in the result set.

This is explained when setting the sort order of the base_table.id field:

    // We order by ID ASC so that fetchAllKeyed() returns the most recently
    // created alias for each source. Subsequent queries using fetchField() must
    // use ID DESC to have the same effect.
    $select->orderBy('base_table.id', 'ASC');

But this change in ordering is never made in addLanguageFallback so I've introduced a badly named parameter that'll reverse the ordering when appropriate.

...and I've just read that I'm supposed to be using patches, unless I explain why I'd like to change over to MRs:

It's been 7 years since the last patch, and the world has moved on, MRs are more/faster/better. Hope that's enough!

There we go, as expected the MR tests pass, but the tests-only version fails.

Thanks for reviving this one. Think we will need a change record for the new parameter, examples are always good too in the CR.

Searching http://grep.xnddx.ru/search?text=addLanguageFallback&filename=

https://www.drupal.org/project/variants or https://www.drupal.org/project/language_hierarchy may be doing something with this. Lightly took a look.

@smustgrave thanks, do you happen to have an example of a CR for documenting adding a new parameter to a protected method?

As for language hierarchy, yes, we're all over this in #3135440: LH limits to 1 alias when preloading multiple aliases :)

This is a pretty basic one https://www.drupal.org/node/3372008

I was writing out the change notice, when I thought that actually this all seems really ugly: that for one method that wants the results in a different order we need to thread this parameter around etc. Seems a lot like tail wagging the dog.
Then I realised that it was only because preloadPathAlias wants to use fetchAllKeyed that we need to do this at all.

If this method does a fraction more work, then everything can be nice and consistent and actually all the queries generated by the \Drupal\path_alias\AliasRepository class can actually have the same order by clauses, meaning that everything is consistent and easier to reason about anyway.

So...I've added that approach to the MR, which now means there's no API change.

I'm also not sure if I agree with @Gábor Hojtsy in #18 that this change is now* a BC break, since, at the moment, depending on your configuration results are simply inconsistent. If preloadPathAlias is called, you'll get one set of aliases, but if you were to call lookupBySystemPath then you might get a different one for the same set of data.
Core has no control over the order and use of these functions really, so I don't think you can consider this weird bug a feature that can't be fixed in a minor or even patch release.

*: Also when @Gábor Hojtsy wrote #18 the patch on this issue was essentially inverting the sort to shift the problem to the methods that only seek to return on alias, so yeah, that would have been the same bug, but a different presentation.

Setting this back to Needs review to get some more feedback on this change of tack.

Not sure I can speak on it but if a new solution is taken the issue summary will need ot be updated to reflect that.

So circled back to this one

1) Drupal\Tests\path_alias\Kernel\AliasTest::testPreloadPathAlias
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
     '/und/src' => '/und/alias'
     '/en/src' => '/en/alias'
-    '/en-und/src' => '/en-und/en'
+    '/en-und/src' => '/en-und/und'
     '/en-xx-lolspeak/src' => '/en-xx-lolspeak/en-dup'
-    '/en-xx-lolspeak-und/src' => '/en-xx-lolspeak-und/en'
+    '/en-xx-lolspeak-und/src' => '/en-xx-lolspeak-und/und'
 )

Test-only feature shows the issue.

Hiding patches as fix is in MR, also highlighted it in the IS.

Looking at the change itself nothing seems glaring so going to mark it.

Committed/pushed to 11.x and cherry-picked to 10.2.x, thanks!

I deleted the change record because it was for a different approach to the final one.