[8.x] Collection prevent null being added to resulting collection with pop and shift

[craig-sen]

ISSUE

In this PR the ability to pop and shift a specified number of items from a collection was added.
However, if the supplied parameter is outside the bounds of the collection NULL values are added to the end of the collection.

    $collect = collect([1, 2, 3 , 4]);

    $collect->shift(6);

    // collect([1, 2, 3, 4, NULL, NULL])

this is also the same for negative parameters, and for when the collection is empty

changes in this pr

This PR adds checks to the parameter passed to the pop and shift methods and if out of bounds returns only the items in the collection. This is in line with the behaviour of splice and take which do not add null values if the parameter is out of bounds.

return null when the collection is empty and a parameter is passed to the pop() or shift() function

return null when the parameter is less than 1
(throwing an IllegalArgumentException is also a possiblity)

[morloderex]

This should probably be a new empty collection instead of null to allow for chaining?

[bert-w]

I think you can just use return;, there are 2 more spots in this file related to the functions median() and mode() which also have a null return. I'm not sure if returning a new collection is appropriate here when comparing it to the php array_pop()

[craig-sen]

as mentioned by @bert-w with php the functions array_pop and array_shift returning null in the case of an empty array, when the $count parameter is 1 this implementation seems appropriate.

However, in the case that there is a user supplied parameter which is not 1 things get a little bit more difficult...
Considering the current behaviour of the function it may be better to apply something similar to the following logic

if ($count === 1) {
    return object | null;
}

if ($count < 0 || $this->count() === 0) {
    return empty collection;
}

return $count item collection;

The more I think about it, it is probally more appropriate to seperate the funtionality into classic pop functionality returning object | null, and popMultiple which takes a parameter and returns a collection. (As this is already in production it would be a breaking change?)

[devcircus]

Current behavior seems reasonable. Either way, probably doesn't need to change before version 9.

[taylorotwell]

I agree with the comments that determining when to return null or an empty collection needs some work.

[driesvints]

This is a breaking change.

[craig-sen]

wow! that was a fast review!!!!

[driesvints]

This is a breaking change.

[craig-sen]

@taylorotwell @devcircus @morloderex @bert-w

Thanks for the feedback!

after thinking about it over the weekend, instead of having a special case for 1,returning the traditional item|null when a parameter was supplied and returning an (empty) collection in other situations made more sense to my way of thinking.

To make chaining simpler, I also decided to return an empty collection in the case the collection was empty or the parameter was less than 1 as popping from the start of the array seemed redundant (that's what shift is for). while I know that it is a breaking change, it makes things simpler than adding boolean logic into the mix.

Let me know what you think.

[taylorotwell]

I'll fix this on my end.

[taylorotwell]

bd89575