Support language switcher links that are render arrays, instead of possible TypeError: htmlspecialchars() WSOD?

Created on 1 August 2023, over 1 year ago
Updated 19 August 2024, 3 months ago

Coming from a core issue ✨ Add Exception for TypeError Argument must be String in \Drupal\Component\Utility\Html escape{} Needs work which is not clear enough yet regarding core handling of such issues and by helping a user with a TypeError issue and to find its reasons, I made this finding. Many reports come up on D.O. lately because of the change to strict in PHP handling of Type. From the users report to me:

"After updating to latest module version and a successful user login, a WSOD appears with the following TypeError message on screen.

TypeError: htmlspecialchars(): Argument #1 ($string) must be of type string, array given in htmlspecialchars() (line 432 of core\lib\Drupal\Component\Utility\Html.php)

Making the admin path unmaintainable."

This is not the only issue like this, but I was sure each is caused by another reason. So I started to make some points and brought up some thoughts on this in Slack and had a very helpful chat with @smustgrave who suggested to print the array content in the moment the error gets initiated. So I did the following for the users case: Changing the respective code in the spot of the error message from

public static function escape($text): string {
  if (is_null($text)) {
    @trigger_error('Passing NULL to ' . __METHOD__ . ' is deprecated in drupal:9.5.0 and will trigger a PHP error from drupal:11.0.0. Pass a string instead. See https://www.drupal.org/node/3318826', E_USER_DEPRECATED);
  return '';
  }
  return htmlspecialchars($text, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8');
}

to

public static function escape($text): string {
  if (is_array($text)) {
    var_dump($text);
  }
  if (is_null($text)) {
    @trigger_error('Passing NULL to ' . __METHOD__ . ' is deprecated in drupal:9.5.0 and will trigger a PHP error from drupal:11.0.0. Pass a string instead. See https://www.drupal.org/node/3318826', E_USER_DEPRECATED);
  return '';
  }
  return htmlspecialchars($text, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8');
}

to print a var_dump from an array which actually should be string here. Which leads to the following output in place of the admin menu section.

/var/www/html/dp/web/core/lib/Drupal/Component/Utility/Html.php:431:
 
  'flag' => 
     
      '#theme' => string 'flags' 
      '#code' => string 'de' 
      '#source' => string 'language' 
  'title' => 
     
      '#markup' => string 'DE' 
/var/www/html/dp/web/core/lib/Drupal/Component/Utility/Html.php:431:
 
  'flag' => 
     
      '#theme' => string 'flags' 
      '#code' => string 'de' 
      '#source' => string 'language' 
  'title' => 
     
      '#markup' => string 'DE' 

Since the user runs an English/German multilanguage site with admin toolbar language switch enabled it was obvious which module is causing this. Disabling and uninstalling the module makes the error disappear.

Hope it helps others and maintainers to improve the code to fulfill PHP's new strict handling of var Types.

✨ Feature request
Status

Needs work

Version

2.0

Component

Code

Created by

πŸ‡«πŸ‡·France dqd London | N.Y.C | Paris | Hamburg | Berlin

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @dqd
  • πŸ‡«πŸ‡·France dqd London | N.Y.C | Paris | Hamburg | Berlin
  • πŸ‡«πŸ‡·France dqd London | N.Y.C | Paris | Hamburg | Berlin
  • πŸ‡«πŸ‡·France dqd London | N.Y.C | Paris | Hamburg | Berlin
  • πŸ‡«πŸ‡·France dqd London | N.Y.C | Paris | Hamburg | Berlin
  • πŸ‡«πŸ‡·France dqd London | N.Y.C | Paris | Hamburg | Berlin
  • πŸ‡«πŸ‡·France Chris64 France

    @diqidoq, could you provide an associated stack trace?

  • πŸ‡¬πŸ‡§United Kingdom james.williams

    This module currently sets the 'title' properties in the render array to a string / TranslatableMarkup, whereas the posted stack trace shows it has been set to an array (containing a #markup), alongside a 'flag' render array (using the 'flags' theme hook).

    But there are no mentions of 'flag' or 'flags' in the toolbar_language_switcher codebase (2.0.x-dev at least); are you running any other modules that could be adding these? Perhaps this module is currently incompatible with one you're using, because it assumes the language names would be strings rather than render arrays? (Which is why turning this module off would fix your problem of course.) If we can establish which module is conflicting, perhaps we can consider building support for language switcher links that are render arrays rather than simple strings. I suppose \Drupal\Core\Language\LanguageManagerInterface::getLanguageSwitchLinks() never actually promises what will be inside the array of links.

    Adjusting priority to normal, since this will only affect sites using the combination of conflicting modules, which is not possible with core and the most common contrib modules. (At least until we can be shown otherwise!)

  • πŸ‡«πŸ‡·France Chris64 France

    In #7 stack trace means the functions list. With a WSOD such a list may be in the log.

  • Status changed to Postponed: needs info about 1 year ago
  • πŸ‡ΊπŸ‡¦Ukraine voleger Ukraine, Rivne

    Setting the appropriate status of the issue.

  • πŸ‡ΊπŸ‡¦Ukraine Yurii Novak

    The error occurs when using the "languageicons" module.

  • Status changed to Needs work 3 months ago
  • πŸ‡¬πŸ‡§United Kingdom james.williams

    Ah great! Thanks for the specific use case. That patch would solve the WSOD, but presumably replaces whatever the languageicons module wanted to do? Perhaps we can catch render arrays and allow them to be rendered, instead? But I wonder what that would look like, given what languageicons wants to set it to. Render arrays could quite easily produce markup that is simply far too big to fit inside the toolbar item.

    Changing to a feature request, as this is really about integrating with other modules that change the type of input data to something unexpected.

  • πŸ‡«πŸ‡·France dqd London | N.Y.C | Paris | Hamburg | Berlin

    #8 & #12 Yes and no. It does not hijack the issue but it somewhat turns the issue around in a way. But if it helps to see what needs to be done, then I leave it up to this view angle and label on it. Agreed with FR. But just let me add the question: Which default would break less? The chance is high that menu links turn into render arrays for so many reasons that I think it does not need to be listed here. I am even not sure if the new Drupal 11 core navigation module renders its links this way. Oh and not to mention superfish, link badges, and what not sure to be come. But never mind, I actually came from an core issue regarding the problem of raising TypeError issues all over the places, possibly most of them based on newer PHP versions getting more strict on this. And that we maybe need more handling of this in many code bases. Especially in cases when the type can change. Which seem to the issue on many spots. And this is exactly what my original issue summary brings up in a way, considered that I am not a native tongue. And well, actually your title change covers it too. But maybe it should be changed one more time into something like:

    "Support language switcher links that can vary in type of string or render array"

    so that both scenarios are covered.

  • πŸ‡«πŸ‡·France dqd London | N.Y.C | Paris | Hamburg | Berlin

    No idea what happened with the last code block in the issue summary back in the days ... changed it accordingly for readability and left out the last assumption since the scope if the issue maybe changes here. In case I have time to come back on this I probably would start with tests of different typical multi language Drupal setups to see if and how often the type changes then.

  • πŸ‡¬πŸ‡§United Kingdom james.williams

    Thanks for the additional detail, and yes, I think you've put that very well. Let's ensure the module doesn't run into any kind of error. Whether that means just showing the language name as in comment 11 (as a bug fix), or supporting render arrays (as a feature request), I accept that either would be an improvement. And it doesn't really matter what kind of fix this is; I'd just like it to be a good one :)

    Perhaps we could support render arrays and see how those from the languageicons module look. Maybe even with something like a max-height / overflow: visible rule to avoid seriously breaking the toolbar appearance if any other modules try to render something especially large?

    If none of us get time to work on supporting render arrays any time soon, I'll probably accept the patch from comment 11 that just replaces arrays with the language name.

  • πŸ‡¬πŸ‡§United Kingdom james.williams

    OK, I've opened MR !6 to handle render arrays. I've tested with and without the languageicons module; I believe it works quite nicely! I do love seeing all the little flags in my toolbar now :-)

    Please take a look and review/test if you can; thanks!

  • πŸ‡¬πŸ‡§United Kingdom james.williams

    Here's a screenshot showing how the switcher links look with the languageicons module:

Production build 0.71.5 2024