[Suggestion] Code split for Array Utility

gabbydgab commented 7 years ago

Looking at some dependency graph for zend components, one way of requiring zend-stdlib is to use the array functions.

Some of these libraries are:

Zend\Config
Zend\Router
Zend\View
Zend\ConfigAggregator - (copy-pasted code because it ONLY needs the array utility)

Separating this may have a relevant value to packages that utilizes array functions such as the framework itself since the default configuration is in array.

Ported repository: https://github.com/php-refactoring/zend-array-utility

[X] Supports PHP 5.6
[X] Added QA checks like Zend coding standard and license header validation

Pros:

Be able to upgrade the array utility classes, such as array sort, merge, find, etc.
Will be able to stress test and benchmark all scenarios.
We can have separate documentation on how to use this library.
Zend Components and other packages that requires zend-stblib but not using the Array* classes will not be (that much) affected.

Cons:

Zend components and other packages that requires zend-stblib and uses the Array* classes will be affected. Thus, it will require zendframework/zend-array-utility as dependecy for zend-stdlib to be BC compatible.

To do:

[ ] Include the array utility package as zendframework/zend-array-utility as dependecy for zend-stdlib
[ ] Integration testing for components requiring zend-stdlib and uses the Array* classes
[ ] Dedicated documentation for array utilities

gabbydgab commented 7 years ago

Addendum:

This will also address to issues like:

Ocramius commented 7 years ago

This seems like a good idea to me, since I generally require Zend\Stdlib just for some array processing and queue stuff.

I still want to drop PHP 5 support, especially considering that the array utils API didn't change over the past year, but I'll probably end up in a fighting pit with @weierophinney :-P

@gabbydgab there is still a massive issue with your repository though: you didn't import the original commits, which are in fact really important for tracing bugs and for attribution.

Also, a benchmark test suite us absolutely necessary from now on (see https://github.com/zendframework/zend-servicemanager/pull/64).

gabbydgab commented 7 years ago

@Ocramius just copy-pasted it as of the moment, but if you can guide me on how to properly subsplit it from the upstream then I'll redo it.

Ocramius commented 7 years ago

@gabbydgab just cloning this repo and using git rm and git mv to change paths is sufficient ;-)

gabbydgab commented 7 years ago

@Ocramius updated the repository link and retains the copy-pasted codes here

gabbydgab commented 7 years ago

In addition to the QA toolkit, here's the output when I try check code quality using PHP Mess Detector

> phpmd src text codesize,unusedcode,naming,design,controversial
/var/www/projects/ZendFramework/ArrayUtility/src/ArrayObject.php:22     The class ArrayObject has 20 public methods. Consider refactoring ArrayObject to keep number of public methods under 10.
/var/www/projects/ZendFramework/ArrayUtility/src/ArrayObject.php:22     The class ArrayObject has an overall complexity of 52 which is very high. The configured complexity threshold is 50.
/var/www/projects/ZendFramework/ArrayUtility/src/ArrayObject.php:409    Avoid variables with short names like $ar. Configured minimum length is 3.
/var/www/projects/ZendFramework/ArrayUtility/src/ArrayUtils.php:21      The class ArrayUtils has an overall complexity of 51 which is very high. The configured complexity threshold is 50.
/var/www/projects/ZendFramework/ArrayUtility/src/ArrayUtils.php:216     The method iteratorToArray() has a Cyclomatic Complexity of 10. The configured cyclomatic complexity threshold is 10.
/var/www/projects/ZendFramework/ArrayUtility/src/ArrayUtils.php:269     The method merge() has a Cyclomatic Complexity of 11. The configured cyclomatic complexity threshold is 10.
/var/www/projects/ZendFramework/ArrayUtility/src/ArrayUtils.php:269     Avoid variables with short names like $a. Configured minimum length is 3.
/var/www/projects/ZendFramework/ArrayUtility/src/ArrayUtils.php:269     Avoid variables with short names like $b. Configured minimum length is 3.
Script phpmd src text codesize,unusedcode,naming,design,controversial handling the phpmd event returned with error code 2

PS: Didn't change anything yet. It's all based on the current upstream. Also I didn't removed yet the Parameter* classes due to this hard dependency.

Thoughts, @Ocramius @weierophinney ?

gabbydgab commented 7 years ago

since athletic/athletic is no longer maintained, shall we use phpbench/phpbench?

Ocramius commented 7 years ago

Yep, phpbench FTW.

As for the ccloc, ignore that for now.

On 7 Jan 2017 03:34, "Gab Amba" notifications@github.com wrote:

