path.alias_repository service does not use a proper language fallback mechanism

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. 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 request → as a guide.

As a bug this will need a test case.

Woah, patches 78 & 79 have taken a completely different approach to all previous patches, with no explanation or interdiff. They reference a service that isn't even in core, but looks like a contrib one (language_fallback.controller)? So we'll need updates about those please (@rzanetti?), otherwise those will have to be ignored, as yes, that approach needs tests (and more).

But returning to the patch in #69, which is otherwise the 'current' one, that one does include tests. Its tests-only patch shows failures as expected, and the fix+tests patch passes tests. That said, I haven't myself had time to review updates since comment 64, so there are outstanding concerns that might need addressing.

Reviewing #69

+      @trigger_error('The language_manager service must be passed to AliasRepository::__construct(), it is required before Drupal 9.1.1. See https://www.drupal.org/node/3108585.', E_USER_DEPRECATED);

1. This doesn't seem like the normal template. But will also need version update.

Tests look good but would need a 2nd pair of eyes.

@smustgrave Fair point. Which version should this issue be targeting? Should it really be Drupal 10.1 by now? Given the pace of this issue, I'm reluctant to spend time changing the version number in that string when it's only likely to need to change again! I've always wondered, how is that handled on other issues, do they just keep updating such strings every time a new release comes along??

Can look at other example in core for the template wording. But it would be required for Drupal 11.

+++ b/core/modules/path_alias/src/AliasRepository.php	(revision da61e77ac1a1354030d2e6c0251553effd47d9cc)
@@ -122,20 +138,29 @@
+    // hook_tag_path_alias_language_fallback_alter().

I think this should be updated to the following:

// hook_query_tag_path_alias_language_fallback_alter().

@james.williams have found two examples for the wording:

@trigger_error("The property $name ($service_name service) is deprecated in $class_name and will be removed before Drupal 11.0.0.", E_USER_DEPRECATED);

@trigger_error('The key-value pair "experimental: true" is deprecated in drupal:10.1.0 and will be removed before drupal:11.0.0. Use the key-value pair "lifecycle: experimental" instead. See https://www.drupal.org/node/3263585 → ', E_USER_DEPRECATED);

Maybe it helps?

Thanks both. Here are updated patches based on #69, taking into account the feedback from #83 (I used an example from \Drupal\migrate\Plugin\migrate\id_map\Sql which was the first similar one that I found) and #86.

I've also updated the change record, targeting 10.2.0 (though I don't know if that's possible).

The Needs Review Queue Bot → tested this issue. We recommend that you convert this patch to a merge request → .

Merge requests are preferred over patches. Be sure to hide the old patch files as well (converting an issue to a merge request without other improvements is not recommended and will not receive credit).

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

bot was mistaken about the actual problem but it's a valid NW

Can anyone help me understand/resolve the Nightwatch test failure https://git.drupalcode.org/issue/drupal-3091336/-/jobs/308229 ? I don't understand how this is something these changes could have triggered.

Have you reran it? Nightwatch has been weird this week.

Oh wow, that was all that was needed. Bizarre. Took two attempts, but there now.

Hiding the patches for clarity now that the fix is in the MR.

Could the issue summary be updated though. I got it started but not as familiar with the issue to be able to speak on it.

And as far as your comment

can well imagine this isn't the "right" thing to do! The changes in PathItem.php replace blocks of code that triggered these messages, reducing them in total, ...

I don't think that's problem. That property is already there and being used, you're just using it again.

All threads have been resolved and issue summary updated to match what's being proposed. Going to mark it but could the MR 5332 be updated to point to 11.x, that's the current development branch and all code has to land there first. Then to be backported. If a separate MR is opened for 11.x can you make note of what is what in the issue summary

Thanks! OK, so branch 3091336-alias-repo-service-fallback-d11 / MR !5373 is for the 11.x branch. I've cherry-picked the 3 commits from the already-reviewed MR !5332 (branch 3091336-path.aliasrepository-service-does), which is for the 10.2.x branch.

The branch 3091336-path.aliasrepository-service-fallback-d11x can be ignored, that was a mistake, sorry. (Can incorrect branches be hidden/deleted?)

Does the addition of the language manager argument for the alias repository need to be made required now, for that 11.x version? Or is that best done as a follow-up?

