[regression] Inaccessible language switcher links are removed before alternatives can be provided

patch #4 at 3349111 does the job, and it's only one line of code
#3349111-04: [regression] Core language switcher disappeared in Drupal 9.5.5

With that said, I'm not sure which is the better approach.

Without patching core we have no language switcher.

patch #2 does not work for us
Warning: Undefined array key "en" in Drupal\wxt_library\Plugin\Block\LanguageBlock->build() (line 161 of modules/contrib/wxt_library/src/Plugin/Block/LanguageBlock.php).

patch #4 from 3349111 does work for us.
#3349111-04: [regression] Core language switcher disappeared in Drupal 9.5.5

It looks like efforts are focused on #3349111: [regression] Core language switcher disappeared in Drupal 9.5.5.

Let's see what the bot says, the patch seems reasonable

Thanks for filing the issue and for working on a patch!

I have some projects where the home page is not accessible in certain languages, mostly because the language is in the process of being setup by the editors, but not really ready for publication yet. Is it worth checking for such a case in the patch? I haven't yet tested the patch on one of those projects and I haven't done a really thorough review, but just from a quick glance at the patch it looks like this would then return the link to the home page even though it is inaccessible, so it would then be a security issue again, wouldn't it?

Personally, I also think that the new approach introduced in #10 is a step to much, I'd rather have used the original approach in #2. But I can see why some people, maybe even the majority might want such a behavior, so maybe this is indeed the best way to do it for the 80% use case.

I figured that since #3348592: [regression] Language switcher block throws exception when no route is matched used the homepage as a fallback for the scenario where an exception could be thrown, it's quite reasonable to link to the homepage.

That's a good point indeed. Then I think we can keep that.

The key with the security issue was that unpublished pages commonly have a path that contains their page title (e.g. when using pathauto), so putting the link on a page exposes that. The homepage shouldn't have that though, as its path is omitted in the links.

I have actually seen some pages where the home page link now (after the original security patch) links to a path provided by pathauto or even an internal path that has been configured to use as the home page when it linked directly to the front page before. This is not the case on all pages, though, and I didn't yet have time to investigate whether this is caused by the patch or maybe by some custom code and if the former, in what exact configuration. That's why I didn't file an issue for that yet. But that's not of concern here, really.

I would think that you'd need to use hook_language_switch_links_alter() if you want to hide links to the untranslated homepages; but that would have been the case before SA-CORE-2023-003 too?

That's true. I don't actually have a very strong opinion about that, I can add that code I just removed back, that's fine with me ;).

For me the expected behaviour is, that links that are not accessable are not shown on my page. Therefore I think the apporach of the patch is correct. In every menu we have the same behaviour.

The problem is, that we mixed up an API call (getLanguageSwitchLinks()) and the block for languageswitcher.

I think, we need a 3rd parameter for getLanguageSwitchLinks() with checkAccess=TRUE. Then everyone can get all languageswitch-links via API and can write their own code and show what their want.

For the languageswitch-block I think the expected default behaviour is not to show unaccessable links. It could be a solution to add an option "show link to homepage for missing or unaccessable links". Than the sitebuilder can decide what to do with missing links. But to do this by default like in patch #10/16 seems to invasive for me.

To fix this for current installations I would prefer patch #2, so they can change inaccessable links by their own before access check. But in the long way we should differentiate between API-call and block - and they should write their own block.

Your patch is good for everyone who needs this functionality as soon as possible back! Thank you for this, I am sure the patch is a big help for them. But I dont think this should merged in core.

My 2c: I would like the links to disappear as they do in the current core, but only after it has been decided (by the alter hook implementations) that there is no access to them. I'm basing this on 0 evidence, just a personal feeling of not wanting to overrule the decision made by core developers.

After all, an alter hook implementation can switch the inaccessible links into a home page link if it so chooses.

In summary, #2 is the correct approach IMO.

The failed patch attempts to move the filtering of links for access to them after the hook_language_switch_links_alter() has run. It seems the placement of code in the patch might be causing the issue.

Please review.

Thank you for the interest @kunal_sahu. It is expected to include an interdiff between patches and to check the patch before uploading so removing credit for the 2nd part. Also that patch removed test coverage.

@james.williams if the proposed solution changes if you could update the issue summary please.

Still moving to NW as #24 had test failures in the main patch.

