Skip to content

[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

Open
wants to merge 2 commits into
base: 7.3
Choose a base branch
from

Conversation

alexandre-daubois
Copy link
Member

@alexandre-daubois alexandre-daubois commented Jul 16, 2025

Q A
Branch? 7.3
Bug fix? yes
New feature? no
Deprecations? no
Issues -
License MIT

This PR makes the JsonPath component RFC compliant by removing all skipped tests of the compliance test suite.

*/
private function isJsonArrayStructure(mixed $value): bool
{
return \is_array($value) && array_is_list($value) && (1 !== \count($value) || !isset($value[0]));
Copy link
Member

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 ?

Copy link
Member Author

@alexandre-daubois alexandre-daubois Jul 17, 2025

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?

Copy link
Member

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.

Copy link
Member Author

@alexandre-daubois alexandre-daubois Jul 17, 2025

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?

Copy link
Member

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.

Copy link
Member Author

@alexandre-daubois alexandre-daubois Jul 17, 2025

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.

@alexandre-daubois alexandre-daubois force-pushed the jp-compliance branch 2 times, most recently from d1364da to 99ff096 Compare July 17, 2025 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants