Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[10.x] Fix collection items pop when count is 0 or less than 0. #51699

Closed
wants to merge 5 commits into from

Conversation

YazeedAlsaif
Copy link

@YazeedAlsaif YazeedAlsaif commented Jun 3, 2024

Hello

Building upon the improvements from Issue #51684 and in merged PR #51686; the pop method in Laravel's Collection class does not correctly handle cases where the $count parameter is zero or negative.

This proposal seeks to refine the method's behavior to:

  1. Return empty collection when $count is zero.
  2. Throw an InvalidArgumentException when $count is less than zero.

Current

$c = collect([10, 20, 30, 40]);
$c->pop(0); 
$c; // [10, 20]
$c = collect([10, 20, 30, 40]);
$c->pop(-1); 
$c; // [10]
$c = collect([10, 20, 30, 40]);
$c->pop(-2); 
$c; // []

Expected

$c = collect([10, 20, 30, 40]);
$c->pop(0); 
$c; // []
$c = collect([10, 20, 30, 40]);
$c->pop(-1);  // InvalidArgumentException('Number of popped items may not be less than zero.');
$c = collect([10, 20, 30, 40]);
$c->pop(-2); // InvalidArgumentException('Number of popped items may not be less than zero.');

I hope you find this helpful.

Thank you

@YazeedAlsaif YazeedAlsaif changed the title [10.x] Fix collection items pop with less than one item [10.x] Fix collection items pop with when count is 0 or less than 0. Jun 3, 2024
@YazeedAlsaif YazeedAlsaif changed the title [10.x] Fix collection items pop with when count is 0 or less than 0. [10.x] Fix collection items pop when count is 0 or less than 0. Jun 3, 2024
@rodrigopedra
Copy link
Contributor

Why is $c->pop(0); expected to return the whole array?

@rodrigopedra
Copy link
Contributor

Ah ok, from the code I see it returns an empty collection instead, that might have been an error on your examples

@YazeedAlsaif YazeedAlsaif marked this pull request as draft June 3, 2024 21:24
@YazeedAlsaif
Copy link
Author

YazeedAlsaif commented Jun 3, 2024

Ah ok, from the code I see it returns an empty collection instead, that might have been an error on your examples

Thank you, what is proposed indeed returns the whole collection when we $c->pop(0); since no items are popped. I have corrected that now.

@YazeedAlsaif YazeedAlsaif marked this pull request as ready for review June 3, 2024 22:09
@rodrigopedra
Copy link
Contributor

since no items are popped

I would not expect anything to be returned then, "since no items are popped"

When a greater than zero value is provided, items removed from the collection are returned. Why should anything be returned if no items were removed?

I would expect either to receive an empty array (in line with the recently merged PR for shift()), or preferably, to have an exception thrown.

@rodrigopedra
Copy link
Contributor

As for greater than zero values, all items returned are removed from the original collection, if all items are returned, when 0 is passed, then the original collection becomes empty?

YazeedAlsaif and others added 2 commits June 4, 2024 08:35
Fixing pop(0) and adding relevant tests.

Co-authored-by: Faissal Wahabali <[email protected]>
Adding tests.

Co-authored-by: Faissal Wahabali <[email protected]>
@YazeedAlsaif
Copy link
Author

Thank you @rodrigopedra @faissaloux, batched.

@taylorotwell
Copy link
Member

I don't want to mess with this method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants