I have just updated my php version to 7.2.0RC3 .. so nothing released yet!

and I am seeing an errors - which to me implied we have a undiscovered bug

drush si .....

still works but outputs 9 identical warnings

[warning] count(): Parameter must be an array or an object that implements Countable FormValidator.php:260

here is the count call in context

        // A simple call to empty() will not cut it here as some fields, like
        // checkboxes, can return a valid value of '0'. Instead, check the
        // length if it's a string, and the item count if it's an array.
        // An unchecked checkbox has a #value of integer 0, different than
        // string '0', which could be a valid value.
        $is_empty_multiple = (!count($elements['#value']));
        $is_empty_string = (is_string($elements['#value']) && Unicode::strlen(trim($elements['#value'])) == 0);
        $is_empty_value = ($elements['#value'] === 0);

so count is being fed a value which it cannot process and returns an int anyway

https://blog.martinhujer.cz/php-7-2-is-due-in-november-whats-new/

it is coming out in November

to cherry pick a line from the blog post

I really like this change, because no one writes such code intentionally, but rather as a bug that needs fixing.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

martin107 created an issue. See original summary.

martin107’s picture

Issue summary: View changes

by boolean I mean int.

martin107’s picture

Status: Active » Needs review
FileSize
946 bytes

The solution is a one liner..... it seems like the obvious thing to do.

The error I see is removed.

Status: Needs review » Needs work

The last submitted patch, 3: 2915820-3.patch, failed testing. View results

martin107’s picture

Status: Needs work » Needs review
FileSize
1.31 KB

The tests failed because there is a bug here..

if the parameter input to count() is null

!count(null) // evaluates to TRUE

so in the old ... the working code

when $elements['#value'] is SET to a null value $is_empty_multiple is TRUE which in my view is the wrong outcome,

We are getting tripped up on the difference between isset and is_null

to maintain backwards compatibility

only I am introducing a new variable is_empty_null and future contrib coders must learn to unset a value rather that set to null!

so this becomes a three line change...

Status: Needs review » Needs work

The last submitted patch, 5: null-2915820-5.patch, failed testing. View results

martin107’s picture

The drupal7 version is here....

dawehner’s picture

+++ b/core/lib/Drupal/Core/Form/FormValidator.php
@@ -257,10 +257,11 @@ protected function doValidateForm(&$elements, FormStateInterface &$form_state, $
-        $is_empty_multiple = (!count($elements['#value']));
+        $is_empty_multiple = (is_array($elements['#value']) && count($elements['#value']) == 0);

Shouldn't we check for is_array || (instanceof Countable)or something along those lines?

martin107’s picture

Status: Needs work » Needs review
FileSize
1.4 KB
1.03 KB

Thanks I think the suggestion makes the code more readable.

I would rather avoid the brackets around instanceof Countable

the order of precedence is clear

instanceOf get looked at well before the logical && || wot-not.

http://php.net/manual/en/language.operators.precedence.php

martin107’s picture

FileSize
1.4 KB

Ah things that ended .txt should end .patch.... my bad,

Status: Needs review » Needs work

The last submitted patch, 10: 2915820-9.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

martin107’s picture

Status: Needs work » Needs review
FileSize
1007 bytes
1.4 KB

Doh.

Status: Needs review » Needs work

The last submitted patch, 12: 2915820-12.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mondrake’s picture

Priority: Normal » Critical

Bumping to Critical, alike other issues in the PHP 7.2 space that have to do with fatals raised under 7.2 with current code.

mondrake’s picture

Title: FormValidator: Parameter must be an array or an object that implements Countable » [PHP 7.2] FormValidator: Parameter must be an array or an object that implements Countable
tim.plunkett’s picture

Thanks for your work on this!

The $is_empty_multiple variable was used further on in the method, and so that conditional also needs the new $is_empty_null.

Additionally:
fixed instanceOf Countable to instanceof \Countable
renamed from $test_countability to $is_countable, for clarity

dawehner’s picture

+++ b/core/lib/Drupal/Core/Form/FormValidator.php
@@ -257,10 +257,12 @@ protected function doValidateForm(&$elements, FormStateInterface &$form_state, $
+        $is_countable = is_array($elements['#value']) || $elements['#value'] instanceof \Countable;

Can we add a line of documentation why we need that?

Out of curiosity reasons I had a look at what phpunit does in its code to count values, see \PHPUnit_Framework_Constraint_Count::getCountOf. I think we don't need to support traversables right now, so this approach seems 100% sane.

alexpott’s picture

This patch fixes all the PHP errors that occur with the message when running $ ./vendor/bin/phpunit -c ./core core/tests/Drupal/Tests/Core/Form - so the fix looks good. I agree that we don't need to deal with Traversable. Initially I agreed with @dawehner that a comment about what is going on here might be useful but looking at the code I think the code is quite readable and it is simple to see we are only counting countable things and not things where count() is meaningless. If @tim.plunkett feels an additional comment is good then +1 but I can see arguments against it.

tim.plunkett’s picture

The patch seems to feel like there should be a comment, but if you apply it and read the actual code afterward, it seems very straightforward.
I'd skip any extra comments.

xjm’s picture

@plach, @catch, @larowlan, @effulgentsia, and I discussed this issue today. We agreed it should be critical if for no other reason than that it will likely cause test failures on 7.2, which was just released.

@larowlan did point out that it wasn't clear what, if anything, would actually break as a result (other than a lot of warnings being raised in tests, server logs, etc.), so tagging for an issue summary update for that information (and to provide test coverage for it if applicable). According to https://wiki.php.net/rfc/counting_non_countables#backward_incompatible_c... it's supposed to be a BC change that calls attention to "a bug in the existing code".

Thanks!

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

So @tim.plunkett answered my question in #19 so I think this is rtbc.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 16: 29158200-form-16.patch, failed testing. View results

mondrake’s picture

Status: Needs work » Reviewed & tested by the community

#22 seems a random fail. back to rtbc

  • catch committed c6d6c4d on 8.5.x
    Issue #2915820 by martin107, tim.plunkett, dawehner: [PHP 7.2]...

  • catch committed 4ecce0b on 8.4.x
    Issue #2915820 by martin107, tim.plunkett, dawehner: [PHP 7.2]...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.5.x. Thanks!

  • catch committed cd8d11e on 8.4.x
    Revert "Issue #2915820 by martin107, tim.plunkett, dawehner: [PHP 7.2]...

Status: Fixed » Closed (fixed)

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

Chi’s picture

catch committed cd8d11e on 8.4.x
Revert "Issue #2915820 by martin107, tim.plunkett, dawehner: [PHP 7.2]...

Why? This is a critical bug that still appears in current stable Drupal version 8.4.4.

Berdir’s picture

Because it doesn't matter, Drupal 8.4 does not and will not support PHP 7.2, there are other issues too.

Chi’s picture

@Berdir seems like Drupal 8.4.4 release notes are not quite correct. Per that announcement, the only remaining critical bug on PHP 7.2 is #2923015: [PHP 7.2] Incompatible method declarations.

JamesOakley’s picture

Does this need backporting to 7.x, or are instances of this error in D7 entirely distinct?

David_Rothstein’s picture

Does this need backporting to 7.x, or are instances of this error in D7 entirely distinct?

See #7 for the backport issue.

aadeshvermaster@gmail.com’s picture

I have got the same problem in Drupal 8.3.7 with PHP 7.2. I am using the patch (https://www.drupal.org/files/issues/2915820-12.patch) and errors are gone now.