Problem/Motivation
Follow-up from #3224299-40: [META] Make Drupal 7 core compatible with PHP 8.1
Deprecated function: str_replace(): Passing null to parameter #3 ($subject) of type array|string is deprecated in system_modules() (line 842 of /var/www/html/web/modules/system/system.admin.inc).
Steps to reproduce
Go to /admin/modules after a fresh install of drupal standard profile. Download a dev copy of probably any module (I used feeds) and version is null
Proposed resolution
Remaining tasks
Write tests
User interface changes
API changes
Data model changes
Release notes snippet
| Comment | File | Size | Author |
|---|---|---|---|
| #14 | 3255068-14.patch | 4.22 KB | mcdruid |
| #14 | 3255068-14_test_only.patch | 2.42 KB | mcdruid |
| #13 | 3255068_tests_only.patch | 2.42 KB | mcdruid |
| #7 | deprecated_passing_null_str_replace3255068-7.patch | 1.8 KB | mulambo |
Issue fork drupal-3255068
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 3255068-d7-php-8.1
changes, plain diff MR !1629 /
changes, plain diff MR !1564
Comments
Comment #2
joelpittetComment #3
joelpittetComment #5
joelpittetComment #7
mulambo commented@joelpittet: I've fixed your merge request (you created merge request to 9.4.x) to 7.x.
I've also created patch from your commits to be used while waiting for merge.
Comment #8
mulambo commentedComment #10
joelpittet@Mulambo FYI you can use the merge request to get the patch.
https://git.drupalcode.org/project/drupal/-/merge_requests/1629
Can be converted to a patch with: .diff or .patch at the end:
https://git.drupalcode.org/project/drupal/-/merge_requests/1629.patch
https://git.drupalcode.org/project/drupal/-/merge_requests/1629.diff
Same can be done in github pull requests.
Comment #11
mulambo commented@joelpittet Thanks for the info, didn't know about patches from merge requests.
Comment #12
mcdruid commentedIdeally we'd like one or more tests to accompany all of these PHP 8.1 fixes.
However, both of these changes look like they're buried in long functions that are not well suited to targetted unit testing.
Is there any existing test coverage that could be adapted / built upon to exercise this problem with PHP 8.1?
Comment #13
mcdruid commentedWow, the functions we're changing here really don't lend themselves to nice clean unit tests :)
I think this ought to work, but I'm having trouble getting it to fail with PHP 8.1 locally. Let's see what the bot says.
It's actually the module's dependencies that cause a problem if they have a null version, so we need two test modules.
I'm fairly certain this test results in this line getting a null version:
https://git.drupalcode.org/project/drupal/-/blob/7.87/modules/system/sys...
...so I'm not sure why I'm getting no error locally.
Comment #14
mcdruid commentedWell that certainly seems to have triggered the exception with PHP 8.1 - strictly speaking I think this is only exercising the code in system.admin.inc and not the .install function that we're also changing. I think that's probably sufficient though.
Here's the test only patch again with a tiny tweak (proper version for the test module that requires the problematic dependency) plus a test + fix patch (latest from the MR).
Comment #16
mcdruid commentedThanks everyone!