-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[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
Conversation
lyrixx
commented
Aug 18, 2021
Q | A |
---|---|
Branch? | 5.4 |
Bug fix? | no |
New feature? | no |
Deprecations? | yes |
Tickets | |
License | MIT |
Doc PR |
5ddc752
to
54a3436
Compare
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 |
Could we find a better deprecation message ?
With the tests suite fixtures it gives
I'm not sure people understand what it means |
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) |
|
…le, raw object when normalizing
54a3436
to
806bb8f
Compare
@nicolas-grekas Should be OK 👍🏼 |
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