Problem/Motivation

\Drupal\Core\Render\Element\HtmlTag result in passing a NULL value in a PHP string method via Xss::filterAdmin().

Steps to reproduce

See PHP 8.1 testing on #3220021: [meta] Ensure compatibility of Drupal 9 with PHP 8.1 (as it evolves)

Proposed resolution

Refactor code to take into account NULL values.

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

N/a

CommentFileSizeAuthor
#2 3233012-2.patch1.06 KBalexpott

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Active » Needs review
Issue tags: +PHP 8.1
Parent issue: » #3220021: [meta] Ensure compatibility of Drupal 9 with PHP 8.1 (as it evolves)
StatusFileSize
new 3233012-2.patch1.06 KB
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Do we need a test for this?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@longwave there's no change of output. A NULL value currently be changed to an empty string because it would be cast implicitly by the built-in php functions that in PHP 8.1 are now emitting a deprecation. Test coverage comes when we have PHP 8.1 testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

See \Drupal\Tests\Core\Render\Element\HtmlTagTest::providerPreRenderHtmlTag() we already have a test case for NULL values.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Reviewed & tested by the community

Ah OK, the existing test triggers the deprecation in PHP 8.1, that makes sense.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Needs review 
+++ b/core/lib/Drupal/Core/Render/Element/HtmlTag.php
@@ -95,8 +95,15 @@ public static function preRenderHtmlTag($element) {
+      if ($element['#value'] === NULL) {
+        $element['#markup'] = '';
+      }
+      elseif ($element['#value'] instanceof MarkupInterface) {
+        $element['#markup'] = $element['#value'];
+      }
+      else {
+        $element['#markup'] = Markup::create(Xss::filterAdmin($element['#value']));

I think this would read better as a new method, then you could return early and avoid elseif/else.

protected static function getMarkup(array $element): MarkupInterface {
  if ($element['#value'] instance of MarkupInterface) {
    return $element['#value'];
  }
  return Markup::create(Xss::filterAdmin($element['#value'] ?? '');
}

// Then it becomes
$element['#markup'] = static::getMarkup($element);

Also means it can scale in the future if there are other cases

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@larowlan I'm not sure about #7. There's a wasted call to Markup::create() and Xss::filterAdmin() on a NULL and this is not really an extension point or API so I can't see where the other cases come.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Needs review » Reviewed & tested by the community

There's a time and place to push my elseif is a code-smell agenda, but a critical that blocks php8.1 is not it.

Will commit this shortly, restoring RTBC

  • larowlan committed 6769562 on 9.3.x 
    Issue #3233012 by alexpott: Refactor \Drupal\Core\Render\Element\HtmlTag...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Fixed

Committed 6769562 and pushed to 9.3.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.