Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stop warnings when using count in QueryCompiler in PHP 7.2 #10864

Merged
merged 1 commit into from
Jul 7, 2017

Conversation

MCF
Copy link
Contributor

@MCF MCF commented Jul 5, 2017

  • PHP 7.2 has changed the count function to emit warnings when the object
    being "counted" is not an array or does not implement the Countable
    interface. The QueryCompiler was using count for objects that did not
    fall into those two categories. This fixes that problem while
    maintaining the same logic (returning early when $parts is not set, is
    null, or has a count of 0).

Fixes #10862

- PHP 7.2 has changed the count function to emit warnings when the object
  being "counted" is not an array or does not implement the Countable
  interface.  The QueryCompiler was using count for objects that did not
  fall into those two categories.  This fixes that problem while
  maintaining the same logic (returning early when $parts is not set, is
  null, or has a count of 0).
@@ -125,7 +125,9 @@ public function compile(Query $query, ValueBinder $generator)
protected function _sqlCompiler(&$sql, $query, $generator)
{
return function ($parts, $name) use (&$sql, $query, $generator) {
if (!count($parts)) {
if (!isset($parts) ||
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't just checking if (empty($parts)) work?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not for the full set of conditionals (if that was the question?).
Objects with Countable interfaces and count() == 0 are not considered "empty" by the empty function.

Also I am mimicing the old conditional's (possible) reliance on count returning 1 when an object was not an array or Countable. In other words I know that this new conditional acts exactly the same as !count($parts) but without the warning emitted.

Copy link
Contributor Author

@MCF MCF Jul 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a follow on, $parts can be an integer (I've dumped out the type of parts so this is not a hypothetical, sometimes an integer is passed in). So the following case shows why empty would be a change from current behavior:

$parts = 0;
!count($parts)   // ==  false
empty($parts)    // == true

(edit - fixed the comment in my little example above - but the end result is still the same !count($parts) != empty($parts) in some cases)

@codecov-io
Copy link

codecov-io commented Jul 5, 2017

Codecov Report

Merging #10864 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #10864      +/-   ##
============================================
+ Coverage     94.91%   94.92%   +<.01%     
- Complexity    12123    12126       +3     
============================================
  Files           422      422              
  Lines         30142    30143       +1     
============================================
+ Hits          28610    28613       +3     
+ Misses         1532     1530       -2
Impacted Files Coverage Δ Complexity Δ
src/Database/QueryCompiler.php 99.06% <100%> (ø) 45 <0> (+3) ⬆️
src/Cache/CacheEngine.php 93.47% <0%> (+2.17%) 19% <0%> (ø) ⬇️
src/Cache/CacheRegistry.php 100% <0%> (+4.16%) 11% <0%> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4bc6a54...2c958f6. Read the comment docs.

@markstory markstory added this to the 3.4.10 milestone Jul 6, 2017
@markstory markstory added the ORM label Jul 6, 2017
@markstory markstory self-assigned this Jul 6, 2017
@markstory markstory merged commit 7d5719e into cakephp:master Jul 7, 2017
@MCF MCF deleted the php72_count_fix branch July 7, 2017 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PHP 7.2 has changed count's behavior causing problems with QueryCompiler
5 participants