jsonmapper

jsonmapper copied to clipboard

This PR fixes issue introduced by PR #197.

PR #197 replaced ArrayObject check with more broad ArrayAccess, thus breaking BC. This PR added additional check for Traversable to make sure given item is a collection and not a simple array-like object. Every collection implementation I know implements either Iterator or IteratorAggregate (both are Traversable descendants). Moreover, sample collection from original issue #175 implements Iterator too, so I can state with great confidence that this PR won't introduce more BC breaks.

Thank you for the patch. This is indeed a BC break that I did not think about. Given that the release that introduced it is already 10 months old, I will not publish it in a patch level release: This patch does introduce a BC break for people that do indeed rely on the crude/broken ArrayAccess behavior. It will be released as 5.0.0.

A question - do we need ArrayAccess at all, isn't Traversable enough?

We can add boolean flag to control behaviour and keep this feature in v4.0. What do you think?

ArrayAccess is indeed needed, as mapArray uses it to set values. Traversable is just a flag to differ collections from array-like objects.

P.S. Just noticed we may apply same treatment for nested arrays/collections (if I got this code right): https://github.com/cweiske/jsonmapper/blob/132c75c7dd83e45353ebb9c6c9f591952995bbf0/src/JsonMapper.php#L476

@cweiske Any progress on this?

@daniser I rebased against current master and fixed all the conflicts that appeared because #232 was merged before. It is merged now, with a lengthy explanation about the reasoning.

Thanks for the patch.

Released with v5.0.0.