search

zendframework / zend-stdlib

Stdlib component from Zend Framework
BSD 3-Clause "New" or "Revised" License
384 stars 76 forks source link

Accept Traversables as ArrayUtils::merge arguments #86

Closed gszy closed 6 years ago

gszy commented 6 years ago

ArrayUtils::merge should be capable of merging both arrays and Traversables (that’s the iterable typehint). Actually it’s used to merge array|Traversable in Zend\Session\SessionManager (see https://github.com/zendframework/zend-session/blob/release-2.8.0/src/SessionManager.php#L136).

This patch removes array typehint from ArrayUtils::merge parameters. Once at least PHP 7.1 is zend-stdlib’s requirement, iterable typehints should be added. I’ve also added Zend\Stdlib\Exception\InvalidArgumentException “import”, because it’s used a few times in the code.

Xerkus commented 6 years ago

:-1: from me on this change. Merging iterators has a potential for side effects. Not every iterable is a configuration provider. Merging traversable services in application configuration, for example, would be a bug.

If concrete consumer of array utils allows traversables, it should do conversion itself.

gszy commented 6 years ago

Alternative approach would be to fix SessionManager so that it’ll pass array to the ArrayUtils::merge.

Update: see gscscnd/zend-session@393ef70f6416ecaaaa96c51999ac75a19e77cd13 zendframework/zend-session#93

Xerkus commented 6 years ago

It was fixed in session manager.