[Serializer] DateTimeNormalizer handling of null and empty values (returning it instead of new object) #25287

Simperfit · 2017-12-03T14:10:49Z

Q	A
Branch?	3.3
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	no
Fixed tickets	#23964
License	MIT
Doc PR

I'm openning the disucussion on this as I think that should be returning null and not a new object.

WDYT ?

Working at home ;)

Simperfit · 2017-12-03T14:32:16Z

Travis failure is unrelated

nicolas-grekas · 2017-12-04T10:47:46Z

src/Symfony/Component/Serializer/Tests/Normalizer/DateTimeNormalizerTest.php

+    public function testDenormalizeWithNullAndEmptyString()
+    {
+        $this->assertNull($this->normalizer->denormalize(null, \DateTimeInterface::class));
+        $this->assertEmpty($this->normalizer->denormalize('', \DateTimeInterface::class));


assertSame(''

nicolas-grekas · 2017-12-04T10:50:10Z

src/Symfony/Component/Serializer/Normalizer/DateTimeNormalizer.php

@@ -67,6 +67,10 @@ public function denormalize($data, $class, $format = null, array $context = arra
    {
        $dateTimeFormat = isset($context[self::FORMAT_KEY]) ? $context[self::FORMAT_KEY] : null;

+        if (empty($data)) {


Could be too broad? Should should only null? Should we also really check the empty string?
@dunglas any hint?

I agree. Maybe should we just tweak the supportDenormalization function to skip this normalizer is the value isn't a string.

And we keep the test on empty strings or do we add it in the supportDenormalization ?

isn't that a BC ?

done, added to supportDenorm and modifed the denormalize

sroze · 2017-12-05T10:06:44Z

src/Symfony/Component/Serializer/Tests/Normalizer/DateTimeNormalizerTest.php

@@ -67,6 +67,7 @@ public function testSupportsDenormalization()
        $this->assertTrue($this->normalizer->supportsDenormalization('2016-01-01T00:00:00+00:00', \DateTime::class));
        $this->assertTrue($this->normalizer->supportsDenormalization('2016-01-01T00:00:00+00:00', \DateTimeImmutable::class));
        $this->assertFalse($this->normalizer->supportsDenormalization('foo', 'Bar'));
+        $this->assertFalse($this->normalizer->supportsDenormalization(null, \DateTimeImmutable::class));


What does this mean? If not denormalizer supports the "thing", what happens? 🤔

Simperfit · 2017-12-05T10:21:53Z

@sroze AFAIK it will return null if no normalizer supports it.

ogizanagi · 2017-12-05T10:50:50Z

@Simperfit , @sroze : Actually you'll get a Could not denormalize object of type \DateTime, no supporting normalizer found exception.

Simperfit · 2017-12-05T11:38:31Z

even if it’s null ? Le mar. 5 déc. 2017 à 11:51, Maxime Steinhausser <notifications@github.com> a écrit :

…

@Simperfit <https://github.com/simperfit> , @sroze <https://github.com/sroze> : Actually you'll get a Could not denormalize object of type \DateTime, no supporting normalizer found exception. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#25287 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADSq8hmKipIiAjyAs1w9u9spR-fo2I07ks5s9SAcgaJpZM4Qzu3l> .

ogizanagi · 2017-12-05T16:13:29Z

IIRC: as a nullable nested attribute on an object when using the ObjectNormalizer, when encountering null it should return null.
But in case you directly call $this->serializer->denormalize(null, '\DateTime', $format, $context) (in your own serializer/denormalizer aware denormalizer for instance), you'll get this exception with your changes.
That's probably fine, as we never account for null as $data in any denormalizer AFAIK, but it's kind of disturbing having an exception about no denormalizer found for \DateTime while there is one, but just won't support null.
Shouldn't we throw an UnexpectedValueException from denormalize on null or empty string instead?

Simperfit · 2017-12-05T18:11:59Z

@ogizanagi I kind of agree with you, @dunglas WDYT ?

Simperfit · 2017-12-09T08:30:17Z

PR Modified to now throw on empty strings and null values.

ostrolucky · 2017-12-10T15:00:59Z

Why why does this normalizer lie in supportsDenormalization if it doesn't support null/empty string?

Simperfit · 2017-12-10T15:03:47Z

Because if we add a test it will throw an invalid message. Le dim. 10 déc. 2017 à 16:01, Gabriel Ostrolucký <notifications@github.com> a écrit :

…

Why why does this normalizer lie in supportsDenormalization if it doesn't support null/empty string? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#25287 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADSq8vCdjpPvAIwTUCFsyWSWTFq9J1XFks5s-_I8gaJpZM4Qzu3l> .

ostrolucky · 2017-12-10T15:24:15Z

It's kind of right, isn't it? It says no supporting denormalizer has been found, which is true. This denormalizer does not support null.

That message could be improved, but that's different problem.

ogizanagi · 2017-12-10T15:29:36Z

None of the denormalizers supports null actually AFAIK. IMHO, it's confusing getting a "No denormalizer found" exception when explicitly asking for \DateTime and knowing I have a DateTimeNormalizer registered. Here it's calling denormalize on null which is wrong, hence the UnexpectedValueException makes more sense to me.

ostrolucky · 2017-12-10T15:33:33Z

it's confusing getting a "No denormalizer found"

It is, but that's not a problem of this normalizer, that's a thing to be fixed in Serializer.

If denormalizers are not allowed to decide if they support denormalization based on a value passed, that's serious flaw in DenormalizerInterface

ostrolucky · 2017-12-13T09:10:41Z

I suggest we return null for null input. It would be consistent with JMS serializer.

Simperfit · 2017-12-16T04:51:15Z

@ostrolucky I don't know if we have to be consistent with JMS. I think returning an error is better for DX because you may have a bug at some point and passing null instead of the right value, this shows you that something is wrong.

If you are returning null it could lead to unwanted behaviour since this should not be able to return null.

I agree that something has to be done in the denormalizer (supports method) but this is IMO the best way to deal with this.

ostrolucky · 2017-12-16T10:58:50Z

Forcing developer to implement his own denormalizer just to return null for null input and messing with order of denormalizers in serializer is bad DX. It's also bad DX to work around the root problem (not fixing message in Serializer), so when developer implements supportsDenormalization with check based on value, he will get weird message again.

How about this then: Allow to specify target of denormalization to be ?DateTime::class alongside DateTime::class. Now it's clear output of denormalization is allowed to be null.

don't know if we have to be consistent with JMS

We don't have to do it same way, but we should take more inspiration from it. JMS is much more mature PHP serializer. We don't have to throw exception and hope for the best that developers won't mind it too much. We can learn from history of others. Right now we are dealing with problems which were already solved long ago. We shouldn't have to do that. JMS datetime denormalizer returns null for null input since 2012. It also allows to specify what to do with nulls in scope of whole Serialization context, via Context setting.

But that's outside the scope of this PR. For now my request is to at least give developer option to not crash denormalization if input in this denormalizer is null. Seems returning null is expected behaviour from user who posted linked issue too, after all.

Simperfit · 2017-12-21T16:17:25Z

So @ogizanagi what do you think ?

ogizanagi · 2017-12-21T17:55:27Z

Forcing developer to implement his own denormalizer just to return null for null input and messing with order of denormalizers in serializer is bad DX.

There might be a little misunderstanding here: no need for the developper to implement his own denormalizers just for handling null properties. That's calling denormalize on null which is wrong in the current state of the serializer and core denormalizers implementations.
And it's probably motivated by a simple reason: it'd be a noop (null stays null, no need to denormalize anything).

High-level denormalizers like the ObjectNormalizer are clever enough to not call denormalize on null. And if you write your own high-level denormalizer-aware denormalizer for your objects with nested properties to denormalize, you'll save a useless call to $this->denormalizer->denormalize for a given field if the data you get for it is null.

Low-level denormalizers in core like the DateTime ones never accounted for null as $data.

So to sum-up: I think this patch is fine regarding the current state of the serializer and core denormalizers. Handling null to return null inside denormalize would be a paradigm-shift so it'll need its own issue & discussion and each core denormalizers would need to be updated.
Or it'll might be fine to just update Serializer::denormalize() to return null on $data being null.

ogizanagi · 2017-12-21T18:00:43Z

src/Symfony/Component/Serializer/Normalizer/DateTimeNormalizer.php

@@ -67,6 +67,10 @@ public function denormalize($data, $class, $format = null, array $context = arra
    {
        $dateTimeFormat = isset($context[self::FORMAT_KEY]) ? $context[self::FORMAT_KEY] : null;

+        if ('' === $data || null === $data) {
+            throw new UnexpectedValueException('The data is either an empty string or null, you should pass an object implementing \DateTimeInterface.');


This message is wrong: we don't expect an object implementing \DateTimeInterface. We expect a string that can be parsed according to the configured format or the \DateTime::__construct() if not configured.

…turning null or empty instead of new object)

fabpot · 2018-01-17T08:02:41Z

Thank you @Simperfit.

… values (returning it instead of new object) (Simperfit) This PR was merged into the 3.3 branch. Discussion ---------- [Serializer] DateTimeNormalizer handling of null and empty values (returning it instead of new object) | Q | A | ------------- | --- | Branch? | 3.3 | Bug fix? | yes | New feature? | no  | BC breaks? | no | Deprecations? | no  | Tests pass? | no | Fixed tickets | #23964 | License | MIT | Doc PR | I'm openning the disucussion on this as I think that should be returning null and not a new object. WDYT ? Working at home ;) ![img_2914](https://user-images.githubusercontent.com/3451634/33526107-ec2a6ce8-d83b-11e7-8949-f8d360ebb4b9.JPG) Commits ------- 74726f3 [Serializer] DateTimeNormalizer handling of null and empty values (returning null or empty instead of new object)

florianajir · 2018-04-06T10:27:55Z

With this feature we can't anymore denormalize object with null datetime attributes ?

On this call:

/** @var Symfony\Component\Serializer\Normalizer\DenormalizerInterface $denormalizer */
$denormalizer->denormalize($response['_source'], City::class);

I get this error :
The data is either an empty string or null, you should pass a string that can be parsed with the passed format or a valid DateTime string

The null attribute is City::updatedAt of DateTime type

There isn't an option to skip null values ?

florianajir · 2018-04-06T13:08:19Z

I found a solution : php doc type hinting null too:

/**
 * @var DateTime|null
 * @Assert\DateTime()
 */
private $updatedAt;

ogizanagi · 2018-04-06T13:10:23Z

@florianajir : ~~Is your City::updatedAt properly identified as nullable? Check if the docblock has proper @var \DateTime|null so the PropertyInfo can extract this info.~~
Indeed, you have to properly typehint/document the nullable properties for the ObjectNormalizer to work as expected. Otherwise the PropertyInfo component cannot guess the field is nullable.

As I said in a previous comment, the ObjectNormalizer is clever enough to not call denormalize on null. Of course that's only true if it knows it's nullable.
Without this info, it'll try to call the DateTime denormalizer, which would result in the bug mentioned in original issue, i.e a new DateTime object set to the current date and time, which was an unexpected behavior.

carsonbot added Status: Needs Review Serializer Bug labels Dec 3, 2017

Simperfit force-pushed the bugfix/return-null-when-passing-null-to-date-time-normalizer branch from c95353a to 6292a6b Compare December 3, 2017 14:11

nicolas-grekas added this to the 3.3 milestone Dec 3, 2017

nicolas-grekas reviewed Dec 4, 2017

View reviewed changes

Simperfit force-pushed the bugfix/return-null-when-passing-null-to-date-time-normalizer branch 4 times, most recently from 94f5658 to 233ad77 Compare December 4, 2017 17:07

sroze reviewed Dec 5, 2017

View reviewed changes

Simperfit force-pushed the bugfix/return-null-when-passing-null-to-date-time-normalizer branch from 233ad77 to 6bfb5a2 Compare December 9, 2017 08:29

ogizanagi reviewed Dec 21, 2017

View reviewed changes

Simperfit force-pushed the bugfix/return-null-when-passing-null-to-date-time-normalizer branch from 6bfb5a2 to 9bfbf3d Compare January 12, 2018 13:53

[Serializer] DateTimeNormalizer handling of null and empty values (re…

74726f3

…turning null or empty instead of new object)

Simperfit force-pushed the bugfix/return-null-when-passing-null-to-date-time-normalizer branch from 9bfbf3d to 74726f3 Compare January 12, 2018 13:55

carsonbot added Status: Reviewed and removed Status: Needs Review labels Jan 16, 2018

ogizanagi approved these changes Jan 16, 2018

View reviewed changes

fabpot approved these changes Jan 17, 2018

View reviewed changes

fabpot merged commit 74726f3 into symfony:3.3 Jan 17, 2018

This was referenced Jan 29, 2018

Release v3.3.16 #25955

Merged

Release v3.4.4 #25956

Merged

Release v4.0.4 #25957

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Serializer] DateTimeNormalizer handling of null and empty values (returning it instead of new object) #25287

[Serializer] DateTimeNormalizer handling of null and empty values (returning it instead of new object) #25287

Simperfit commented Dec 3, 2017

Simperfit commented Dec 3, 2017

nicolas-grekas Dec 4, 2017

nicolas-grekas Dec 4, 2017

dunglas Dec 4, 2017

Simperfit Dec 4, 2017

Simperfit Dec 4, 2017

Simperfit Dec 5, 2017

sroze Dec 5, 2017

Simperfit commented Dec 5, 2017 via email •

edited

Loading

ogizanagi commented Dec 5, 2017 •

edited

Loading

Simperfit commented Dec 5, 2017 via email

ogizanagi commented Dec 5, 2017 •

edited

Loading

Simperfit commented Dec 5, 2017

Simperfit commented Dec 9, 2017

ostrolucky commented Dec 10, 2017

Simperfit commented Dec 10, 2017 via email

ostrolucky commented Dec 10, 2017

ogizanagi commented Dec 10, 2017

ostrolucky commented Dec 10, 2017

ostrolucky commented Dec 13, 2017

Simperfit commented Dec 16, 2017

ostrolucky commented Dec 16, 2017 •

edited

Loading

Simperfit commented Dec 21, 2017

ogizanagi commented Dec 21, 2017 •

edited

Loading

ogizanagi Dec 21, 2017

fabpot commented Jan 17, 2018

florianajir commented Apr 6, 2018 •

edited

Loading

florianajir commented Apr 6, 2018 •

edited

Loading

ogizanagi commented Apr 6, 2018 •

edited

Loading

[Serializer] DateTimeNormalizer handling of null and empty values (returning it instead of new object) #25287

[Serializer] DateTimeNormalizer handling of null and empty values (returning it instead of new object) #25287

Conversation

Simperfit commented Dec 3, 2017

Simperfit commented Dec 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Simperfit commented Dec 5, 2017 via email • edited Loading

ogizanagi commented Dec 5, 2017 • edited Loading

Simperfit commented Dec 5, 2017 via email

ogizanagi commented Dec 5, 2017 • edited Loading

Simperfit commented Dec 5, 2017

Simperfit commented Dec 9, 2017

ostrolucky commented Dec 10, 2017

Simperfit commented Dec 10, 2017 via email

ostrolucky commented Dec 10, 2017

ogizanagi commented Dec 10, 2017

ostrolucky commented Dec 10, 2017

ostrolucky commented Dec 13, 2017

Simperfit commented Dec 16, 2017

ostrolucky commented Dec 16, 2017 • edited Loading

Simperfit commented Dec 21, 2017

ogizanagi commented Dec 21, 2017 • edited Loading

Choose a reason for hiding this comment

fabpot commented Jan 17, 2018

florianajir commented Apr 6, 2018 • edited Loading

florianajir commented Apr 6, 2018 • edited Loading

ogizanagi commented Apr 6, 2018 • edited Loading

Simperfit commented Dec 5, 2017 via email •

edited

Loading

ogizanagi commented Dec 5, 2017 •

edited

Loading

ogizanagi commented Dec 5, 2017 •

edited

Loading

ostrolucky commented Dec 16, 2017 •

edited

Loading

ogizanagi commented Dec 21, 2017 •

edited

Loading

florianajir commented Apr 6, 2018 •

edited

Loading

florianajir commented Apr 6, 2018 •

edited

Loading

ogizanagi commented Apr 6, 2018 •

edited

Loading