11.x is the development branch, think of it as the main. Future D10 releases, 10.3 for example, will be tagged off that.

xjm → closed merge request !5332

Closed the old MR.

Posted suggestions for miscellaneous coding standards issues on the merge request. Thanks!

Thanks for that! I've committed fixes for all those bits of feedback.

Appears all threads have been addressed from @xjm

@smustgrave This is one of those cases where it'd be really helpful for you to do your own careful re-review rather than auto-RTBCing when "all committer feedback is addressed".

I feel weird committing this since I made so many suggestions. It got a bit out of hand. 🙂 I probably should have just given the high-level feedback of "remember your void typehints and 'the'". So another set of eyes reviewing not only that my changes were added, but that each was a correct change and made sense, would be really useful.

Setting back to NR to try to get some close review of my suggestions and @james.williams' fixes and that it all made sense, since no other committers have taken this up for commit in the past three weeks either.

Left a few small changes.

I requested push access but still don't seem to be able to run the test only feature. @James.williams do you have the ability to so we can all see. Else I can run locally.

Thanks; I've addressed remaining feedback where I can.

What should I do about the Functional Javascript test failure, which is reporting that (at least) one additional database query has been performed in Drupal\Tests\standard\FunctionalJavascript\StandardPerformanceTest::testAnonymous ? Is there any way to get reports/assets from Drupal CI to see which is the extra query, or does it have to be investigated locally? I had a go at trying to just spot the extra query from just tracing the code statically by eye, but I couldn't find it that way.

As for the test-only failure, I've triggered the 'Test-only changes' item under the 'DEFAULT: PHP 8.2 MySQL 8' job - but should I be using the 'DEFAULT: Test-only (PHP 8.2 MySQL 8)' test job itself? They might well be triggering the same thing; I can't tell!

A rebase should be all that is needed to fix the counts.

The rebase didn't fix the counts. I've spent several hours setting up chromedriver etc to be able to run the functional javascript tests locally, and found it's because the 'path_alias_language_fallback' tag is added to the alias lookup query, when looking up the incoming path. So hook_query_TAG_alter() is invoked, which includes querying the 'cache_bootstrap' database table for the 'module_implements' and 'hook_info' cache entries, i.e. which modules implement which hooks, at the point of early bootstrapping. To me, that seems a reasonable addition, as that cache entry will almost certainly need looking up at some point for users on other common pages anyway? I could be wrong. (FWIW, Berdir had done some performance testing up in comment #53 and found the performance hit was very small.)

On the belief that this is OK to do, I'll increase the count of allowed queries to accommodate this fix.

I've now avoided the need for monolingual sites to perform the extra queries, and run the test-only job. Properly ready for review again!

There were 2 failures:
1) Drupal\Tests\path_alias\Kernel\AliasLanguageFallbackTest::testLookupByAlias
The Afrikaans path alias is returned if we specify English to the alias repository and Afrikaans is a valid fallback candidate for English.
Failed asserting that null matches expected '/user/1'.
/builds/issue/drupal-3091336/vendor/phpunit/phpunit/src/Framework/Constraint/Equality/IsEqual.php:94
/builds/issue/drupal-3091336/core/modules/path_alias/tests/src/Kernel/AliasLanguageFallbackTest.php:71
/builds/issue/drupal-3091336/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
2) Drupal\Tests\path_alias\Kernel\AliasLanguageFallbackTest::testLookupBySystemPath
The Afrikaans path alias is returned if we specify English to the alias repository and Afrikaans is a valid fallback candidate for English.
Failed asserting that null matches expected '/test-login-alias'.
/builds/issue/drupal-3091336/vendor/phpunit/phpunit/src/Framework/Constraint/Equality/IsEqual.php:94
/builds/issue/drupal-3091336/core/modules/path_alias/tests/src/Kernel/AliasLanguageFallbackTest.php:103
/builds/issue/drupal-3091336/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
FAILURES!
Tests: 2, Assertions: 8, Failures: 2.

Thanks for running test-only job. Not sure why push access broke on this single issue.

Reading the threads believe all feedback has been addressed.

mei2020 → changed the visibility of the branch 3091336-path.aliasrepository-service-does to active.

I'm triaging RTBC issues → . I read the IS, the comment, the MR and the CR. I didn't find any unanswered questions.

