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:

  1. Install Drupal 7.84 with standard profile
  2. 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)
  3. Go to "Add content" - "Article" (node/add/article)
  4. 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

Command icon 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:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

poker10 created an issue. See original summary.

beatrizrodrigues’s picture

Assigned: Unassigned » beatrizrodrigues

will work on that

beatrizrodrigues’s picture

Assigned: beatrizrodrigues » Unassigned
Status: Active » Needs review

I applied the proposed resolution and no message is being displayed anymore. thank you

steinmb’s picture

Status: Needs review » Reviewed & tested by the community

Looks OK to me.

Liam Morland made their first commit to this issue’s fork.

mcdruid’s picture

Issue tags: +Needs tests

Can we add a test that reproduces the error before this change is made please?

mcdruid’s picture

Here'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.

Liam Morland’s picture

The test looks good and passes on at least one PHP 8.1 testing configuration.

poker10’s picture

On the second run all tests with the patch applied seems to be green (funny).

  • mcdruid committed bb5b229 on 7.x
    Issue #3258313 by beatrizrodrigues, mcdruid, Liam Morland, poker10,...
mcdruid’s picture

Status: Reviewed & tested by the community » Fixed

Great, those were the results we wanted.

Thank you everyone that contributed.

jromine’s picture

Is empty("0") still considered true? Could this be a regression if description is "0".

Liam Morland’s picture

empty("0") is TRUE.

Maybe we should use:
if (strlen((string) $description)) {

mcdruid’s picture

Theoretically 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.

Status: Fixed » Closed (fixed)

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