Skip to content

[12.x] Use array_first and array_last #56706

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

Merged

Conversation

KIKOmanasijev
Copy link
Contributor

This PR complements #56703. It replaces all of the end and reset function calls with the new array_first and array_last functions from PHP 8.5 (used through Polyfil).

@@ -203,7 +203,7 @@ function data_forget(&$target, $key)
*/
function head($array)
{
return reset($array);
return array_first($array);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not exactly the same: reset() updates the internal array pointer to the first element, while array_first() does not update internal pointer. Additionally, reset() returns false if the array is empty, while array_first() returns null.

Same goes to the end() and array_last().

Copy link
Contributor

Choose a reason for hiding this comment

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

The concern about the internal pointer changing is irrelevant to most of these changes, as arrays in PHP are passed into functions as copies, and the use of reset() or end() happens, mostly, in the return statement.

I say "most of the cases," as I didn't thoroughly check every change. But regardless of being used on the return statement, all changes seem fine overall.

The internal pointer change would be relevant if arrays were explicitly being passed as references, which is not the case in any of the changes.

<?php

function byCopy($arr) { return reset($arr); }
function byReference(&$arr) { return end($arr); }

$arr = [1, 2, 3];

// move internal pointer to the second element
next($arr);

echo 'current element: ', current($arr), PHP_EOL, PHP_EOL;

echo 'result of byCopy(): ', byCopy($arr), PHP_EOL;
echo 'original array pointer should be unchanged: ', current($arr), PHP_EOL, PHP_EOL;

echo 'result of byReference(): ', byReference($arr), PHP_EOL;
echo 'original array pointer should be changed: ', current($arr), PHP_EOL;

Output:

$ php test.php 
current element: 2

result of byCopy(): 1
original array pointer should be unchanged: 2

result of byReference(): 3
original array pointer should be changed: 3

But indeed the false on an empty array case would be a breaking change for anyone already handling that outcome.

Maybe it is best to send this to master and document the change on the upgrade guide to avoid any surprises for developers handling the false on an empty array case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I appreciate the explanations! I now realize that head() and last() are the only real cases that introduce breaking changes. I still think their implementation should use array_first() and array_last(), since the purpose of these helpers is simply to return the first or last element of an array. I agree though that these 2 should target the master branch, and not 12.x. I can create another PR for them.

The other changes don’t introduce breaking changes, since they dont rely on the internal pointer or the special false return value, so I believe those can safely target the 12.x branch.

Copy link
Contributor

Choose a reason for hiding this comment

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

Even in cases where the array is a reference, not manipulating the array or its internal pointer would be the expected default behavior. So if anything, this actually fixes potentially non-apparent behavior. But as always, someone might rely on this and sending it to master instead might make sense here therefore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally agree with @shaedrich,, that explains basically I still think their implementation should use array_first() and array_last() from my previous message 😅.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for explanation @rodrigopedra. Indeed, I checked other usages and it seems to not impact userland except for false -> null return on empty array.
While the contract is not clearly established (return mixed) in the PHPDoc, documentation states:

The head function returns the first element in the given array. If the array is empty, false will be returned:

https://laravel.com/docs/12.x/helpers#method-head

Which should be considered a minor BC break and requires doc update. It also appears that this edge case is not covered by tests.

@taylorotwell
Copy link
Member

Can't have a breaking change on the head method on a patch release. Can you fix it to return false on empty array?

@taylorotwell taylorotwell marked this pull request as draft August 22, 2025 13:40
@KIKOmanasijev
Copy link
Contributor Author

Can't have a breaking change on the head method on a patch release. Can you fix it to return false on empty array?

@taylorotwell Done it for both the head and the last methods.

@KIKOmanasijev KIKOmanasijev marked this pull request as ready for review August 22, 2025 13:51
@taylorotwell taylorotwell merged commit f73517f into laravel:12.x Aug 22, 2025
60 checks passed
@KIKOmanasijev KIKOmanasijev deleted the feat/use-new-arr-helper-functions branch August 22, 2025 14:31
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