Skip to content
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

[WIP] Allow deserializing null #1005

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

goetas
Copy link
Collaborator

@goetas goetas commented Oct 31, 2018

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 goetas force-pushed the kunicmarko20-deserialize_null branch from 22b1d4f to 3dbeb21 Compare October 31, 2018 12:04
@goetas
Copy link
Collaborator Author

goetas commented Oct 31, 2018

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

@@ -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);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was just a typo in the previous test

@@ -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));
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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'));
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how is this even happening if deserialize null is false

@goetas goetas changed the title Allow deserializing null [WIP] Allow deserializing null Oct 31, 2018
@@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@goetas goetas force-pushed the kunicmarko20-deserialize_null branch from 3dbeb21 to 110bebf Compare October 31, 2018 13:00
@@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change looks like it isn't needed

@kunicmarko20
Copy link
Contributor

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
Copy link
Collaborator Author

goetas commented May 2, 2019

@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.

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

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

@goetas
Copy link
Collaborator Author

goetas commented May 3, 2019

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

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

@goetas
Copy link
Collaborator Author

goetas commented May 3, 2019

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

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

@goetas
Copy link
Collaborator Author

goetas commented May 3, 2019

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

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

@goetas
Copy link
Collaborator Author

goetas commented May 3, 2019

Also the target doesn't have to be master anymore

What do you mean?

@kunicmarko20
Copy link
Contributor

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

@goetas
Copy link
Collaborator Author

goetas commented May 3, 2019

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

kunicmarko20 commented May 3, 2019

My bad, then we just wait. 😄

@goetas goetas force-pushed the kunicmarko20-deserialize_null branch from 01a91f3 to 20eb38d Compare May 3, 2019 22:05
@goetas goetas force-pushed the kunicmarko20-deserialize_null branch from 20eb38d to b5f5ba2 Compare May 3, 2019 22:15
@pscheit-lillydoo
Copy link

I would guess some time is passed now :)

@scaytrase
Copy link
Contributor

Any plans to release this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants