Skip to content

Add ArrayObject::isAssoc()#58

Closed
leocavalcante wants to merge 3 commits into
swoole:masterfrom
leocavalcante:arr/is-assoc
Closed

Add ArrayObject::isAssoc()#58
leocavalcante wants to merge 3 commits into
swoole:masterfrom
leocavalcante:arr/is-assoc

Conversation

@leocavalcante

Copy link
Copy Markdown
Member

Checks whether the given array wrapped by ArrayObject is associative (uses string keys like a hashmap).

@deminy

deminy commented Oct 8, 2020

Copy link
Copy Markdown
Member

The Swoole team had discussed the implementations of this particular method two months ago. It's always inefficient when implemented in PHP, like the one implemented in Laravel ( https://github.com/laravel/framework/blob/v8.9.0/src/Illuminate/Collections/Arr.php#L389 ). Although there were thoughts like comparing keys in reverse order, it doesn't help much in worse cases.

Let's see how others think. Thanks @leocavalcante for the PRs!

@twose

twose commented Oct 13, 2020

Copy link
Copy Markdown
Member

The best way is php/php-src#6070

$index = count($array);
reverse_foreach($array as $key => void) {
    if ($key !== --$index) {
        return false;
    }
    return true;
}

@deminy

deminy commented Jan 31, 2021

Copy link
Copy Markdown
Member

According to this PHP RFC (Add array_is_list(array $array): bool), a new PHP function array_is_list() will be added to PHP 8.1. This new function can be found in the Docker image for PHP nightly (8.1-dev only):

docker run --rm -ti phpdaily/php:8.1-dev php -r 'echo function_exists("array_is_list") ? "yes" : "no", "\n";'

As discussed, the same function written in PHP is much less efficient compared to a built-in PHP function like array_is_list(). Thus, we prefer not to add method ArrayObject::isAssoc() to the Swoole library; developers can add and use this method (or some variations) manually in their projects when needed.

Thanks again to @leocavalcante for submitting the PR and for contributing to the open source community!

@deminy deminy closed this Jan 31, 2021
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.

3 participants