[5.4] Cast ids to array #20330
[5.4] Cast ids to array #20330
Conversation
| @@ -349,7 +349,7 @@ public function detach($ids = null, $touch = true) | |||
| // associations, otherwise all of the association ties will be broken. | |||
| // We'll return the numbers of affected rows when we do the deletes. | |||
| if (! is_null($ids = $this->parseIds($ids))) { | |||
| if (count($ids) === 0) { | |||
| if (count((array) $ids) === 0) { | |||
There was a problem hiding this comment.
I'm confused. Why does the null check prior to this line not stop us getting here in the first place?
If it's not null - then it should be a countable result?
There was a problem hiding this comment.
Actually - this is not an error here. It's in ParseIds() - since it should always return an array?
There was a problem hiding this comment.
It's failing on master, check failed test 23) https://travis-ci.org/laravel/framework/jobs/259204796 https://github.com/laravel/framework/blob/master/tests/Integration/Database/EloquentBelongsToManyTest.php#L179 is passing an integer to the function, integer also isn't considered a countable result.
There was a problem hiding this comment.
Yep - see my comment below. IMO it should be fixed in a different way.
There was a problem hiding this comment.
Yes, it makes no sense that we would even hit this line with null?
There was a problem hiding this comment.
@taylorotwell - we worked it out. The parseIds() function returns mixed, not array().
So you can end up with count(1) if it was just a single model id - which will now fail in PHP7.2, because 1 (the int) is not countable.
|
edit: disregard - now fixed. |
|
Actually that's the correct fix, changing parseIds will make it not pass the if clause anymore (it'll never be a null value), I've changed the docblock then. |
* Fix travis CI error * Cast ids to array * Revert cast in favor of #20330 * Always remove xdebug
This PR fixes another call to count that is throwing an exception on PHP7.2