Option for Collection->shift() to always return a collection

[mjomble]

The current behavior of Collection->shift() (since merging #51686) is:

• shift(0) returns a Collection
• shift(1) returns a single item ⚠️
• shift(2) returns a Collection
• shift(3) returns a Collection
• etc

This can easily lead to bugs (as just happened to me) if you pass a variable as the parameter, e.g. shift($count).
A current workaround is to use Collection::wrap($coll->shift($count)), but it's not immediately obvious that such a wrapper exists, or that it's necessary to use it.

I propose adding an optional second boolean parameter to shift() which forces it to always return a Collection, even when shifting exactly one element. So that:

• shift(0, true) returns a Collection
• shift(1, true) returns a Collection
• shift(2, true) returns a Collection
• shift(3, true) returns a Collection
• etc

whereas shift(1) remains backwards-compatible.

[wizzymore]

But why?

It must return the item it removes AS IS, that's what shifting always did no matter the language or the object it was on, it returns the value it removes.

[mjomble]

@henzeb are you saying my recommendation is "bullshit" or am I misreading your comment?

[henzeb]

The idea is good, just the extra parameter is bullshit. This must be a breaking change for laravel 12. A workaround until the might be a macro.

[mjomble]

The idea of this Macroable function is that when something doesn't act like you want it to, you can make it to.

The goal of this change is not to fix the bug in my project only. I have already done that and know to watch out for this from now on.
However, it seems likely to me that other developers may make the same assumption and end up with similar bugs.
The reason I started this discussion was to try to reduce the risk of that happening.

This must be a breaking change for laravel 12

I'm not sure if a breaking change is needed here. But I'm also not very familiar with Laravel's policy on breaking changes.

As a non-breaking alternative, what about keeping shift() as is and adding a new shiftMany() method that would always return a Collection?

PS. Laravel has a code of conduct which states, for example:

Participants must ensure that their language and actions are free of personal attacks and disparaging personal remarks

This is my first attempt at contributing an improvement to this project.
So far I've been told that the change, which I carefully designed to be backwards-compatible, is breaking (with a hint that my bug was 100% my fault and had nothing to do with the framework behaving surprisingly) and that a part of my proposal is bullshit.

I don't consider myself an easily-offended snowflake who can't handle any negative words. However, my free time is quite limited and I'm starting to feel like it would be better used elsewhere.

[henzeb]

well, I am not going into that kind of discussion again... the word bullshit is in no way an attack to you.

I agree with your idea that this has to be fixed, I just don't think we need a "backwards compatible" change here.

This is my final message in this thread.

[mjomble]

well, I am not going into that kind of discussion again... the word bullshit is in no way an attack to you.

Thanks for clarifying your intent 👍

I agree that it's pointless to argue over whether or not it is objectively an attack.
I also hope we can agree that even if you don't consider it an attack, some people may still subjectively interpret it as one (seems like this was not the first time it happened).

If you keep telling people their ideas are bullshit, some may interpret your intent correctly while others may not.
Whether you want to change your wording to avoid that is up to you.