[PHP 7.2] Avoid count() calls on uncountable variables

Thanks for working on this.

Your issue summary speaks of warning notices being thrown "here and there", whereas your patch only addresses form.inc. Was that deliberate?

So: Your patch definitely helps the issue on the site I tested it on. But I'm still getting a lot of these:

Parameter must be an array or an object that implements Countable in theme_table() (line 2061 of {site_root_path}/includes/theme.inc).

I'll mark this back as Needs Work, but if the warning above is a genuinely different issue it can instead go RTBC, and I'll open a separate issue about theme_table in theme.inc

There's a patch at #2932916-7: [PHP7] Warning: count(): Parameter must be an array or an object that implements Countable in PHP 7.2 that could perhaps be combined with this one.

For the form.inc changes, any reason for the subtle differences between this patch and what was committed to Drupal 8 in #2915820: [PHP 7.2] FormValidator: Parameter must be an array or an object that implements Countable?

php 7.2 compatibility very close. I created a related issue to test php 7.2 compatibility with this issue fix and also the each fix in #2925449: [PHP 7] Function each() is deprecated since PHP 7.2 [D7] only one test failure , although I am unsure why. would take some debugging.

here: #2947772: Fully support PHP 7.2 in Drupal 7

combined patch 7 above with patch from the other issue
#2925449-55: [PHP 7] Function each() is deprecated since PHP 7.2 [D7]

Code needs work per #8.

#8 is referring to the revert commit.
The actual D8 commit David Rothstein refers to is
https://cgit.drupalcode.org/drupal/commit/?id=c6d6c4d

I've attempted to make these changes. as I interpret them.

the roll up all patch was off of the interdiff, oops.
here it is again corrected.

try again

patch #7 looks like the best solution

I tried the suggestion in #8 , but it caused form errors. Whereas patch #7 has no form errors.

of note
in patch 14 I changed this:

-  $items = $variables['items'];
+  $items = (array) $variables['items'];

Didn't seem to make a difference, but maybe we don't have enough theme test coverage for that. as there is a count() on those items but they're apparently already countable (or arrays) because no issue there.

and also

-        $cells = $row['data'];
+        $cells = (array) $row['data'];

Patch 7 doesn't have this, however it doesn't seem to change the test results as they're apparently already countable (or arrays) because no issue there either afaik.

The 7.2 rollup issue (referenced) only has 1 failure left, and it appears to be unrelated to patch 7.

Patch 7 is an improvement, we can start with that.
I suggest we move forward and attack the one single error that remains
#2947772: Fully support PHP 7.2 in Drupal 7

so commit patch 7 (or something close to it)
and commit RTBC patch 106 here:
#2925449-106: [PHP 7] Function each() is deprecated since PHP 7.2 [D7]

and then focus on the remaining issue in:
#2947772: Fully support PHP 7.2 in Drupal 7

I'd go with #7 here by Ayesh.

Crack open a bottle of bubbly! php 7.2 now passing all tests.
Great work everyone!
See:
#2947772: Fully support PHP 7.2 in Drupal 7

ready for commit

