ConfigurableLanguageManager::getLanguageSwitchLinks() doesn't respect what LanguageManagerInterface::getLanguageSwitchLinks() says to return

@kala4ek
Made the required line from code to comment.
Please review.

The code that uses that method also needs to be changed. For example, LanguageBlock::build() contains the following code.

  $links = $this->languageManager
    ->getLanguageSwitchLinks($type, Url::fromRoute($route_name));
  if (isset($links->links)) {
    $build = [
      '#theme' => 'links__language_block',
      '#links' => $links->links,
      '#attributes' => [
        'class' => [
          "language-switcher-{$links->method_id}",
        ],
      ],
      '#set_active_class' => TRUE,
    ];
  }
  return $build;

It should use $links['links'], not $links->links.

Hm, https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Language%... does actually not specify what format a link has and whether it is an array or an object. The thing is that its implementation returns nothing and nobody uses that, so wondering if we should instead fix the documentation to be more correct and document the case that is actually used. If someone else calls that method, then it will otherwise break, while it is unlikely that someone relies on it being an array as it doesn't actually return anything.

Actually, LanguageManager::getLanguageSwitchLinks() returns an empty array, which is exactly what its documentation says for the returned value.

Return value

array A keyed array of links ready to be themed.

Rather than having code that checks if the returned value is an array, a Boolean value or an object, it would be better to always get an array.

Yes, I meant it doesn't specify what the content of that array is and what exactly a "link" is.

You converted it to an array, bit it's still not a list of links. Its an array with the keys links and method_id. if someone would indeed write generic code based on that documentation it still wouldn't work. So if someone is using the method, then it is based on how ConfigurableLanguageManager actually works not that minimalistic documentation. So instead of changing that implementation, we should probabably fix the documentation and implementation of the base class/interface?

The more I look at the code, the more I am convinced the returned value has been changed to allow the language block to get the value of the language negotiation method ID (which is used for the CSS class of the links). Probably, the code originally returned the value it got from LanguageSwitcherInterface::getLanguageSwitchLinks() directly, which effectively is an array of links indexed by the language ID. (See LanguageNegotiationSession::getLanguageSwitchLinks(), for example.)

I would rather change the code to directly return the array LanguageSwitcherInterface::getLanguageSwitchLinks() gives, but this would require changing the code of the language block too, which would not get the language negotiation method ID anymore.

I will provide a patch that changes the documented return value, and the value returned from LanguageManager::getLanguageSwitchLinks() soon.

Tested #15 on PHP7 with specified return type:

private function getLinks():\stdClass {
    $links = $this->languageManager
      ->getLanguageSwitchLinks(
        LanguageInterface::TYPE_URL,
        Url::fromRoute($this->routeName)
      );
    return !$links ? new \stdClass() : $links;
  }

seems correct, so +1 to RTBC

In \Drupal\language\ConfigurableLanguageManager::getLanguageSwitchLinkswe need to initialise it to NULL now.

   * {@inheritdoc}
   */ 
  public function getLanguageSwitchLinks($type, Url $url) {
    $links = FALSE;

Or maybe preferably because it is easier to rationalise remove the initialisation and do:

 return isset($links) ? $links : NULL;

imo empty array is more compatible, contrib modules using this old format of result

@andypost it's not an old format. It's the language manager. \Drupal\language\LanguageSwitcherInterface::getLanguageSwitchLinks is still an array.

Right now we return NULL in one case and FALSE in another. We need to make it at least consistent.

The reason the previous patch used FALSE is because that already exists as a return value in \Drupal\language\ConfigurableLanguageManager::getLanguageSwitchLinks(). And that is likely the implementation that matters because only then it can ever return anything, so if someone was calling it, they might have a === FALSE check or something.

Of course someone could also be explicitly checking for an empty array, but the implementation is LanguageManager is not really relevant, as there is no reason to call that if the language module is not enabled.

#19 Applied NULL as the empty value for both \Drupal\Core\Language\LanguageManager and \Drupal\language\ConfigurableLanguageManager

#22 From a data point of view NULL is probably more correct. Since #19 didn't specifically comment whether the empty value should be NULL or FALSE, I implemented the smallest possible interdiff using NULL.

Also reworded the @return param of \Drupal\Core\Language\LanguageManagerInterface::getLanguageSwitchLinks() to start with the positive case, since this is the most relevant for developers using it.

Personally I think Drupal using multiple different return types in many places is one of it's biggest weaknesses and should be avoided whenever possible. Especially when dealing with things that are normally iterable but then suddenly are not.

Attached is a patch that changes the default return type to keep the object shape the same for our base LanguageManager and the ConfigurableLanguageManager. I was doubting between having method_id set to an empty string or NULL.

This changes makes it always possible to simply iterate over $links->links making both the calling and producing code simpler.

The patch also adjusts the LanguageBlock implementation because the isset call would now return true with the default LanguageManager's return.

Is the issue here that the docs for LanguageManagerInterface::getLanguageSwitchLinks() are incorrect?

We currently use LanguageManagerInterface::getLanguageSwitchLinks() to retrieve a list of all available language links, which we then reduce to only the ones our site wants available as a menu of interface language links - i.e., we have more content language translations available, than the site wants available as a language switcher menu for languages available in interface elements like main menu, labels etc.

And so we currently rely on the return value as described in #27 - an stdClass, with an array of links and method_id.

Apologies if I've missed something.

Hrm, given this wrong phpdoc and/or wrong code in core, it's not easy to get static analysis to pass in my contrib module :/

Looks like tests are still passing for both #25 and #27

I don't have a strong opinion on what is returned, as long as it's correctly documented :)

I created a draft change record @ https://www.drupal.org/node/3247792 which can be revised if a different approach is taken..

Now rector is happy and do not add anymore property_exists anymore (in my usecase). ;-)

+++ b/core/modules/language/src/ConfigurableLanguageManager.php
@@ -422,7 +420,7 @@ public function getLanguageSwitchLinks($type, Url $url) {
+    return isset($links) ? $links : NULL;

return $links ?? NULL;
We just did #3222251: [November 8, 2021] Replace all isset constructs with the null coalescing operator

Can be set back to rtbc once the change is made.

That's just CS change so RTBC

Hmm I wrote the change record for #27 but it sounds like y'all prefer the approach of #25?

I think having a return type of ?object is okay. The change in #27 has BC issues - this can be seen by the fact it has to change the block that calls the function in question. Changing a FALSE to a NULL is far less risky.

So we need to update the CR based on this to say it either returns NULL or an object. Instead of FALSE, empty array or object!

Once the CR has been fixed this issue can be re-rtbc'd.

Ok change record updated - feel free to tweak it

Oops forgot to remove tag

Composer 2.2.0 issue

Committed and pushed c45aba90e8e to 10.0.x and 4e4f76c944c to 9.4.x. Thanks!