Skip to content
This repository has been archived by the owner on Jan 31, 2020. It is now read-only.

[Suggestion] Code split for Array Utility #69

Open
2 of 5 tasks
gabbydgab opened this issue Jan 6, 2017 · 16 comments
Open
2 of 5 tasks

[Suggestion] Code split for Array Utility #69

gabbydgab opened this issue Jan 6, 2017 · 16 comments

Comments

@gabbydgab
Copy link

gabbydgab commented Jan 6, 2017

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:

  1. Zend\Config
  2. Zend\Router
  3. Zend\View
  4. 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

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
@Ocramius
Copy link
Member

Ocramius commented Jan 6, 2017

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 zendframework/zend-servicemanager#64).

@gabbydgab
Copy link
Author

@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
Copy link
Member

Ocramius commented Jan 6, 2017

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

@gabbydgab
Copy link
Author

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

@gabbydgab
Copy link
Author

gabbydgab commented Jan 7, 2017

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
Copy link
Author

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

@Ocramius
Copy link
Member

Ocramius commented Jan 7, 2017 via email

@gabbydgab
Copy link
Author

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

  1. SimpleIterator.php - not commonly used
  2. 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
Copy link
Member

Ocramius commented Jan 7, 2017

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

I don't see the history/past commits.

@Ocramius
Copy link
Member

Ocramius commented Jan 8, 2017

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
Copy link
Author

gabbydgab commented Jan 8, 2017

@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
Copy link
Author

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
Copy link
Author

@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:

  1. ArrayFilter
  2. ArrayMerge
  3. ArrayValidator
  4. IteratorToArray Converter

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

Optimization and extraction of functionalities will be next.

@gabbydgab
Copy link
Author

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
Copy link
Member

This repository has been closed and moved to laminas/laminas-stdlib; a new issue has been opened at laminas/laminas-stdlib#3.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants