Problem/Motivation

\Drupal\Core\Ajax\AjaxHelperTrait::getRequestWrapperFormat() can return a NULL. On PHP 8.1 this causes deprecations because we use the result in strpos() and there expects strings.

Proposed resolution

Refactor to not trigger a deprecation on PHP 8.1

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

N/a

CommentFileSizeAuthor
#6 3232691-6.patch2.59 KBalexpott
#6 2-6-interdiff.txt1.43 KBalexpott
#3 3232691-3.patch1.16 KBalexpott
#3 2-3-interdiff.txt535 bytesalexpott
#2 3232691-2.patch1.16 KBalexpott

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Title: Fix \Drupal\Core\Ajax\AjaxHelperTrait::getRequestWrapperFormat() to always return a string » Refactor \Drupal\Core\Ajax\AjaxHelperTrait to not cause deprecations in PHP 8.1
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new 3232691-2.patch1.16 KB
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 2-3-interdiff.txt535 bytes
new 3232691-3.patch1.16 KB
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented 
+++ b/core/lib/Drupal/Core/Ajax/AjaxHelperTrait.php
@@ -18,19 +18,17 @@ trait AjaxHelperTrait {
+    return str_contains($wrapper_format, 'drupal_ajax') ||

First usage of str_contains() in core. I guess we are OK to assume our dependency on the polyfill for PHP 7, although we only inherit from Symfony and don't explicitly state our dependency on symfony/polyfill-php*.

alexpott’s picture

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

#4 is an interesting question. I think we should add an explicit dependency... not that it is going anywhere...

symfony/browser-kit           v4.4.27  requires  symfony/polyfill-php80 (^1.16)
symfony/console               v4.4.30  requires  symfony/polyfill-php80 (^1.16)
symfony/css-selector          v4.4.27  requires  symfony/polyfill-php80 (^1.16)
symfony/dependency-injection  v4.4.27  requires  symfony/polyfill-php80 (^1.16)
symfony/dom-crawler           v4.4.30  requires  symfony/polyfill-php80 (^1.16)
symfony/event-dispatcher      v4.4.30  requires  symfony/polyfill-php80 (^1.16)
symfony/filesystem            v4.4.27  requires  symfony/polyfill-php80 (^1.16)
symfony/finder                v4.4.30  requires  symfony/polyfill-php80 (^1.16)
symfony/http-foundation       v4.4.30  requires  symfony/polyfill-php80 (^1.16)
symfony/http-kernel           v4.4.30  requires  symfony/polyfill-php80 (^1.16)
symfony/lock                  v4.4.27  requires  symfony/polyfill-php80 (^1.16)
symfony/mime                  v5.3.7   requires  symfony/polyfill-php80 (^1.16)
symfony/process               v4.4.30  requires  symfony/polyfill-php80 (^1.16)
symfony/routing               v4.4.30  requires  symfony/polyfill-php80 (^1.16)
symfony/serializer            v4.4.27  requires  symfony/polyfill-php80 (^1.16)
symfony/translation           v4.4.30  requires  symfony/polyfill-php80 (^1.16)
symfony/validator             v4.4.30  requires  symfony/polyfill-php80 (^1.16)
symfony/var-dumper            v5.3.7   requires  symfony/polyfill-php80 (^1.16)
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 2-6-interdiff.txt1.43 KB
new 3232691-6.patch2.59 KB

Here's the patch with the dependency added explicitly.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented 
+++ b/composer.lock
@@ -6964,6 +6965,7 @@
+            "abandoned": true,

This is from sebastian/resource-operations and is automatically added as this has now been abandoned...

longwave’s picture

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

str_contains() is much easier to read, so +1 from me, otherwise this is straightforward enough and a docs fix to boot as @string is not a valid phpdoc tag :)

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

This looks good to go post freeze, will come back to it at the end of the week

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Issue tags: +Needs release manager review

Tagging for needs release manager review for the new dependency, will ping jess and catch. Given this is already in our lock file - can't see it being an issue.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs release manager review

Yep given we're already using the polyfill implicitly, using it explicitly seems fine. We could probably open an issue to start using str_contains() more in core.

Committed 4e54392 and pushed to 9.3.x. Thanks!

  • catch committed 4e54392 on 9.3.x 
    Issue #3232691 by alexpott, longwave: Refactor \Drupal\Core\Ajax\...
catch’s picture

catch
he/him
Primary language English
commented
quietone’s picture

quietone commented

This issue introduced str_contains into Core which is only available with PHP 8.0. However, the PHP requirements page shows that Drupal 9 will work with PHP 7.3 and 7.4, although it is not recommended.

xjm’s picture

xjm
she/her
Primary language English
commented

+1 on a revert for this to sort #14.

alexpott’s picture

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

We have a polyfil for str_contains() and this issue added a proper dependency on it. A revert is not necessary. The polyfil is part of Symfony.

alexpott’s picture

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

See \Symfony\Polyfill\Php80\Php80::str_contains() for the code...

quietone’s picture

quietone commented

@alexpott, thanks for the explanation.

Status: Fixed » Closed (fixed)

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