Wrong Collection->shift() fix was made (Attempt #3)

[ALaDST]

Laravel Version

11.34.2

PHP Version

8.3.3

Database Driver & Version

No response

Description

(This is the follow-up issue to #53761 and #53773)

Dear Laravel-Team,

I develop on my own system with framework version 10.45.1. But I encountered some error on my external production system (framework version 10.48.23), which was not happening on my developer system.

This issue is still persistent in the current version of the Laravel Framework (11.34.2). So it doesn't matter whether I encounter it in version 10. The error is still out there and it's ok if you fix it in the current version, as long as you just fix it. Would be great if you could also fix it in version 10 where it was produced, but if this is against your policies, then we have to check whether we can update to version 11.

By tracing down the error, I found the reason by a wrong fix you made to the "src\Illuminate\Collections\Collection.php".
Came from this PR: #51841
Regarding this original issue: #51840

Since the first implementation of multi shift (and multi pop (!! important later on)) in 2021-07-21 from "Ed Grosvenor", it was always the case that if you request more than 1 item to be shifted, that you will always get a collection as the result. Even if no items are returned. Then you got an empty collection but not null. If you request a single item to be shifted, only then you got null if the collection was empty.

This changed in 2024-06-03 by "Faissal Wahabali" during his commit:

* [10.x] Fix collection shift less than one item (#51686)
* fix collection shift less than 1
* throw exception earlier
* Update Collection.php
---------
Co-authored-by: Taylor Otwell <[email protected]>

(8154eb6)

Here the function workflow changed conceptionally. Besides some new error handling the function now returns an empty collection as a shift-result if the collection is empty. That's correct for a multi-shift, but wrong for a single-shift operation. So here happened the real error.

But on 2024-06-19 "Tonko Mulder" "miss-fixed" this during his commit:

* [10.x] fix handle `shift()` on an empty collection (#51841)
* add test for collection shift on a empty collection
* fix collection shift when dealing with an empty collection
* place the `isEmpty()` check before the count check
* update naming and assert the actual values

(16472d1)

He changed the behaviour mostly back, so that there will be always null returned, if the collection is empty. That's wrong for a multi-shift, but correct for a single-shift operation. So the original error is still open and not fixed. There were only new bugs introduced.

Besides that shift-function stuff, there is still the pop-function left. Which is the counterpart to shift and still works the old (correct) way. So if you use pop, you will get null as a result if the collection is empty if it is a single-pop operation. And you will always get a collection as a result if it is a multi-pop. So pop is working fine but different from shift now.

So even if you say, that the new shift-behaviour is correct (which I doubt), then pop would be wrong.

So please revoke your previous fix and make a new one. Here are the test cases that I would expect to be correct (considering your version-flow (We are working since several years with Laravel and started way back with version 8/9))

collect()->shift(); // should be null
collect()->pop(); // should be null

collect()->shift(3); // should be an empty collection [] <--------- Is null at the moment (which is wrong in my opinion)
collect()->pop(3); // should be an empty collection []

collect()->shift(0); // should be an empty collection []
collect()->pop(0); // should be an empty collection []

$c = collect(['a', 'b']);
$c->shift(); // should be 'a'
$c->shift(); // should be 'b'
$c->shift(); // should be null

$c = collect(['a', 'b']);
$c->pop(); // should be 'b'
$c->pop(); // should be 'a'
$c->pop(); // should be null

collect(['a', 'b'])->shift(3); // should be the collection ['a', 'b']
collect(['a', 'b'])->pop(3); // should be the collection ['a', 'b']

collect()->shift(-1); // should be an exception
collect()->pop(-1); // should be an exception

Kind regards

Steps To Reproduce

collect()->shift(1); // should be null
collect()->shift(3); // should be an empty collection [] <--------- Is null at the moment (which is wrong in my opinion)