@james.williams, thank you for getting this issue to RTBC. I found your comments were always helpful and explained the changes and referred to the review that requested a change.

Starting with the CR, that needs to be updated, including the title. The CR should include before and after code samples. The title is to be a statement of the change without the 'should', this is a new requirement. Searching existing change records provides some examples that should help; https://www.drupal.org/node/3387233 → , https://www.drupal.org/node/3014688 → . I've added the tag for change record updates.

I read the CR, not a code review, and I didn't see anything that I thought should change.

I think all the core gates have been completed here.

Finally, let's improve the title of this issue so it says something about the fix, not the problem. Also, what is a 'proper' mechanism anyway? :-) Maybe, "Ensure path alias repository returns the correct fallback language'?

So, just a few things.

OK, thanks @quietone - I've updated this issue's title, and the CR: https://www.drupal.org/node/3108585 →

"what is a 'proper' mechanism anyway?" - haha, fair question! I think just supporting a sane way of altering language fallbacks: via the existing hook_language_fallback_candidates_alter() hook (which the usual entity repository service respects, but not path.alias_repository service, until this change).

james.williams → changed the visibility of the branch 3091336-path.aliasrepository-service-does to hidden.

james.williams → changed the visibility of the branch 3091336-path.aliasrepository-service-fallback-d11x to hidden.

CR and title read well.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Rebased with 11.x by resolving the conflicts.

MR at #123 seems fine but as on comparison we can see a lot of changes at the MR
Moving the issue to needs review as its previous status was RTBC over #120

Let's wait for some more reviews and then move this to RTBC

RTBC+ From my side

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Fixed the tests... changes to the baseline for phpstan had been missed from the rebase, and the extra assertion about the number of queries on user pages was no longer helpful (presumably work on core elsewhere had reduced the number of queries, which is a good sign!), so I removed it.

restoring status from before

One comment on the MR, but also the issue summary seems very out of date with what's going on in the MR - it's changing the form handling and the changes to alias repository are different to what's in the issue summary too.

Thanks; I've now updated the issue summary.

@catch I'm not sure what you meant about form handling, as there's no mention of forms in the MR - but I'm guessing perhaps you were referring to the changes in the PathItem class, so I've added a specific mention of them in the proposed resolution. Otherwise, I'm not all that sure what you feel is specifically wrong with the issue summary? Of course I can well believe there's something needing more explanation!

Resolved that last thread, moving the special handling for path aliases into the path_alias module.

(And for any language_hierarchy users, I've pushed a fix to respect that to its 2.x branch.)

:crossed_fingers:

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Resolved issues in MR (resolved merge conflict + added strict typing for new class files). Tests pass, and tests-only job fails as expected. Back to needs review; everything should be answered. Really looking forward to this finally getting over the last hurdle(s). Core development is one big exercise in persistance!

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Updated MR to be mergeable again. And of course some coding standards have tightened since the last update, so added a couple of trailing commas accordingly. So many hurdles, many of which are purely down to the amount of time this has sat waiting :sob: Very much looking forward to getting this one over the line eventually, I hope.

Though of course I realise we're now at beta for 11.0... so does that mean we now need to adjust the change record & deprecation to target 11.1 & 12 instead of 10.3 & 11? :despair:

Left some small nitpicky stuff. But deprecation believe needs to be updated + the associated change record.

I've resolved each of those and updated the change record.

I feel like there should really be a better process for chasing deprecation versions for long-running issues like this. All too often here it's been so long between reviews (I'm not complaining about that btw), and so having to manually update the versions just to chase that game is incredibly tedious :( Are there any efforts anywhere for improving that process perhaps?

Patches don't apply work on 10.3.x

Hi @google01,

Merge request definitely applies to 10.3.x.

The contents of this: https://git.drupalcode.org/project/drupal/-/merge_requests/5373.diff
Should be downloaded and placed in a local folder (e.g. YOUR_PROJECT/patches/core/3091336.patch), make sure there's an empty line at the end, and then add `patches/core/3091336.patch` in your composer patches list.

Left a comment on MR. Sorry for taking so long.

I have left a suggestion so that someone with write permission can apply it directly to MR.

Re-reviewed from before and believe all feedback has been addressed

Couple of comments on the MR.

Thanks @catch ... ready for review again... :doubly_crossed_fingers: