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

[8.x] Add Arr::isList() method #39277

Merged
merged 1 commit into from
Oct 20, 2021
Merged

[8.x] Add Arr::isList() method #39277

merged 1 commit into from
Oct 20, 2021

Conversation

PHLAK
Copy link
Contributor

@PHLAK PHLAK commented Oct 20, 2021

Added the Arr::isList() method for determining if an array is a "list". An array is a "list" if all array keys are sequential integers starting from 0 with no gaps in between.

This is a polyfilled version of the array_is_list() function that is coming to PHP with v8.1 (currently unreleased). Additionally, Laravel 8 supports PHP versions back to 7.3 so this PR brings this functionality to applications still on these versions.

Implementation

This implementation uses the polyfill code as described in the RFC proposal which avoids potential issues when using other methods (e.g. array_values($array) === $array) for determining "listness".

Future Considerations

In the future this method can be refactored to use the native array_is_list() function when support for PHP versions < 8.1 are dropped. Alternatively the method may be removed all together.

@taylorotwell
Copy link
Member

Can you give an example where ! Arr::isAssoc(...) wouldn't work correctly for this use case?

@JacekAndrzejewski
Copy link

JacekAndrzejewski commented Oct 20, 2021

@taylorotwell The RFC describes cases where obvious implementation (which is used in isAssoc) won't work. Those would be rare, but technically could happen and would be hard to debug.

// Pitfalls of simpler polyfills - NAN !== NAN
$x = ['key' => 2, NAN];
unset($x['key']);
var_export($x === array_values($x));  // false because NAN !== NAN
var_export($x);  // array (0 => NAN)
var_export(array_is_list($x));  // true because keys are consecutive integers starting from 0

@taylorotwell
Copy link
Member

taylorotwell commented Oct 20, 2021

Arr::isAssoc does not use array_values so just want to confirm.

@taylorotwell
Copy link
Member

CleanShot 2021-10-20 at 12 30 47@2x

@JacekAndrzejewski
Copy link

That's what happens when you read code tired -_-
You're right. And I can't come up with any case where it wouldn't work, as keys are cast and no NAN can be a key. Calling array_keys twice will always create an array of numbers 0 to n, and one call will always create array of strings or ints. So comparison will work as expected every time.

One thing to consider would be performance for huge arrays, and without tests I can't even give a guess which would be faster. Probably if array is million of elements long and last key is wrong then array_keys would be faster.

@taylorotwell
Copy link
Member

taylorotwell commented Oct 20, 2021

@PHLAK would it be better to just convert this method to return ! Arr::hasList($array) until we can use array_is_list?

@PHLAK
Copy link
Contributor Author

PHLAK commented Oct 20, 2021

@taylorotwell I assume you meant return ! Arr::isAssoc($array). This would be fine as long as we can't identify a case that breaks this (I can't). I went ahead and made this change.

@taylorotwell taylorotwell merged commit 41ac7e8 into laravel:8.x Oct 20, 2021
@GrahamCampbell GrahamCampbell changed the title Add Arr::isList() method [8.x] Add Arr::isList() method Oct 20, 2021
@PHLAK PHLAK deleted the arr-is-list branch October 21, 2021 22:18
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.

None yet

3 participants