Contrib.social

Runtime PHP TypeError when comparing broken revisions

Open on Drupal.org →
Created on 17 October 2023, about 1 year ago
Updated 31 May 2024, 6 months ago

Problem/Motivation

When comparing revisions that have unexpectedly missing data, PHP 8.0 and above will return a TypeError:
Argument #1 ($value) must be of type Countable|array, int given in diff_process_state_lines() (line 521 of diff/diff.pages.inc).

Steps to reproduce

Unfortunately it's difficult to reproduce the issue; it involves a combination of the Panels IPE and Workbench Moderation. The Panels IPE allows unchecking the Create Revision checkbox when customizing a page. In some cases, unchecking this box when saving a draft of a moderated node results in a broken revision. When you compare revisions, any comparison that encompasses the broken revision will return the TypeError.

Proposed resolution

Here's the function in question:

/**
 * Helper function to get line counts per field.
 */
function diff_process_state_lines(&$diff, $key) {
  foreach ($diff['#states'] as $state => $data) {
    if (isset($data['#old'])) {
      if (is_string($data['#old'])) {
        $diff['#states'][$state]['#old'] = explode("\n", $data['#old']);
      }
      $diff['#states'][$state]['#count_old'] = count($diff['#states'][$state]['#old']);
    }
    else {
      $diff['#states'][$state]['#count_old'] = 0;
    }
    if (isset($data['#new'])) {
      if (is_string($data['#new'])) {
        $diff['#states'][$state]['#new'] = explode("\n", $data['#new']);
      }
      $diff['#states'][$state]['#count_new'] = count($diff['#states'][$state]['#new']);
    }
    else {
      $diff['#states'][$state]['#count_new'] = 0;
    }
  }
}

This function assumes that if the incoming data isn't a string, it's an array. In the broken case, it's neither, it's an integer. We can add a conditional to check whether the non-string is an array. However, in versions of PHP prior to 8.0, in some cases count() would return 1 instead of 0 when trying to count a non-array/non-string, so behavior might change.

It's tempting to just cast the non-array to a string - this would work for booleans, integers, and floats, but not for objects that don't implement toString.

Remaining tasks

Patch and test.

User interface changes

None.

API changes

None.

Data model changes

None.

🐛 Bug report
Status

Needs work

Version

3.0

Component

Code

Created by

🇺🇸United States cboyden

Live updates comments and jobs are added and updated live.
  • PHP 8.0

    The issue particularly affects sites running on PHP version 8.0.0 or later.

Sign in to follow issues

Comments & Activities

  • Issue created by @cboyden
  • Issue was unassigned.
  • Status changed to Needs review about 1 year ago
  • Open on Drupal.org →
    Core: 7.x + Environment: PHP 5.3 & MySQL 5.5
    last update about 1 year ago
    Waiting for branch to pass
  • Comment about 1 year ago →
    🇺🇸United States cboyden

    Here's a patch that works in my testing. The broken revision comparison no longer causes a type error. It seems pretty unlikely that the data passed to the function will be an object, but if it's important to consider that case, I can update the patch to account for that.

  • Comment about 1 year ago →
    🇺🇸United States cboyden
  • Status changed to RTBC about 1 year ago
  • Comment about 1 year ago →
    🇺🇸United States dsnopek USA

    This makes sense to me! While this may lead to slight change in behavior with weird inputs, it seems to me that the outputs it would generate are a little bit more sensible than previously, so I think this is OK.

  • Status changed to Needs work 6 months ago
  • Comment 6 months ago →
    🇦🇺Australia acbramley

    Thank you for your contribution. This issue currently does not meet the Contribution guidelines which are required to get this change committed.

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024