1. +++ b/includes/form.inc
   @@ -1440,9 +1440,11 @@ function _form_validate(&$elements, &$form_state, $form_id = NULL) {
   +      $is_empty_null = is_null($elements['#value']);
   

   This needs to be actually used.

   See:

   https://cgit.drupalcode.org/drupal/commit/?id=c6d6c4d

   Can someone find out why D8 added this?

2. +++ b/includes/theme.inc
   @@ -1986,8 +1986,8 @@ function theme_breadcrumb($variables) {
   +  $header = (array) $variables['header'];
   +  $rows = (array) $variables['rows'];
   
   @@ -2082,7 +2082,7 @@ function theme_table($variables) {
   -        $cells = $row['data'];
   +        $cells = (array) $row['data'];
   
   @@ -2173,7 +2173,7 @@ function theme_mark($variables) {
   -  $items = $variables['items'];
   +  $items = (array) $variables['items'];
   

   This needs a comment in why this is necessary - as it looks strange without context:

   When does this even happen and why?

   Was this changed in D8 in the same way or is it different here, because of the Twig conversion?

@Fabianx #18:

#18.1
$is_empty_null was added since $is_empty_multiple no longer equals TRUE when $element['#value'] is NULL.
Not adding this condition will allow required fields to have NULL values without setting a form error message.

#18.2
Looking at function template_preprocess_table in D8 it seems D8 checks if the arrays contain values by using !empty($variable) instead of count($variable). This seems logic since the count() method is only called for checking if the array contains any value. Using count instead of !empty has overhead (it unnecessarily counts the array items while we just need to check if the array contains any item).

Default values in D7 in common.inc contain NULL:

function drupal_common_theme() {
  return array(
    'table' => array(
      'variables' => array('header' => NULL, 'rows' => NULL, 'attributes' => array(), 'caption' => NULL, 'colgroups' => array(), 'sticky' => TRUE, 'empty' => ''),
    ),
  );
}

The default value for items in item_list is an empty array:

function drupal_common_theme() {
  return array(
    'item_list' => array(
      'variables' => array('items' => array(), 'title' => NULL, 'type' => 'ul', 'attributes' => array()),
    ),
  );
}

Updated the patch for #18.1 and #18.2

• Added $is_empty_null check in required field validation
• Replaced count() on possible NULL values with !empty check.
• Added an additional !empty check before iterating over the $headers variable
• Removed variable casting to arrays.

Great work!

Don't we need to add the is_null check also to the 2nd conditional check like in Drupal 8?

@Fabianx #21
Doesn't seem so.
D8 (source) first sets $elements['#required_but_empty'] = TRUE;, then triggers element_validate validators and finally checks the statement again to set the 'field is required' error. Whereas D7 (source) directly sets the 'field is required' error and then triggers element_validate validators.

Okay, great.

If someone else could RTBC this, I can give it a final committer review and make it ready for commit ;).

Great #joseph.olstad patch 20 got all the error and logs gone

@Bahson, do you mean patch 19 by @sjerdo ? there is no patch 20 here in this issue?

**EDIT**
We should now go with patch 19 by @sjerdo as it has adjustments made as recommended by FabianX
the patch I uploaded in comment #14 is not good, it is hidden for that reason.

As I said: If I were to RTBC it, I can't commit it technically ;), so if someone could test it and review it and give it community blessing we can get PHP 7.2 support sorted.

RTBC Patch #19. One step closer to getting php 7.2 tests going on the contrib modules. As soon as we get these related patches into core I'll start adding php 7.2 tests for the modules I have access to like media, file_entity , i18n, webform_localization, exif_custom, and as many other contrib modules as possible. Once this is in, we can start making noise in the ctools queues and views queue and the rest of contrib land so that we can get contrib ready for 7.2 asap.

Very satisfied with the progress in D7 core, just two other related 7.2 patches and we're off to the races.

we need:
#2925449: [PHP 7] Function each() is deprecated since PHP 7.2 [D7]

AND

#2946045: Unable to update modules due to Archive_Tar incompatibility with PHP 7.2

This is pending commit

Committed and pushed to 7.x. Thanks!

I think this broke PHP 5.2 compatibility -- not sure there are that many people who care at this point, but it's still officially supported. Can we fix via the attached?

Of course

Thanks a lot @David_Rothstein. your patch worked perfectly. But I have applied this for Drupal core 7.6 version.

Cannot apply patch #32 to Core 7.60! The form.inc file has changed on 7.60

Rather than patch, the easiest thing to do (if you have drush) is upgrade to D7 core version 7.x dev as follows:

drush up drupal-7.x
Update information last refreshed: Fri, 10/26/2018 - 23:47
 Name    Installed Version  Proposed version  Message                     
 Drupal  7.60               7.x-dev           Specified version available 


Code updates will be made to drupal core.
WARNING:  Updating core will discard any modifications made to Drupal core files, most noteworthy among these are .htaccess and robots.txt.  If you have made any modifications to these files, please back them up before updating so that you can re-create your modifications in the updated version of the file.
Note: Updating core can potentially break your site. It is NOT recommended to update production sites without prior testing.

Do you really want to continue? (y/n): y

Thanks, Patch #19 works on core 7.60.

Back to fixed

FYI, I've created a new issue for support of PHP 7.3.
#3028648: Fully support PHP 7.3 in Drupal 7