Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Serializer] Remove support for returning empty, iterable, countable, raw object when normalizing#42699
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
… raw object when normalizing
Uh oh!
There was an error while loading.Please reload this page.
| switch (true) { | ||
| case ($context[AbstractObjectNormalizer::PRESERVE_EMPTY_OBJECTS] ??false) &&\is_object($data): | ||
| if (!$datainstanceof \ArrayObject) { | ||
| trigger_deprecation('symfony/serializer','5.4','Returning empty object of class "%s" from "%s()" is deprecated. This class should extend "ArrayObject".',get_debug_type($data),__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.
Re-reading, I think we should return the original object when it extends ArrayObject
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.
I disagree. It produces really bad behavior that I explained in your PR.
See the current diff on the test suite to understand.
- "g1":{"list":{"list":[]},"settings":[]},"g2":{"list":["greg"],"settings":[]}}';+ "g1":{"list":{},"settings":[]},"g2":{"list":["greg"],"settings":[]}}';
before: There is an innerlist when the list is empty, but it does not exist when the list is not empty
after: the shape is always the same, whereas the list is empty or not
=> This is a bug fix
nicolas-grekasAug 24, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
That's a behavior change that we'd better not do imho. The type hint doesn't require this.
Or we'd need another deprecation on 5.4, but I don't see why we'd do this (especially since there is a test about this).
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.
I added this test BTW, to show how it behaves.
I thought it was clear in the previous conversation... 😬
I can do what you want, but I persist to say it's a bad idea, since the serializer is not doing what people expect!
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.
See#42730 as follow up of this discussion.
…jects when PRESERVE_EMPTY_OBJECTS is set (nicolas-grekas)This PR was merged into the 5.4 branch.Discussion----------[Serializer] Return an ArrayObject for empty collection objects when PRESERVE_EMPTY_OBJECTS is set| Q | A| ------------- | ---| Branch? | 5.4| Bug fix? | yes| New feature? | no| Deprecations? | no| Tickets | -| License | MIT| Doc PR | -In#42619, we tried adding a BC layer for this behavior change, but when reviewing#42699, I figured out there is no possible BC layer: userland implementations (eg an entity returning an empty doctrine collection) won't extend ArrayObject because we ask them to do so.I therefor propose to return an ArrayObject as of 5.4, relying on the intuition that this should not have much BC impact in practice./cc `@lyrixx`Commits-------7856fe7 [Serializer] Return an ArrayObject for empty collection objects when PRESERVE_EMPTY_OBJECTS is set
The test suite is now green 👍
ping@nicolas-grekas