Avoid warning in PHP 7.2 because of non-countable data #20859

wouterj · 2016-12-10T17:02:30Z

Q	A
Branch?	2.7
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	-
License	MIT
Doc PR	-

Recently, the "Counting of non-countable objects" RFC was merged in PHP 7.2-dev. This means count() now causes a warning when passing anything that's not countable (e.g. null or '').

As PHP does not lazily execute conditions, FormUtil::isEmtpy($data) || 0 === count($data) will cause both conditions to be executed. This means count($data) errors when $data is empty.

Splitting it up in 2 statements avoids the warning being triggered in PHP 7.2.

See https://travis-ci.org/symfony-cmf/content-bundle/jobs/181815895 for a failing test caused by this bug.

ogizanagi · 2016-12-10T17:53:44Z

As PHP does not lazily execute conditions, FormUtil::isEmtpy($data) || 0 === count($data) will cause both conditions to be executed. This means count($data) errors when $data is empty.

I'm quite surprised by your statement and the fact it solves your issue, because AFAIK php does execute lazily expressions (short circuit evaluation): https://3v4l.org/V4H6E.
So it should execute 0 === count($data) only when FormUtil::isEmtpy($data) is false.

See also http://php.net/manual/en/language.operators.logical.php#example-103

Am I missing something?

ogizanagi · 2016-12-10T17:59:40Z

src/Symfony/Component/Form/Form.php

+        }
+
+        // arrays, countables
+        return 0 === count($this->modelData) ||
            // traversables that are not countable
            ($this->modelData instanceof \Traversable && 0 === iterator_count($this->modelData));


Shouldn't it check this before 0 === count($this->modelData)?

With the latest update, it doesn't have to be done before the count.

wouterj · 2016-12-10T18:18:18Z

Oh, indeed. You're completely correct, I interpreted the erroring build incorrectly. The problem is actually when counting non-empty strings or the like.

Updated the PR with the correct change.

ogizanagi · 2016-12-10T18:24:03Z

👍

Status: Reviewed.

nicolas-grekas · 2016-12-12T10:02:01Z

src/Symfony/Component/Form/Form.php

@@ -748,7 +748,7 @@ public function isEmpty()

        return FormUtil::isEmpty($this->modelData) ||
            // arrays, countables
-            0 === count($this->modelData) ||
+            ((is_array($this->modelData) || $this->modelData instanceof \Countable) && 0 === count($this->modelData)) ||


what about:

array() === $this->modelData || ($this->modelData instanceof \Countable && 0 === count($this->modelData) ||

This is to avoid calling count() on an array? If that's the case, shouldn't we fix this throughout all 0 === count() calls in the code base?

I find it more readable :)

creating an empty array to test if some var is also an empty array seems a bit much for syntax sugar?

Shouldnt this be covered by FormUtils::isEmpty (or maybe add isEmptyCollection)?

Ie. what about https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php#L98?

xabbuh · 2016-12-25T18:28:13Z

What is the state here?

wouterj · 2016-12-25T22:13:21Z

@xabbuh PR is ready to merge imo. @nicolas-grekas's comment seems to be more related to a change in coding standard (array() === $var instead of 0 === count($var)). This should be moved into a separate issue, to discuss it and fix it throughout the code base consistently.

fabpot · 2016-12-26T11:10:10Z

Thank you @wouterj.

…uterj) This PR was merged into the 2.7 branch. Discussion ---------- Avoid warning in PHP 7.2 because of non-countable data | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Recently, the "[Counting of non-countable objects][1]" RFC was merged in PHP 7.2-dev. This means `count()` now causes a warning when passing anything that's not countable (e.g. `null` or `''`). As PHP does not lazily execute conditions, `FormUtil::isEmtpy($data) || 0 === count($data)` will cause *both* conditions to be executed. This means `count($data)` errors when `$data` is empty. Splitting it up in 2 statements avoids the warning being triggered in PHP 7.2. See https://travis-ci.org/symfony-cmf/content-bundle/jobs/181815895 for a failing test caused by this bug. [1]: https://wiki.php.net/rfc/counting_non_countables Commits ------- 94253e8 Only count on arrays or countables to avoid warnings in PHP 7.2

nicolas-grekas · 2016-12-26T11:31:09Z

@wouterj note that I'm a bit disappointed here. In https://speakerdeck.com/nicolasgrekas/go-beyond-composer-update-contribue, you'll find a slide saying: "When you don't care, follow the suggestions".
Why? Because it saves everyone's time, and gratifies reviewers for their contribution. I could elaborate but enough time on this, merging was indeed required now.
Note that I added this slide by thinking of my own experience resisting to suggestions. The human brain is lazy and resisting is often the default behavior. Yet, I learned that this is making everyone loose their time, so that when I don't care enough, I'd better do the change than start another bike-shading.

