Fix mapping objects implementing ArrayAccess (not collections) #224
Conversation
|
Thank you for the patch. This is indeed a BC break that I did not think about. A question - do we need |
|
We can add boolean flag to control behaviour and keep this feature in v4.0. What do you think?
P.S. Just noticed we may apply same treatment for nested arrays/collections (if I got this code right): Line 476 in 132c75c |
# Conflicts: # tests/ArrayTest.php # tests/support/JsonMapperTest/Array.php # tests/support/JsonMapperTest/ArrayAccessCollection.php # tests/support/JsonMapperTest/ArrayAccessObject.php
|
@cweiske Any progress on this? |
Only handle objects as array when they implement both ArrayAccess and Traversable. BC break! ---- Originally, JsonMapper handled objects extending ArrayObject as arrays. Extending own collection classes from ArrayObject is not always feasible (issue #175, #175), so a way was sought to rely on interfaces only. Patch #197 (#197) changed the implementation to check for the ArrayAccess interface instead of ArrayObject. This unfortunately breaks objects-that-allow-array-access-but-are-not-traversable-arrays (issue #224, #224), for example when you allow array access to properties stored in some internal variable. The correct solution is to check that the object implements ArrayAcces *and* Traversable - then we can be sure the object is intended to be used with e.g. foreach(). Resolves: #224
Only handle objects as array when they implement both ArrayAccess and Traversable. BC break! ---- Originally, JsonMapper handled objects extending ArrayObject as arrays. Extending own collection classes from ArrayObject is not always feasible (issue #175, #175), so a way was sought to rely on interfaces only. Patch #197 (#197) changed the implementation to check for the ArrayAccess interface instead of ArrayObject. This unfortunately breaks objects-that-allow-array-access-but-are-not-traversable-arrays (issue #224, #224), for example when you allow array access to properties stored in some internal variable. The correct solution is to check that the object implements ArrayAcces *and* Traversable - then we can be sure the object is intended to be used with e.g. foreach(). Resolves: #224
|
Released with v5.0.0. |
This PR fixes issue introduced by PR #197.
PR #197 replaced
ArrayObjectcheck with more broadArrayAccess, thus breaking BC.This PR added additional check for
Traversableto make sure given item is a collection and not a simple array-like object.Every collection implementation I know implements either
IteratororIteratorAggregate(both areTraversabledescendants).Moreover, sample collection from original issue #175 implements
Iteratortoo, so I can state with great confidence that this PR won't introduce more BC breaks.