LanguageManager does not check against altered language_switch_links

As a bug it will need a test case to show the issue.

Triggering 11.x also.

When is this patch going to be commited? This bug is crashing websites updated from Drupal 9.5.7 to 9.5.10!
Priority should be higher than Normal...

I'm changing priority to critical as this bug makes WSOD!

I'm not changing status to revieved and tested as i'm not a developer although this patch fixed WSOD.

Confirming #2 worked for me on Core 10.2.3. The switcher was working fine on translation-enabled content that has a translation, but not on translation-enabled content that does NOT have a translation.

The problem also only appeared if you were logged out, not if you were logged in (at least as an admin).

I ran into this bug as well, confirmed that the patch worked for me. I am also using "language switcher extended" module.

Patch #2 is working for me on Core 10.3.1.
I am also using "language switcher extended" module.

Is this a Core responsibility or just a bug in "Language Switcher Extended" module? I'm happily merging a fix for it into the module, if it's handled wrong there.

jan kellermann → made their first commit to this issue’s fork.

@szeidler Here you returns NULL and this triggers the WPOD. Maybe you could try to return [] and filter out the block if link-array is empty later until this issue is solved.

I just created an issue fork and commitet tests and bugfixes for 10.3 and 11.x and created MRs.

1st commit in each branch is the PHP-Unit test which fails (status code 500).

2nd commit in each branch is the fix for this bug (test for type array).

A 2nd test is added in 1st commit to validate that the switch-block is not shown if $result is NULL after alter()-function.

General consideration: The function determines the links for the language switcher from the negotiators. If no links are returned here, the entire block is omitted.
The aim must be to enable this behavior with the alter() function as well. So far, an empty array after the alter() function leads to a language switcher block without links. To hide the block, NULL must be returned. (This must also be permitted in the alter() function; simple typing via array() is then no longer possible - see patches for API file).

The previous patches are based on allowing NULL. Another variant would be that the block is not rendered if the array is empty after the alter() function. This would eliminate the zero setting and also the display of an empty block.

Would that be a more appropriate solution?

Both MRs appear to have failures. Would recommend just sticking with 11.x MR for now.

jan kellermann → changed the visibility of the branch 3362713-10.3 to hidden.

jan kellermann → changed the visibility of the branch 11.x to hidden.

The MR for D11 is fixed.
The MR for D10.3 fails because of #3466822, but the commit should be okay.

But in general see comment 18 🐛 LanguageManager does not check against altered language_switch_links Needs review

I added branch 3362713-11.x for D11 with proposals from comment 18. See interdiff.

I think this is the cleanest way.

Considering the implemented condition I'm assuming that $result is either empty or null or other non-array value and if it is non-array then no use to iterate through array_filter.
I would suggest return false; if non-array value is there would help.

Keeping this in review so that other reviewer have something else in the mind.

jan kellermann → changed the visibility of the branch 3362713-languagemanager-does-not to hidden.

jan kellermann → changed the visibility of the branch 10.3.x to hidden.

I wouldn't recommend opening MR for other branches as it will just make more work to keep up and could get kicked back by the review bot.

If the 11.x doesn't apply to the other branches the committer will mark it for backport. But very seldom do the MRs themselves get merged.

smustgrave → changed the visibility of the branch 3362713-10.4.x to hidden.

smustgrave → changed the visibility of the branch 3362713-10.3.x to hidden.

Left some comments/question on the MR

Two questions are still open in MR that need to be answered by core developers. The other suggestions have been adopted.

Can I support to close the two remaing questions?

• https://git.drupalcode.org/project/drupal/-/merge_requests/9143#note_359008
• https://git.drupalcode.org/project/drupal/-/merge_requests/9143#note_359011

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.

I added the missing declare(strict_types=1); and updated the MR.

The failed tests are not related to this issue.

Perhaps someone can decide the two open questions. I don't see any need for action on either point.

Hello,
Getting error while applying patch.
Attaching error screenshot.

@sagarmohite0031 I updated the fork. Please test again.

Tested on 11.x with the language switcher use case in the initial issue description and functioning as expected.

I started to review this but then decided to make the changes myself. I changed comments to remove the reference to the issue number, as that is not our practice and git can tell use the issue number. I moved the test code block to the end of the test method as was able to remove some clean up line, and the added an very simple example to the hook_language_switch_links_alter.

Setting to review for someone to check those changes.

And Un-assigning per Assigning ownership of a Drupal core issue → .

Left small comments on MR

If you are another contributor eager to jump in, please allow the previous poster at least 48 hours to respond to feedback first, so they have the opportunity to finish what they started!

Added new Hooks Implementation. Please review and feedback.

Unresolved 1 thread for the committer to see.

New API will need a CR also and noticed that section was removed from the issue summary. Wouldn't recommend removing headers from the summary, even if they aren't used

I changed summary and created CR.

Re-unresolving the same thread, want the committers to see

Think this needs a core-committer to decide on open thread, so leaving it open and moving to RTBC as rest seems fine.

I think we should be logging an error when $results is altered to a non array type. Even though that used to work before the security issue fix it is non interoperable with other hook implementation.

Also I think we should change the code to obey the interface and return NULL if the array filter results in $results being empty.

Furthermore I think we need a try {} finally {} here to ensure that $this->negotiatedLanguages is reset regardless of what we do.

Here's how I think the code should look:

  public function getLanguageSwitchLinks($type, Url $url) {
    if ($this->negotiator) {
      foreach ($this->negotiator->getNegotiationMethods($type) as $method_id => $method) {
        if (is_subclass_of($method['class'], LanguageSwitcherInterface::class)) {
          $original_languages = $this->negotiatedLanguages;
          try {
            $result = $this->negotiator->getNegotiationMethodInstance($method_id)
              ->getLanguageSwitchLinks($this->requestStack->getCurrentRequest(), $type, $url);

            if (!empty($result)) {
              // Allow modules to provide translations for specific links.
              $this->moduleHandler->alter('language_switch_links', $result, $type, $url);

              if (!is_array($result)) {
                trigger_error('A language_switch_links hook implementation has changed $result to a non-array.', E_USER_WARNING);
                $result = [];
              }

              $result = array_filter($result, function (array $link): bool {
                $url = $link['url'] ?? NULL;
                $language = $link['language'] ?? NULL;
                if ($language instanceof LanguageInterface) {
                  $this->negotiatedLanguages[LanguageInterface::TYPE_CONTENT] = $language;
                  $this->negotiatedLanguages[LanguageInterface::TYPE_INTERFACE] = $language;
                }
                try {
                  return $url instanceof Url && $url->access();
                } catch (\Exception) {
                  return FALSE;
                }
              });

              if (empty($result)) {
                return NULL;
              }
              $links = (object) ['links' => $result, 'method_id' => $method_id];
              break;
            }
          }
          finally {
            $this->negotiatedLanguages = $original_languages;
          }
        }
      }
    }

    return $links ?? NULL;
  }

I guess we could change trigger_error('A language_switch_links hook implementation has changed $result to a non-array.', E_USER_WARNING); to a proper deprecation and say that in Drupal 12 this will cause an exception when we remove the check...