AliasStorage::preloadPathAlias() incorrectly prioritizes und aliases

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 🐛 LH limits to 1 alias when preloading multiple aliases Needs review :)

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.

Automatically closed - issue fixed for 2 weeks with no activity.