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.
Comment | File | Size | Author |
---|---|---|---|
#16 | 29158200-form-16.patch | 2.06 KB | tim.plunkett |
#16 | 29158200-form-16-interdiff.txt | 1.79 KB | tim.plunkett |
#12 | 2915820-12.patch | 1.4 KB | martin107 |
#12 | interdiff-2915820-10-12.txt | 1007 bytes | martin107 |
Comments
Comment #2
martin107 CreditAttribution: martin107 as a volunteer commentedby boolean I mean int.
Comment #3
martin107 CreditAttribution: martin107 as a volunteer commentedThe solution is a one liner..... it seems like the obvious thing to do.
The error I see is removed.
Comment #5
martin107 CreditAttribution: martin107 as a volunteer commentedThe 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
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...
Comment #7
martin107 CreditAttribution: martin107 as a volunteer commentedThe drupal7 version is here....
Comment #8
dawehnerShouldn't we check for
is_array || (instanceof Countable)
or something along those lines?Comment #9
martin107 CreditAttribution: martin107 as a volunteer commentedThanks 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
Comment #10
martin107 CreditAttribution: martin107 as a volunteer commentedAh things that ended .txt should end .patch.... my bad,
Comment #12
martin107 CreditAttribution: martin107 as a volunteer commentedDoh.
Comment #14
mondrakeBumping to Critical, alike other issues in the PHP 7.2 space that have to do with fatals raised under 7.2 with current code.
Comment #15
mondrakeComment #16
tim.plunkettThanks 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
toinstanceof \Countable
renamed from
$test_countability
to$is_countable
, for clarityComment #17
dawehnerCan 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.
Comment #18
alexpottThis 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.Comment #19
tim.plunkettThe 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.
Comment #20
xjm@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!
Comment #21
alexpottSo @tim.plunkett answered my question in #19 so I think this is rtbc.
Comment #23
mondrake#22 seems a random fail. back to rtbc
Comment #26
catchCommitted/pushed to 8.5.x. Thanks!
Comment #29
Chi CreditAttribution: Chi commentedWhy? This is a critical bug that still appears in current stable Drupal version 8.4.4.
Comment #30
BerdirBecause it doesn't matter, Drupal 8.4 does not and will not support PHP 7.2, there are other issues too.
Comment #31
Chi CreditAttribution: Chi commented@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.
Comment #32
JamesOakleyDoes this need backporting to 7.x, or are instances of this error in D7 entirely distinct?
Comment #33
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedSee #7 for the backport issue.
Comment #34
aadeshvermaster@gmail.com CreditAttribution: aadeshvermaster@gmail.com as a volunteer commentedI 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.