[5.4] Refactor pivot fix for PHP7.2 #20336
Conversation
| if (! is_null($ids = $this->parseIds($ids))) { | ||
| if (count((array) $ids) === 0) { | ||
| if (! is_null($ids)) { | ||
| $ids = $this->parseIds($ids); |
There was a problem hiding this comment.
👍 The only change of behavior here is for the case where you'd pass a Model instance that isn't saved yet (thus id = null).
There was a problem hiding this comment.
Hmmm... let me check.
What would a failing test for that look like? Would be good to capture it...
There was a problem hiding this comment.
Is that because your saying inside parseIds() that $value->getKey() might return null itself?
There was a problem hiding this comment.
What about use empty instead of count? Need to test if not gonna break
There was a problem hiding this comment.
@gmsantos - nice idea - that would work.
There was a problem hiding this comment.
@laurencei Yep that's exactly it however IMO that seems more like a bug than a feature so the change shouldn't matter... 😄
I think it makes more sense to keep
parseIds()as anarrayreturn (as it was 15mins ago before #20330) - and instead just remove all the casting that was occuring throughout the function.i.e. every time
parseIds()was called - it was being cast toarray- so we might as well just honor the original docblock intention and fix the bug inparseIds()itself.This also solves the
count()issue for PHP7.2, so that you only ever count anarray, and still honors thenullreturns for$idsif that situation occurs.All tests pass green pre/post this change.