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] Deprecate support for returning empty, iterable, countable, raw object when normalizing#42619
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
lyrixx commentedAug 18, 2021
| Q | A |
|---|---|
| Branch? | 5.4 |
| Bug fix? | no |
| New feature? | no |
| Deprecations? | yes |
| Tickets | |
| License | MIT |
| Doc PR |
5ddc752 to54a3436CompareUh oh!
There was an error while loading.Please reload this page.
nicolas-grekas commentedAug 18, 2021
I propose to apply the patch below on top of this PR. Then we might need a new test case for the legacy code path. diff --git a/src/Symfony/Component/Serializer/Serializer.php b/src/Symfony/Component/Serializer/Serializer.phpindex 1e4fb793e8..08f8e960ff 100644--- a/src/Symfony/Component/Serializer/Serializer.php+++ b/src/Symfony/Component/Serializer/Serializer.php@@ -169,7 +169,7 @@ class Serializer implements SerializerInterface, ContextAwareNormalizerInterface switch (true) { case ($context[AbstractObjectNormalizer::PRESERVE_EMPTY_OBJECTS] ?? false) && \is_object($data): if (!$data instanceof \ArrayObject) {- trigger_deprecation('symfony/serializer', '5.4', 'The method "%s()" will return an instance of "%s" as of Symfony 6.0 when the object is iteratable, countable and empty.', __METHOD__, \ArrayObject::class);+ trigger_deprecation('symfony/serializer', '5.4', 'Returning empty object of class "%s" from "%s()" is deprecated unless it extends "ArrayObject".', get_debug_type($data), __METHOD__); } return $data;diff --git a/src/Symfony/Component/Serializer/Tests/SerializerTest.php b/src/Symfony/Component/Serializer/Tests/SerializerTest.phpindex 2afbd51c22..ab74b06ab0 100644--- a/src/Symfony/Component/Serializer/Tests/SerializerTest.php+++ b/src/Symfony/Component/Serializer/Tests/SerializerTest.php@@ -578,10 +578,7 @@ class SerializerTest extends TestCase $this->assertSame($expected, $serializer->serialize($data, 'json')); }- /**- * @dataProvider provideObjectOrCollectionTests- * @group legacy- */+ /** @dataProvider provideObjectOrCollectionTests */ public function testNormalizePreserveEmptyArrayObject(Serializer $serializer, array $data) { $expected = '{"a1":{},"a2":{"k":"v"},"b1":[],"b2":{"k":"v"},"c1":{"nested":{}},"c2":{"nested":{"k":"v"}},"d1":{"nested":[]},"d2":{"nested":{"k":"v"}},"e1":{"map":[]},"e2":{"map":{"k":"v"}},"f1":{"map":{}},"f2":{"map":{"k":"v"}},"g1":{"list":{"list":[]},"settings":[]},"g2":{"list":["greg"],"settings":[]}}';@@ -599,10 +596,7 @@ class SerializerTest extends TestCase ])); }- /**- * @dataProvider provideObjectOrCollectionTests- * @group legacy- */+ /** @dataProvider provideObjectOrCollectionTests */ public function testNormalizeEmptyArrayAsObjectAndPreserveEmptyArrayObject(Serializer $serializer, array $data) { $expected = '{"a1":{},"a2":{"k":"v"},"b1":{},"b2":{"k":"v"},"c1":{"nested":{}},"c2":{"nested":{"k":"v"}},"d1":{"nested":{}},"d2":{"nested":{"k":"v"}},"e1":{"map":{}},"e2":{"map":{"k":"v"}},"f1":{"map":{}},"f2":{"map":{"k":"v"}},"g1":{"list":{"list":[]},"settings":{}},"g2":{"list":["greg"],"settings":{}}}';@@ -797,13 +791,15 @@ class Baz } }-class DummyList implements \Countable, \IteratorAggregate+class DummyList extends \ArrayObject { public $list; public function __construct(array $list) { $this->list = $list;++ $this->setFlags(\ArrayObject::STD_PROP_LIST); } public function count(): int |
lyrixx commentedAug 18, 2021
Could we find a better deprecation message ?
With the tests suite fixtures it gives
I'm not sure people understand what it means |
lyrixx commentedAug 18, 2021
And BTW, there is something strange here. It's legit to have class that Finally, I do think this is a very edge case, since the code is ATM broken. So no one could rely on it. (So... I could update my PR with you patch, since I think nobody will see the message :p) |
nicolas-grekas commentedAug 18, 2021
|
…le, raw object when normalizing
54a3436 to806bb8fComparelyrixx commentedAug 23, 2021
@nicolas-grekas Should be OK 👍🏼 |
fabpot commentedAug 23, 2021
Thank you@lyrixx. |
…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