Problem/Motivation
We are experiencing a deprecated function warning after migration to PHP 8.1:
Deprecated function: strlen(): Passing null to parameter #1 ($string) of type string is deprecated in theme_file_upload_help() (line 946 of xxx\modules\file\file.field.inc).
PHP: 8.1
Database: PostgreSQL 11 / MySQL 8 (problem is present regardless of database type)
Drupal: 7.84
The problem appears to be in this part, where $description variable "empty check" is done by strlen() function:
$description = $variables['description'];
$upload_validators = $variables['upload_validators'];
$descriptions = array();
if (strlen($description)) {
$descriptions[] = $description;
}
Steps to reproduce
Issue can be reproduced on clean Drupal 7.84 install:
- Install Drupal 7.84 with standard profile
- Edit field_image in "article" content type (admin/structure/types/manage/article/fields/field_image) - set "Number of values" to more than 1 (2, 3, ... or unlimited)
- Go to "Add content" - "Article" (node/add/article)
- Deprecated warning appears
Proposed resolution
Replace / supplement the strlen() check with an additional !empty() function.
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Issue fork drupal-3258313
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:
Comments
Comment #2
beatrizrodrigueswill work on that
Comment #4
beatrizrodriguesI applied the proposed resolution and no message is being displayed anymore. thank you
Comment #5
steinmb CreditAttribution: steinmb as a volunteer commentedLooks OK to me.
Comment #7
mcdruidCan we add a test that reproduces the error before this change is made please?
Comment #8
mcdruidHere's an incredibly basic unit test that hopefully shows the failure with PHP 8.1
I did start adding the other theme implementations declared in file_theme() but I didn't find any that would work with such a rudimentary unit test.
The patch adds the latest diff from the MR to the test, which should pass with any luck.
Comment #9
Liam MorlandThe test looks good and passes on at least one PHP 8.1 testing configuration.
Comment #10
poker10 CreditAttribution: poker10 at ActivIT s.r.o. commentedOn the second run all tests with the patch applied seems to be green (funny).
Comment #12
mcdruidGreat, those were the results we wanted.
Thank you everyone that contributed.
Comment #13
jromine CreditAttribution: jromine commentedIs empty("0") still considered true? Could this be a regression if description is "0".
Comment #14
Liam Morlandempty("0")
is TRUE.Maybe we should use:
if (strlen((string) $description)) {
Comment #15
mcdruidTheoretically yes it could be a regression in that situation; IIUC the zero would no longer be displayed.
However, what's the use case for the description to be set to
0
?Patches welcome (with tests) - as are arguments to the contrary - but personally I'm not too concerned about this specific edge case.