Skip to content

Conversation

mtarld
Copy link
Contributor

@mtarld mtarld commented Dec 27, 2024

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

Fix encoding of dictionary with integer keys.

Previously, [1 => true, 2 => false] was encoded to {"": true, "": false}, now it is encoded to {"1": true, "2": false}.

Also improve test coverage about list, dictionaries and iterable.

@fabpot
Copy link
Member

fabpot commented Jan 17, 2025

Thank you @mtarld.

@fabpot fabpot merged commit e1a443c into symfony:7.3 Jan 17, 2025
11 checks passed
@mtarld mtarld deleted the fix/list-dict branch January 17, 2025 07:16
nicolas-grekas added a commit that referenced this pull request Sep 1, 2025
This PR was merged into the 7.3 branch.

Discussion
----------

[JsonStreamer] Fix decoding iterable lists

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

Because a `list<T>` is a subtype of `array<T>`, an `iterable<T>` cannot be a list.

This means that, as said [here](#59308 (comment)):
> There is no way to be sure that keys of an iterable are successive (even if they are integers).
> And if keys are not successive, the resulting JSON structure is an object and not an array.
> According, to the [JSON specification](https://datatracker.ietf.org/doc/html/rfc8259#section-4), object' keys must be strings, and not integers.

This imply that it is right now impossible to "yield" items from `[{"itemId": 1, "modificationDate": "2025-10-10"}]` - it can only be yielded from `{"0": {"itemId": 1, "modificationDate": "2025-10-10"}}`.

I think that in the real world, a lot of people want to be able to yield items from `[...]`, even though it does not stick with the JSON specification.

This PR is doing the trade-off of considering `iterable<int, Anything>` as an "iterable list" (like `list<Anything>` but not cast to array).

Commits
-------

2b9b801 [JsonStreamer] Fix decoding iterable lists
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.

5 participants