-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[JsonPath] Make the component RFC compliant #61132
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
base: 7.3
Are you sure you want to change the base?
Conversation
*/ | ||
private function isJsonArrayStructure(mixed $value): bool | ||
{ | ||
return \is_array($value) && array_is_list($value) && (1 !== \count($value) || !isset($value[0])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why wouldn't ['foo']
be a JSON array structure ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method handles an edge case: when a json like [{"0": 5}]
is decoded to PHP, accessing @[0]
on the inner object should return nothing, because numeric index selectors only work on arrays, not objects per the RFC. So just calling array_is_list()
would incorrectly allow it. The ||
part of the condition specifically excludes single-element arrays with index 0 set to handle this distinction.
Does this answer your question?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but this ignores 2 cases:
- AFAICT, it breaks the case of accessing single-element arrays
- you could totally have JSON objects with multiple numeric keys (
{"0": 5, "1": 12}
), which would be considered array structure in your method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems pretty complex to fix... Just in case, @mtarld, is there something in JsonStreamer that could also help here? We would need to "easily" differentiate an array from an object with numeric indices. Maybe Read/Lexer.php
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the case of strings using json_decode
, we might decode objects into stdClass
to distinguish objects from arrays. But this suffers from other limitations as this won't support object keys that are not valid names of PHP properties.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried this approach in 54281cb and succeeded on making the CI green. It makes things a bit more complicated but we can easily differentiate objects and arrays now. Also updated the compliance test suite because the specific cases we were dealing with suffered from the same problem when being decoded from cts.json
.
this suffers from other limitations as this won't support object keys that are not valid names of PHP properties
Do you have an example? It seems to work pretty well, and as far as I know, stdClass are actually "allowed" to use wrong property names like https://3v4l.org/ede7M#vnull.
d1364da
to
99ff096
Compare
99ff096
to
9c7a899
Compare
This PR makes the JsonPath component RFC compliant by removing all skipped tests of the compliance test suite.