-
Notifications
You must be signed in to change notification settings - Fork 11.7k
[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
Changes from 5 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -369,7 +369,7 @@ public function scalar($query, $bindings = [], $useReadPdo = true) | |
| throw new MultipleColumnsSelectedException; | ||
| } | ||
|
|
||
| return reset($record); | ||
| return array_last($record); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here,
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here it seems to be an oversight. I guess it should be fixed. Especially as the method description reads as "Run a select statement and return the first column of the first row." ping @KIKOmanasijev |
||
| } | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -507,7 +507,7 @@ public function fillForInsert(array $values) | |
| return []; | ||
| } | ||
|
|
||
| if (! is_array(reset($values))) { | ||
| if (! is_array(array_first($values))) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here,
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here it is fine. |
||
| $values = [$values]; | ||
| } | ||
|
|
||
|
|
@@ -1265,12 +1265,12 @@ public function upsert(array $values, $uniqueBy, $update = null) | |
| return 0; | ||
| } | ||
|
|
||
| if (! is_array(reset($values))) { | ||
| if (! is_array(array_first($values))) { | ||
| $values = [$values]; | ||
| } | ||
|
|
||
| if (is_null($update)) { | ||
| $update = array_keys(reset($values)); | ||
| $update = array_keys(array_first($values)); | ||
| } | ||
|
|
||
| return $this->toBase()->upsert( | ||
|
|
@@ -1366,7 +1366,7 @@ protected function addUpdatedAtColumn(array $values) | |
|
|
||
| $segments = preg_split('/\s+as\s+/i', $this->query->from); | ||
|
|
||
| $qualifiedColumn = end($segments).'.'.$column; | ||
| $qualifiedColumn = array_last($segments).'.'.$column; | ||
|
|
||
| $values[$qualifiedColumn] = Arr::get($values, $qualifiedColumn, $values[$column]); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -116,7 +116,7 @@ protected function setForeignAttributesForCreate(Model $model) | |
| */ | ||
| public function upsert(array $values, $uniqueBy, $update = null) | ||
| { | ||
| if (! empty($values) && ! is_array(reset($values))) { | ||
| if (! empty($values) && ! is_array(array_last($values))) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here it also seems to be swapped. IMO it should be |
||
| $values = [$values]; | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3130,7 +3130,7 @@ public function value($column) | |
| { | ||
| $result = (array) $this->first([$column]); | ||
|
|
||
| return count($result) > 0 ? reset($result) : null; | ||
| return count($result) > 0 ? array_first($result) : null; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -3142,7 +3142,7 @@ public function rawValue(string $expression, array $bindings = []) | |
| { | ||
| $result = (array) $this->selectRaw($expression, $bindings)->first(); | ||
|
|
||
| return count($result) > 0 ? reset($result) : null; | ||
| return count($result) > 0 ? array_first($result) : null; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -3158,7 +3158,7 @@ public function soleValue($column) | |
| { | ||
| $result = (array) $this->sole([$column]); | ||
|
|
||
| return reset($result); | ||
| return array_last($result); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Many in this file have had Is it intentional? |
||
| } | ||
|
|
||
| /** | ||
|
|
@@ -3781,7 +3781,7 @@ public function insert(array $values) | |
| return true; | ||
| } | ||
|
|
||
| if (! is_array(reset($values))) { | ||
| if (! is_array(array_last($values))) { | ||
| $values = [$values]; | ||
| } | ||
|
|
||
|
|
@@ -3818,7 +3818,7 @@ public function insertOrIgnore(array $values) | |
| return 0; | ||
| } | ||
|
|
||
| if (! is_array(reset($values))) { | ||
| if (! is_array(array_last($values))) { | ||
| $values = [$values]; | ||
| } else { | ||
| foreach ($values as $key => $value) { | ||
|
|
@@ -3976,7 +3976,7 @@ public function upsert(array $values, array|string $uniqueBy, ?array $update = n | |
| return (int) $this->insert($values); | ||
| } | ||
|
|
||
| if (! is_array(reset($values))) { | ||
| if (! is_array(array_last($values))) { | ||
| $values = [$values]; | ||
| } else { | ||
| foreach ($values as $key => $value) { | ||
|
|
@@ -3987,7 +3987,7 @@ public function upsert(array $values, array|string $uniqueBy, ?array $update = n | |
| } | ||
|
|
||
| if (is_null($update)) { | ||
| $update = array_keys(reset($values)); | ||
| $update = array_keys(array_last($values)); | ||
| } | ||
|
|
||
| $this->applyBeforeQueryCallbacks(); | ||
|
|
||
There was a problem hiding this comment.
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, whilearray_first()does not update internal pointer. Additionally,reset()returnsfalseif the array is empty, whilearray_first()returnsnull.Same goes to the
end()andarray_last().There was a problem hiding this comment.
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()orend()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.
Output:
But indeed the
falseon an empty array case would be a breaking change for anyone already handling that outcome.Maybe it is best to send this to
masterand document the change on the upgrade guide to avoid any surprises for developers handling thefalseon an empty array case.There was a problem hiding this comment.
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()andlast()are the only real cases that introduce breaking changes. I still think their implementation should usearray_first()andarray_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 themasterbranch, and not12.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.xbranch.There was a problem hiding this comment.
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
masterinstead might make sense here therefore.There was a problem hiding this comment.
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 😅.There was a problem hiding this comment.
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->nullreturn on empty array.While the contract is not clearly established (
return mixed) in the PHPDoc, documentation states: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.