Problem/Motivation

The line $version = str_replace(\Drupal::CORE_COMPATIBILITY . '-', '', $modules[$dependency]->info['version']); can cause deprecations in PHP 8.1 because $modules[$dependency]->info['version'] can be a NULL value.

Steps to reproduce

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

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#13 3239287-2-13.patch4.46 KBalexpott
#13 3239287-2-13.test-only.patch3.67 KBalexpott
#13 11-13-interdiff.txt2.09 KBalexpott
#11 3239287-2-11.patch2.97 KBalexpott
#11 3239287-2-11.test-only.patch2.01 KBalexpott
#11 8-11-interdiff.txt2.71 KBalexpott
#8 3239287-2-8.patch1.65 KBalexpott
#8 3239287-2-8.test-only.patch713 bytesalexpott
#4 3239287-4.patch801 bytesalexpott
#2 3239287-2.patch982 bytesalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Category: Bug report » Task
Priority: Normal » Critical
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)
FileSize
3239287-2.patch982 bytes

Here's the fix from the meta issue.

I think this fix is okay while ->info is a dynamically set variable on the Extension class. Hopefully someday it will an ExtensionInfo object or something more elegant.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Needs review » Closed (duplicate)
Related issues: +#3239295: Passing NULL to the $replace parameter of str_replace() and preg_replace() triggers a deprecation in PHP 8.1

Going to make this a duplicate of #3239295: Passing NULL to the $replace parameter of str_replace() and preg_replace() triggers a deprecation in PHP 8.1 and handle all deprecations due to str_replace() in the same issue.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Closed (duplicate) » Needs review
FileSize
3239287-4.patch801 bytes

De-scoped this from #3239295: Passing NULL to the $replace parameter of str_replace() and preg_replace() triggers a deprecation in PHP 8.1 in order to work out why this is happening...

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented

I'm not sure this change is necessary we don't seem to be hitting the exception - I've triggered a PHP 8.1 run without this change on the meta issue - see https://www.drupal.org/pift-ci-job/2190498

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Needs review » Closed (cannot reproduce)

I'm pretty sure that this change is not needed given the results on the meta issue. Going to close this one.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Closed (cannot reproduce) » Needs work

This is blocking contrib testing. See https://www.drupal.org/pift-ci-job/2261587 for an example from Search API. If a module is obtained via git or composer and the git_deploy or composer_deploy modules are not available then Drupal will not have the version info and it will be NULL which will cause a deprecation to be triggered.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Needs work » Needs review
FileSize
3239287-2-8.test-only.patch713 bytes
3239287-2-8.patch1.65 KB

#2 remains the simplest thing to do here which preserves the current PHP 8.0 and below behaviour and behaves the same way on PHP 8.1 without the deprecations. The reason core does not trigger this is because all core modules have VERSION as the version string.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

Queued test only patch for 10.x and PHP 8.1

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs review » Needs work

There are plenty of .info.yml in core that don't contain a version line already, e.g. core/modules/system/tests/modules/advisory_feed_test/advisory_feed_test.info.yml

name: 'Advisory feed test'
type: module
description: 'Support module for advisory feed testing.'
package: Testing

Maybe this only triggers for modules outside the Testing package or something like that?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Needs work » Needs review
FileSize
8-11-interdiff.txt2.71 KB
3239287-2-11.test-only.patch2.01 KB
3239287-2-11.patch2.97 KB

Ah I see we need the module dependency system to kick in. Let's do this we modules being added to the site instead of core.

The last submitted patch, 11: 3239287-2-11.test-only.patch, failed testing. View results

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
FileSize
11-13-interdiff.txt2.09 KB
3239287-2-13.test-only.patch3.67 KB
3239287-2-13.patch4.46 KB

Found another place where we have exactly the same code that we need to fix in the same way. See system_requirements()

longwave’s picture

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

Oh it would be so nice to have proper value objects instead of arrays :)

I looked for various combinations of info and version and can't find anything else that would cause a similar compatibility warning so I think this is RTBC.

  • catch committed 205729d on 10.0.x 
    Issue #3239287 by alexpott, longwave: Fix \Drupal\Core\Extension\...

  • catch committed b196520 on 9.4.x 
    Issue #3239287 by alexpott, longwave: Fix \Drupal\Core\Extension\...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 10.0.x and cherry-picked to 9.4.x and 9.3.x, thanks!

  • catch committed 9ab4ed2 on 9.3.x 
    Issue #3239287 by alexpott, longwave: Fix \Drupal\Core\Extension\...
longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented

Did a double-take at the test results here, but it turns out the fail on PHP 8.1 in 3239287-2-13.patch is an unrelated random JavaScript fail.

ressa’s picture

ressa CreditAttribution: ressa at Ardea commented

I just ran into this, #3255175: [D9 PHP 8.1] Deprecated function: str_replace(): Passing null to parameter ... on Extend and Status page. Thanks for fixing it :)

Status: Fixed » Closed (fixed)

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

jonathan1055’s picture

jonathan1055 CreditAttribution: jonathan1055 as a volunteer commented

Thanks for doing this, however, there is still one more to fix. The same ?? correction also needs to be done in the ModuleDependencyMessageTrait 'incompatible with' return message. The test module's dependency just has the module name dependency_test but no version. So I am guessing that !$dependency_object->isCompatible($version) is not true in the test case, and the message is not displayed.

If a version is given for the required module, then the condition will be true and the message is displayed, and php8.1 complains again that the string is null. I discovered this in a site where the required module was a git repo and so have no explicit version in .info.yml

Instead of re-opening this issue I will start a new one, and refer back to here? I will expand the test coverage to try to demonstrate the failure.

jonathan1055’s picture

jonathan1055 CreditAttribution: jonathan1055 as a volunteer commented

Added the follow-up #3310555: ModuleDependencyMessageTrait - htmlspecialchars(): Passing null to parameter #1 is deprecated