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 handle shift() on an empty collection #51841

Conversation

Treggats
Copy link
Contributor

fixes #51840

This PR fixes how the Collection::shift() method handles an empty collection.
Currently it will return the collection when trying to call shift() on an empty collection. Before it would return null.

steps to reproduce

This is also covered in the added test.

  1. create a collection with two objects
  2. call shift() three times on it and assign it to variables
  3. dump a property of the object for every variable
  4. it will throw an exception saying it can not call the property on a collection

@@ -1134,7 +1134,11 @@ public function shift($count = 1)
throw new InvalidArgumentException('Number of shifted items may not be less than zero.');
}

if ($count === 0 || $this->isEmpty()) {
if ($this->isEmpty()) {
Copy link
Contributor Author

@Treggats Treggats Jun 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the correct placement for the isEmpty() check. We want to make sure that there are items to use before we continue to handle the rest.

Also this maintains the behaviour as it was prior to the mentioned PR.

@taylorotwell taylorotwell merged commit 16472d1 into laravel:10.x Jun 19, 2024
24 checks passed
@Treggats Treggats deleted the bugfix/10.x-collection-shift-handle-zero-items branch June 19, 2024 14:28
@nvahalik
Copy link

Yikes. This has broken at least one 3rd party package that depending on shift() returning null rather than an empty collection! laravel-json-api/laravel#287

@Treggats
Copy link
Contributor Author

Yikes. This has broken at least one 3rd party package that depending on shift() returning null rather than an empty collection! laravel-json-api/laravel#287

Same, our tests started failing. So hence this PR. Just waiting on the release. In the mean time we updated our codebase to use the native array_shift(..)

@driesvints
Copy link
Member

I just cut a new release that includes this one.

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.

5 participants