Skip to content

[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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 23, 2021

Conversation

lyrixx
Copy link
Member

@lyrixx lyrixx commented Aug 18, 2021

Q A
Branch? 5.4
Bug fix? no
New feature? no
Deprecations? yes
Tickets
License MIT
Doc PR

@nicolas-grekas
Copy link
Member

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.php
index 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.php
index 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
Copy link
Member Author

lyrixx commented Aug 18, 2021

Could we find a better deprecation message ?

Returning empty object of class "%s" from "%s()" is deprecated unless it extends "ArrayObject".

With the tests suite fixtures it gives

Since symfony/serializer 5.4: Returning empty object of class "Symfony\Component\Serializer\Tests\DummyList" from "Symfony\Component\Serializer\Serializer::normalize()" is deprecated unless it extends "ArrayObject".

I'm not sure people understand what it means

@lyrixx
Copy link
Member Author

lyrixx commented Aug 18, 2021

And BTW, there is something strange here. It's legit to have class that implements \Countable, \IteratorAggregate. And it may not be possible to extends ArrayObject. So we are forcing people to do deep change, for something that will be fixed in 6.0.

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
Copy link
Member

[...] is deprecated. This class should extend "ArrayObject".?

@lyrixx lyrixx force-pushed the serializer-deprec-nested-object branch from 54a3436 to 806bb8f Compare August 23, 2021 08:47
@lyrixx
Copy link
Member Author

lyrixx commented Aug 23, 2021

@nicolas-grekas Should be OK 👍🏼

@fabpot
Copy link
Member

fabpot commented Aug 23, 2021

Thank you @lyrixx.

@fabpot fabpot merged commit 6d70316 into symfony:5.4 Aug 23, 2021
@lyrixx lyrixx deleted the serializer-deprec-nested-object branch August 23, 2021 10:06
fabpot added a commit that referenced this pull request Aug 25, 2021
…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
This was referenced Nov 5, 2021
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