ro0NL · 2016-12-26T11:47:05Z

4-1 against should be followed, i like your point here 👍

Note that the opposite is also true, reviewers not always know about inner details, so if it's ignored they dont care as well (we dont want to say anything (more) stupid do we). Sometimes it's just a brainfart, or a what if suggestion.

Problem is.. if we all start ignoring and stop caring the codebase will suffer.

…ytrase) This PR was squashed before being merged into the 2.7 branch (closes #21155). Discussion ---------- [Validator] Check cascasdedGroups for being countable Prevents notice for PHP 7.2 | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | Just repeated for this place #20859 Waiting travis to fill in the `Tests pass` field `cascasdedGroups` can be null at this point (according to failures and phpdoc) Commits ------- 8fa45a1 [Validator] Check cascasdedGroups for being countable

carsonbot added Status: Needs Review Bug labels Dec 10, 2016

wouterj force-pushed the patch-18 branch from 6c8bdd5 to cb81122 Compare December 10, 2016 17:05

wouterj mentioned this pull request Dec 10, 2016

Remove admins symfony-cmf/content-bundle#157

Merged

ogizanagi reviewed Dec 10, 2016

View reviewed changes

Only count on arrays or countables to avoid warnings in PHP 7.2

94253e8

wouterj force-pushed the patch-18 branch from cb81122 to 94253e8 Compare December 10, 2016 18:18

carsonbot added Status: Reviewed and removed Status: Needs Review labels Dec 10, 2016

nicolas-grekas added this to the 2.7 milestone Dec 12, 2016

nicolas-grekas reviewed Dec 12, 2016

View reviewed changes

fabpot merged commit 94253e8 into symfony:2.7 Dec 26, 2016

wouterj deleted the patch-18 branch December 26, 2016 12:16

scaytrase mentioned this pull request Jan 4, 2017

[Validator] Check cascasdedGroups for being countable #21155

Closed

This was referenced Jan 12, 2017

Release v2.7.23 #21258

Merged

Release v2.8.16 #21259

Merged

Release v3.1.9 #21260

Merged

Release v3.2.2 #21261

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid warning in PHP 7.2 because of non-countable data #20859

Avoid warning in PHP 7.2 because of non-countable data #20859

wouterj commented Dec 10, 2016

ogizanagi commented Dec 10, 2016

ogizanagi Dec 10, 2016

wouterj Dec 10, 2016

wouterj commented Dec 10, 2016

ogizanagi commented Dec 10, 2016

nicolas-grekas Dec 12, 2016

wouterj Dec 12, 2016

nicolas-grekas Dec 12, 2016

SpacePossum Dec 15, 2016

ro0NL Dec 16, 2016 •

edited

Loading

xabbuh commented Dec 25, 2016

wouterj commented Dec 25, 2016

fabpot commented Dec 26, 2016

nicolas-grekas commented Dec 26, 2016 •

edited

Loading

ro0NL commented Dec 26, 2016 •

edited

Loading

Avoid warning in PHP 7.2 because of non-countable data #20859

Avoid warning in PHP 7.2 because of non-countable data #20859

Conversation

wouterj commented Dec 10, 2016

ogizanagi commented Dec 10, 2016

ogizanagi Dec 10, 2016

Choose a reason for hiding this comment

wouterj Dec 10, 2016

Choose a reason for hiding this comment

wouterj commented Dec 10, 2016

ogizanagi commented Dec 10, 2016

nicolas-grekas Dec 12, 2016

Choose a reason for hiding this comment

wouterj Dec 12, 2016

Choose a reason for hiding this comment

nicolas-grekas Dec 12, 2016

Choose a reason for hiding this comment

SpacePossum Dec 15, 2016

Choose a reason for hiding this comment

ro0NL Dec 16, 2016 • edited Loading

Choose a reason for hiding this comment

xabbuh commented Dec 25, 2016

wouterj commented Dec 25, 2016

fabpot commented Dec 26, 2016

nicolas-grekas commented Dec 26, 2016 • edited Loading

ro0NL commented Dec 26, 2016 • edited Loading

ro0NL Dec 16, 2016 •

edited

Loading

nicolas-grekas commented Dec 26, 2016 •

edited

Loading

ro0NL commented Dec 26, 2016 •

edited

Loading