since athletic/athletic https://github.com/polyfractal/athletic is no longer maintained, shall we use phpbench/phpbench https://github.com/phpbench/phpbench?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/zendframework/zend-stdlib/issues/69#issuecomment-271056947, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJakIGt7gUhzh1grQ26nhlqgBh4XiNDks5rPvnOgaJpZM4Lc-Ku .

gabbydgab commented 7 years ago

Created concrete classes for IteratorToArray method to be backwards compatible for zend-stdlib:

SimpleIterator.php - not commonly used
RecursiveIterator.php - mostly used

It resolves ccloc for ArrayUtils::iteratorToArray() method.

public static function iteratorToArray($iterator, $recursive = true)
{
    if (! is_array($iterator) && ! $iterator instanceof Traversable) {
        throw new Exception\InvalidArgumentException(__METHOD__ . ' expects an array or Traversable object');
    }

    if (! $recursive) {
        return SimpleIterator::toArray($iterator);
    }

    return RecursiveIterator::toArray($iterator);
}

TO DO:

[ ] Provide benchmarks for the new Iterator classes
[ ] Documentation for the new Iterator classes
[ ] Additional test for ArrayUtils::iteratorToArray() that covers non-recursive interation

Ocramius commented 7 years ago

@Ocramius updated the repository link and retains the copy-pasted codes here

I don't see the history/past commits.

Ocramius commented 7 years ago

It resolves ccloc for ArrayUtils::iteratorToArray() method.

Please don't do that - the CCLOC is there due to past benchmarks. If you are doing a split, just do that and add issues for further improvements later.

gabbydgab commented 7 years ago

@Ocramius

The commits are in this repository: https://github.com/php-refactoring/zend-array-utility (develop branch)

Please don't do that - the CCLOC is there due to past benchmarks. If you are doing a split, just do that and add issues for further improvements later.

I will not touch that in the upstream (zend-stdlib); just want to demonstrate that the created classes (SimpleIterator and RecursiveIterator) can be used on the current logic.

Upstream code: ArrayUtils::iteratorToArray() method

    public static function iteratorToArray($iterator, $recursive = true)
    {
        if (! is_array($iterator) && ! $iterator instanceof Traversable) {
            throw new Exception\InvalidArgumentException(__METHOD__ . ' expects an array or Traversable object');
        }
        if (! $recursive) {
            if (is_array($iterator)) {
                return $iterator;
            }
            return iterator_to_array($iterator);
        }
        if (method_exists($iterator, 'toArray')) {
            return $iterator->toArray();
        }
        $array = [];
        foreach ($iterator as $key => $value) {
            if (is_scalar($value)) {
                $array[$key] = $value;
                continue;
            }
            if ($value instanceof Traversable) {
                $array[$key] = static::iteratorToArray($value, $recursive);
                continue;
            }
            if (is_array($value)) {
                $array[$key] = static::iteratorToArray($value, $recursive);
                continue;
            }
            $array[$key] = $value;
        }
        return $array;
    }

Will be simplified to use the Iterator classes in the proposed Zend\ArrayUtils package for BC compatibility

    public static function iteratorToArray($iterator, $recursive = true)
    {
        if (! $recursive) {
            return SimpleIterator::toArray($iterator);
        }
        return RecursiveIterator::toArray($iterator);
    }

I'm thinking that, if applied, will set as deprecated method then suggest to use the appropriate array utily (Zend\ArrayUtils) package moving forward.

gabbydgab commented 7 years ago

Also, I've added some test (from the current ArrayUtilsTest) for RecursiveIteratorTest but there are scenarios that are not fully covered - like instance of Traversable and $array[$key] = $value; at the end of the foreach.

Can't find additional specifications on the tests and benchmark. Can you provide some guidance on this matter? @Ocramius

gabbydgab commented 7 years ago

@Ocramius @weierophinney

I've updated the repository and created a release tag (0.1.x) as extracted array utility functions from the current zend-stdlib library.

Added initial benchmarks for:

PS: sample data for the benchmark is based on the current test data.

Optimization and extraction of functionalities will be next.

gabbydgab commented 7 years ago

Created issue for feature request: Checks if the provided array configuration is cache-able;

One of the best practices being taught is to use factories over closures - since closures are not cache-able.

practical usage:

private function cacheConfig(array $config, $cachedConfigFile)
{
    if (!ArrayUtils::isCachable($config)) {
          throw new InvalidArgumentException('Cannot cached config from %s; does not return array', $config);
    }

    // cache the config
}

weierophinney commented 4 years ago

This repository has been closed and moved to laminas/laminas-stdlib; a new issue has been opened at https://github.com/laminas/laminas-stdlib/issues/3.

zendframework / zend-stdlib

[Suggestion] Code split for Array Utility #69