Hopefully this one is close!

W/r/t the test fail in #24:

+    $languages = $this->container->get('language_manager')->getNativeLanguages();

In tests, the container property is initialized during setup. In this case, the test method makes changes to the container, which are relevant to our testing, so we can't just call $this->container at that point, because it is outdated. We would have to update the container first, e.g. by calling $this->rebuildContainer();, preferably in the test method since it should be enough to update it once. In this case however I think we know for certain that our expected languages will be English and French, so I think we can skip this call entirely and just assert those.

W/r/t #26:

I have performed a GitLab code search in the project group and looked at various implementations of the hook in contrib. I think we can just add a boolean access key to the $links array for modules to check, if they want to replace inaccessible links.

I made some changes to the issue summary to that effect and updated the patch accordingly. We now check link access before we pass the results to the hook and add an access key for each link. After the hook, we remove the access key and do the actual filtering of the (potentially changed) results.

Alternatively, we could still just fix the test in #24 and go with that. Let me know, what you think.

Sorry, forgot the interdiff...

Okay, test results not as expected. The regression test only should have failed. We need to look into that.

Doh! Looked at this some more and it is again due to the outdated container... Using \Drupal::state() instead of $this->container->get('state') I got the expected results. So I added a call to $this->rebuildContainer() after installation of node module in the test and locally it looks like it works now. Attached is a new set of patches and an interdiff.

+++ b/core/modules/language/src/ConfigurableLanguageManager.php
@@ -408,27 +408,26 @@ public function getLanguageSwitchLinks($type, Url $url) {
+              return $link + [
+                'access' => $this->isLanguageSwitchLinkAccessible($link),
+              ];

We need to update hook documentation to include info about this key.

I'm not convinced about #26 and the need to call isLanguageSwitchLinkAccessible() twice on the array. I think it is up to the implementation of language_switch_links to determine that it is providing accessible links and it should not be called twice here. Otherwise we're imposing an unnecessary performance penalty on everyone. Even sites which do not implement the hook.

I think we should go back to #24.

Thanks for your reviews @james.williams and @alexpott and for the updated patch. I think the interdiff is reversed, otherwise the code looks good. Now waiting for test results before I RTBC this...

In the meantime, I updated the IS once more.

Can I blame it being a Monday morning?

Yes, you can :) Also not as bad as me mentioning the problem with the first use of the container property and then forgetting there is another one further below... Then I even forgot the whole interdiff for the first iteration. And that was all on a Sunday ;).

Test results as expected. I already did the code review and a test run on my dev system while I was waiting for CI to complete. I think this is RTBC!

1. +++ b/core/modules/language/tests/src/Functional/LanguageSwitchingTest.php
   @@ -505,7 +505,8 @@ public function testLanguageSessionSwitchLinks() {
   +    $this->rebuildContainer();
   +    $entity_type_manager = $this->container->get('entity_type.manager');
   
   @@ -612,6 +613,12 @@ protected function assertLinkMarkup(string $path, int $status, string $marker, s
      /**
   

   Why was this change made?

   Using $this->container in functional tests is not recommended

2. +++ b/core/modules/language/tests/src/Functional/LanguageSwitchingTest.php
   @@ -612,6 +613,12 @@ protected function assertLinkMarkup(string $path, int $status, string $marker, s
   +    $languages = $this->container->get('language_manager')->getNativeLanguages();
   +    $links_for_alter = $this->container->get('state')->get('language_test.language_switch_link_ids');
   

   I think this should use \Drupal rather than $this->container

Couple of questions about things I didn't expect to see in the test changes

Thanks for your review @larowlan.

Why was this change made?

Since the test method installs node module, the container property is outdated at that point. Looks like both @james.williams and me thought that using $this->container to access services was the recommended way to access services in tests. That's why we had to rebuild the container after installation of node module, because otherwise the calls in the assert method would not return the expected results. Since we rebuild the container anyway, we could also get the entity type manager from there... If the recommended way is to use \Drupal::service(), we should just change that.

Yeah according to #2066993: Use magic methods to sync container property to \Drupal::getContainer in functional tests at least for functional tests

Thanks for the updated patch @james.williams!

Feedback from #46 addressed, test results as expected => back to RTBC.

Updating issue credits

Committed to 10.1.x and backported to 10.0.x and 9.5.x - thanks all!