Make Stdlib's ArrayObject a soft dependency of ValueGenerator

[Maks3w]

Alternative to #22.

Note: PropertyReflection require stdlib PriorityQueue due the EventManager use.

Close: #22
Fix: #21

[stof]

I suggest removing stdlib from the suggest section now, as it is used only if your own code passes a stdlib ArrayObject to the ValueGenerator, which means it is used by your own code already.

[Maks3w]

Note: PropertyReflection require stdlib PriorityQueue due the EventManager use.

[stof]

@Maks3w PropertyReflection itself does not use the PriorityQueue. If the EventManager needs it internally, it is the job of the EventManager to declare it IMO

[weierophinney]

The event manager no longer uses it on the develop branch.
On Nov 16, 2015 9:15 AM, "Christophe Coevoet" notifications@github.com
wrote:

@Maks3w https://github.com/Maks3w PropertyReflection itself does not
use the PriorityQueue. If the EventManager needs it internally, it is the
job of the EventManager to declare it IMO

—
Reply to this email directly or view it on GitHub
#23 (comment)
.

[Maks3w]

TBH zend-eventmanager shouldn't be a required dependency. It's only used by the annotation sub component.

[Maks3w]

Ready for merge.

I've tagged this as BC Break because there is a minor change which affect to the return type of getConstants

[stof]

should this return a mixed type, or should it always return a SPL ArrayObject by converting StdLib ones ?

[Maks3w]

Return a different thing of Stdlib version is a BC Break.

Both objects share the same methods and signature so nobody should find the difference except if typehint the expected type.

[stof]

well, returning a SPL object in some cases is already a BC break (it is the only BC break btw).

If you keep using a mixed type for now, making the return type stricter in the future (to always use SPL) would be fine as it would not break any code using it, so the removal of StdLib return values could be done later in the future without BC break.

[weierophinney]

Returning SplArrayObject is less strict, as StdlibArrayObject is currently a superset (extension) of the SPL version. The BC break is that if you were expecting the Stdlib version, you're not getting it.

All that said: it's a moot point. The point of Zend\Stdlib\ArrayObject was to act as a polyfill to provide forward-compatibility from 5.3 and 5.4 to features present in the 5.5+ versions of PHP's SPL. Moreover, in this particular use case, it's the behavior of the returned object that is of interest: it acts like a stateful array.

As such, I will argue this is not a BC break in the least from a consumer point of view.

[weierophinney]

ARGH!!!! I stand corrected: Zend\Stdlib\ArrayObject does not extend ArrayObject. I forgot that we implemented our own version due to limitations in the PHP implementation. It has the same behavior, but a different inheritance chain.

As such, the approach in this PR is a bit suspect. Type-hinting on ArrayAccess requires an additional check later for ArrayObject or Zend\Stdlib\ArrayObject, because ValueGenerator consumes the methods getArrayCopy(), exchangeArray(), and append() from those two implementations (in addition to offsetUnset() from ArrayAccess, and supporting iteration).

I'd argue:

• No typehint for the argument (drop the ArrayAccess typehint, as it incorrectly indicates any ArrayAccess implementation will work).
• In the docblock, specify the two implementations as the typehint (\ArrayObject|\Zend\Stdlib\ArrayObject — as it is already)
• Check in the constructor for an instance of one of those two implementations (as is already being done).

With those changes, I'll gladly merge.

[nicolas-grekas]

would it be possible/desirable to change Zend\Stdlib\ArrayObject to extend \ArrayObject?

[Ocramius]

Would first need to dig through the framework and see where/why it was used: the initial use-case was getting rid of PHP's broken ArrayObject implementation (that's why the inheritance is not there).

[Maks3w]

No. Tests fail.

[weierophinney]

@Maks3w — what was that in reply to? using the SPL ArrayObject only? having Zend\Stdlib\ArrayObject extend ArrayObject? or removing the ArrayAccess typehint in the constructor?

[Maks3w]

Was in reply about to make Stdlib extend from Spl version

[Maks3w]

@weierophinney Typehint removed