Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#6 | 3232691-6.patch | 2.59 KB | alexpott |
#6 | 2-6-interdiff.txt | 1.43 KB | alexpott |
#3 | 3232691-3.patch | 1.16 KB | alexpott |
#3 | 2-3-interdiff.txt | 535 bytes | alexpott |
#2 | 3232691-2.patch | 1.16 KB | alexpott |
Comments
Comment #2
alexpottComment #3
alexpottComment #4
longwaveFirst 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*.
Comment #5
alexpott#4 is an interesting question. I think we should add an explicit dependency... not that it is going anywhere...
Comment #6
alexpottHere's the patch with the dependency added explicitly.
Comment #7
alexpottThis is from sebastian/resource-operations and is automatically added as this has now been abandoned...
Comment #8
longwavestr_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 :)Comment #9
larowlanThis looks good to go post freeze, will come back to it at the end of the week
Comment #10
larowlanTagging 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.
Comment #11
catchYep 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!
Comment #13
catchComment #14
quietone CreditAttribution: quietone as a volunteer commentedThis 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.
Comment #15
xjm+1 on a revert for this to sort #14.
Comment #16
alexpottWe 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.
Comment #17
alexpottSee \Symfony\Polyfill\Php80\Php80::str_contains() for the code...
Comment #18
quietone CreditAttribution: quietone as a volunteer commented@alexpott, thanks for the explanation.