[WIP] Allow deserializing null #1005

goetas · 2018-10-31T11:59:25Z

Q	A
Bug fix?	no
New feature?	yes
Doc updated	no
BC breaks?	yes
Deprecations?	no
Tests pass?	yes
Fixed tickets	-
License	MIT

Alternative implementation of #1004

goetas · 2018-10-31T12:07:04Z

The main trick here are the interaction between the changes in the graph navigator and the visitor

goetas · 2018-10-31T12:07:26Z

tests/Serializer/GraphNavigatorTest.php

@@ -151,7 +156,7 @@ protected function setUp()
        $this->serializationNavigator->initialize($this->serializationVisitor, $this->context);

        $this->deserializationNavigator = new DeserializationGraphNavigator($this->metadataFactory, $this->handlerRegistry, $this->objectConstructor, $this->accessor, $this->dispatcher);
-        $this->deserializationNavigator->initialize($this->deserializationVisitor, $this->context);
+        $this->deserializationNavigator->initialize($this->deserializationVisitor, $this->deserializeContext);


this was just a typo in the previous test

goetas · 2018-10-31T12:07:59Z

tests/Serializer/BaseSerializationTest.php

@@ -726,14 +746,14 @@ public function testDeserializingNull()
        self::assertEquals($this->getContent('blog_post_unauthored'), $this->serialize($post, SerializationContext::create()->setSerializeNull(true)));

        if ($this->hasDeserializer()) {
-            $deserialized = $this->deserialize($this->getContent('blog_post_unauthored'), get_class($post), DeserializationContext::create()->setSerializeNull(true));
+            $deserialized = $this->deserialize($this->getContent('blog_post_unauthored'), get_class($post));


typo. DeserializationContext::setSerializeNull has no effect when doing deserialization

goetas · 2018-10-31T12:08:42Z

tests/Serializer/BaseSerializationTest.php


            self::assertEquals('2011-07-30T00:00:00+00:00', $this->getField($deserialized, 'createdAt')->format(\DateTime::ATOM));
            self::assertAttributeEquals('This is a nice title.', 'title', $deserialized);
            self::assertAttributeSame(false, 'published', $deserialized);
            self::assertAttributeSame(false, 'reviewed', $deserialized);
            self::assertAttributeEquals(new ArrayCollection(), 'comments', $deserialized);
-            self::assertEquals(null, $this->getField($deserialized, 'author'));
+            self::assertEquals($author, $this->getField($deserialized, 'author'));


This is the BC break. Not sure even if the current behavior makes sense

how is this even happening if deserialize null is false

kunicmarko20 · 2018-10-31T12:34:51Z

src/GraphNavigator/DeserializationGraphNavigator.php

@@ -197,7 +214,10 @@ public function accept($data, ?array $type = null)
                    $this->context->pushPropertyMetadata($propertyMetadata);
                    try {
                        $v = $this->visitor->visitProperty($propertyMetadata, $data);
-                        $this->accessor->setValue($object, $v, $propertyMetadata, $this->context);
+
+                        if (null !== $v || true === $this->shouldDeserializeNull) {


so here is where you prevent it instead of doing in the visitor

This is what is breaking the tests and now that I think about it, it is not needed at all. Without it, tests pass.

src/GraphNavigator/DeserializationGraphNavigator.php

kunicmarko20 · 2018-10-31T23:05:41Z

tests/Serializer/BaseSerializationTest.php

@@ -726,14 +746,15 @@ public function testDeserializingNull()
        self::assertEquals($this->getContent('blog_post_unauthored'), $this->serialize($post, SerializationContext::create()->setSerializeNull(true)));

        if ($this->hasDeserializer()) {
-            $deserialized = $this->deserialize($this->getContent('blog_post_unauthored'), get_class($post), DeserializationContext::create());
+            $ctx =  DeserializationContext::create()
+            $deserialized = $this->deserialize($this->getContent('blog_post_unauthored'), get_class($post), $ctx);


this change looks like it isn't needed

kunicmarko20 · 2018-11-01T00:03:28Z

The only problem I see with this that it will make all fields in the request deserialize null, not sure if that is a good thing.

goetas · 2019-05-02T21:51:31Z

@kunicmarko20 I've found some time to work on this.
Te current approach allows to set the property type as nullable (like php 7.4 typed props).

Something as:

class BlogPost
{
    /**
     * @Type("?string")
     */
    private $id = 'what_a_nice_id';

    /**
     * @Type("?DateTime")
     */
    private $createdAt;

    /**
     * @Type("?array<JMS\Serializer\Tests\Fixtures\Comment>")
     */
    private $comments2;

    /**
     * @Type("array<string,?string>")
     */
    private $metadata;
}

When ? is used and setDeserializeNull is specified, then null properties are deserialized as null instead of having the default object value.

kunicmarko20 · 2019-05-03T08:32:36Z

Well, this looks amazing. Now you can choose to deserialize null for everything or per prop?

goetas · 2019-05-03T08:58:18Z

If $context->shouldDeserializeNull() is not called, ? is just ignored.
When $context->shouldDeserializeNull() is called then the props having ? in the type declaration will be null if the JSON contains null.

kunicmarko20 · 2019-05-03T09:10:54Z

ah, so you need to enable it and then select which properties this works on?

goetas · 2019-05-03T09:20:19Z

ah, so you need to enable it and then select which properties this works on?

yes. main reason backward compatibility.

Thinking it twice, having ? could be enough, so shouldDeserializeNull() could be not needed....

just trying to understand if there might have other implications...

kunicmarko20 · 2019-05-03T09:25:31Z

Ye, it looks like shouldDeserializeNull is not needed anymore since you need to add ? to your configuration and that should be BC?

goetas · 2019-05-03T09:27:22Z

Ye, it looks like shouldDeserializeNull is not needed anymore since you need to add ? to your configuration and that should be BC?

No, the ? is not a BC break. This solution is good enough IMO.
I still need to "let some time pass" to see if there is something I'might have missed, since this was the result of some "late night" coding 😄

kunicmarko20 · 2019-05-03T09:33:39Z

What could be an issue or a problem with merging this? Also the target doesn't have to be master anymore

goetas · 2019-05-03T09:35:32Z

Also the target doesn't have to be master anymore

What do you mean?

kunicmarko20 · 2019-05-03T10:01:33Z

Well, it is not a BC break, so no need to merge it on master? (assuming master is next major and not minor?)

goetas · 2019-05-03T10:41:14Z

The master branch is targeting 3.1 and not 4.x.

There are not yet any plans for a new major, so I'm keeping one branch less to manage

kunicmarko20 · 2019-05-03T10:42:08Z

My bad, then we just wait. 😄

pscheit-lillydoo · 2021-03-02T18:28:15Z

I would guess some time is passed now :)

scaytrase · 2021-09-10T13:41:44Z

Any plans to release this?

goetas force-pushed the kunicmarko20-deserialize_null branch from 22b1d4f to 3dbeb21 Compare October 31, 2018 12:04

goetas commented Oct 31, 2018

View reviewed changes

goetas changed the title ~~Allow deserializing null~~ [WIP] Allow deserializing null Oct 31, 2018

kunicmarko20 reviewed Oct 31, 2018

View reviewed changes

goetas force-pushed the kunicmarko20-deserialize_null branch from 3dbeb21 to 110bebf Compare October 31, 2018 13:00

kunicmarko20 reviewed Oct 31, 2018

View reviewed changes

src/GraphNavigator/DeserializationGraphNavigator.php Show resolved Hide resolved

kunicmarko20 reviewed Oct 31, 2018

View reviewed changes

goetas added the RFC label Dec 9, 2018

goetas mentioned this pull request Apr 18, 2019

Add control to deserialization of null values #821

Open

goetas force-pushed the kunicmarko20-deserialize_null branch from 4008e9f to 9efa114 Compare May 2, 2019 21:47

goetas force-pushed the kunicmarko20-deserialize_null branch from 9efa114 to 01a91f3 Compare May 2, 2019 21:52

goetas force-pushed the kunicmarko20-deserialize_null branch from 01a91f3 to 20eb38d Compare May 3, 2019 22:05

kunicmarko20 and others added 2 commits May 4, 2019 00:15

Allow deserializing of null

8151889

handle null

b5f5ba2

goetas force-pushed the kunicmarko20-deserialize_null branch from 20eb38d to b5f5ba2 Compare May 3, 2019 22:15

goetas mentioned this pull request Oct 14, 2024

introduce requireAllRequiredProperties context #1554

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Allow deserializing null #1005

[WIP] Allow deserializing null #1005

goetas commented Oct 31, 2018

goetas commented Oct 31, 2018

goetas Oct 31, 2018

goetas Oct 31, 2018

goetas Oct 31, 2018

kunicmarko20 Oct 31, 2018

kunicmarko20 Oct 31, 2018

kunicmarko20 Oct 31, 2018

kunicmarko20 Oct 31, 2018

kunicmarko20 commented Nov 1, 2018

goetas commented May 2, 2019

kunicmarko20 commented May 3, 2019

goetas commented May 3, 2019

kunicmarko20 commented May 3, 2019

goetas commented May 3, 2019

kunicmarko20 commented May 3, 2019

goetas commented May 3, 2019

kunicmarko20 commented May 3, 2019

goetas commented May 3, 2019

kunicmarko20 commented May 3, 2019

goetas commented May 3, 2019

kunicmarko20 commented May 3, 2019 •

edited

Loading

pscheit-lillydoo commented Mar 2, 2021

scaytrase commented Sep 10, 2021

[WIP] Allow deserializing null #1005

Are you sure you want to change the base?

[WIP] Allow deserializing null #1005

Conversation

goetas commented Oct 31, 2018

goetas commented Oct 31, 2018

goetas Oct 31, 2018

Choose a reason for hiding this comment

goetas Oct 31, 2018

Choose a reason for hiding this comment

goetas Oct 31, 2018

Choose a reason for hiding this comment

kunicmarko20 Oct 31, 2018

Choose a reason for hiding this comment

kunicmarko20 Oct 31, 2018

Choose a reason for hiding this comment

kunicmarko20 Oct 31, 2018

Choose a reason for hiding this comment

kunicmarko20 Oct 31, 2018

Choose a reason for hiding this comment

kunicmarko20 commented Nov 1, 2018

goetas commented May 2, 2019

kunicmarko20 commented May 3, 2019

goetas commented May 3, 2019

kunicmarko20 commented May 3, 2019

goetas commented May 3, 2019

kunicmarko20 commented May 3, 2019

goetas commented May 3, 2019

kunicmarko20 commented May 3, 2019

goetas commented May 3, 2019

kunicmarko20 commented May 3, 2019

goetas commented May 3, 2019

kunicmarko20 commented May 3, 2019 • edited Loading

pscheit-lillydoo commented Mar 2, 2021

scaytrase commented Sep 10, 2021

kunicmarko20 commented May 3, 2019 •

edited

Loading