LH limits to 1 alias when preloading multiple aliases

I can further confirm that I'm seeing this issue as outlined in the OP and I'm also seeing that this fix introduces the problem as described in #4, none of the child aliases get used, the parents get used instead.

Very tricky!

I wonder if we have to go back to back to overriding the path alias repository or something like that.

I think we can actually improve the implementation of language_hierarchy_query_path_alias_language_fallback_alter to do something like:

1. Detect the order of the base_table.idfield on the query
2. Remove the langcode ordering
3. Add the priority ordering using the order from step 1
4. Do not limit the number of results to 1

I think that'll work in all cases, but yeah, this needs some tests too.

I've pushed some code to the MR

I'm not sure if this is the correct place to report this, but in my project this 'hook_query_TAG_alter'-hook in LH is never even called?
I bumped into this because I noticed fallback aliases weren't working (D 10.1.5, LH 2.0.0-RC5)

Not sure whether it's a bug, but when debugging the select query object in the AliasRepository, I also can't find the tag in the 'hook_query_TAG_alter'-hook used by LH.

If this should be its own issue, let me know and I'll create one, no worries.

@RandalV you'll need to apply the core patch from 🐛 path.alias_repository service does not use a proper language fallback mechanism Needs work as mentioned on our project page:
https://www.drupal.org/project/language_hierarchy#:~:text=For%20URL%20al... → .

@Steven Jones Ah, yikes.. I completely glanced over that.
So sorry, thanks for the help!

Have added the core question as related so that they are linked.

Now that 🐛 AliasStorage::preloadPathAlias() incorrectly prioritizes und aliases Fixed has been committed to core, I think we should revisit this issue.

I think either the MR or the patch from #2 are sufficient fixes.

Drupal 10.1 + patch from #2 would break, because the sorting would be in the wrong order, so technically the code in the MR is 'better'.

@james.williams do you feel like this still 'needs tests' ? If so, shall we knock it back to 'Needs work'?

It's still true that tests are needed for this. But personally, I don't mind that being a follow-up task if we can be sure that the change fixes the problem. (In the sense that the tests would be worthwhile for future regression tests.)

I agree that we should use MR !8 over the patch from #2. I believe it's been used on a production site successfully for while now? In which case, I'm happy enough to merge the MR now, unless we're likely to make time to add tests shortly?

Yeah, this change fixes the issue.

Let's get the MR in I reckons, and beef up the tests on this hook another time.

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