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

[Suggestion] Code split for Array Utility #3

Open
2 of 5 tasks
weierophinney opened this issue Dec 31, 2019 · 15 comments
Open
2 of 5 tasks

[Suggestion] Code split for Array Utility #3

weierophinney opened this issue Dec 31, 2019 · 15 comments

Comments

@weierophinney
Copy link
Member

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

Originally posted by @gabbydgab at zendframework/zend-stdlib#69

@weierophinney
Copy link
Member Author

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).


Originally posted by @Ocramius at zendframework/zend-stdlib#69 (comment)

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


Originally posted by @gabbydgab at zendframework/zend-stdlib#69 (comment)

@weierophinney
Copy link
Member Author

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


Originally posted by @Ocramius at zendframework/zend-stdlib#69 (comment)

@weierophinney
Copy link
Member Author

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


Originally posted by @gabbydgab at zendframework/zend-stdlib#69 (comment)

@weierophinney
Copy link
Member Author

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 ?


Originally posted by @gabbydgab at zendframework/zend-stdlib#69 (comment)

@weierophinney
Copy link
Member Author

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


Originally posted by @gabbydgab at zendframework/zend-stdlib#69 (comment)

@weierophinney
Copy link
Member Author

Yep, phpbench FTW.

As for the ccloc, ignore that for now.

On 7 Jan 2017 03:34, "Gab Amba" [email protected] 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
zendframework/zend-stdlib#69 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAJakIGt7gUhzh1grQ26nhlqgBh4XiNDks5rPvnOgaJpZM4Lc-Ku
.


Originally posted by @Ocramius at zendframework/zend-stdlib#69 (comment)

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

Originally posted by @gabbydgab at zendframework/zend-stdlib#69 (comment)

@weierophinney
Copy link
Member Author

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

I don't see the history/past commits.


Originally posted by @Ocramius at zendframework/zend-stdlib#69 (comment)

@weierophinney
Copy link
Member Author

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.


Originally posted by @Ocramius at zendframework/zend-stdlib#69 (comment)

@weierophinney
Copy link
Member Author

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


Originally posted by @gabbydgab at zendframework/zend-stdlib#69 (comment)

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


Originally posted by @gabbydgab at zendframework/zend-stdlib#69 (comment)

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


Originally posted by @gabbydgab at zendframework/zend-stdlib#69 (comment)

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

Originally posted by @gabbydgab at zendframework/zend-stdlib#69 (comment)

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

No branches or pull requests

1 participant