[Serializer] Respect default context in DateTimeNormalizer::denormalize #43329

hultberg · 2021-10-05T17:54:51Z

Q	A
Branch?	4.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Tickets	Fix #29030
License	MIT
Doc PR

Hey there. 👋

I'm bringing my first code patch to symfony today and it's for fixing a bug I encountered today while working with DateTimeNormalizer. In my project I have set a default date format in the service container and I noticed that the denormalized date did not follow my set date format (I enforce time but the serialized date did not contain the time stamp which should have failed when using DateTime::createFromDate). After some debugging I discovered the issue described in issue #29030

DateTimeNormalizer::normalize respects the date time format in context or default context while DateTimeNormalizer::denormalize only respects the former. This patch adds logic to denormalize too attempt create a DateTime object from the default context format (by default set to \DateTime::RFC3339) with createFromFormat and then if that fails continue on with the current behaviour. In my opinion this patch does not break the symfony BC promise as it simply adds another way to denormalize the date time.

One issue: There are no error handling when creating the date time from default context as the logic just continues on which might give the impression to the developer that the denormalization with the specific format worked correctly. I was not able to find a way to handle any error and keeping BC.
Future tough: At some point the new DateTime call in denormalize should be deprecated since creating a date time with createFromFormat is considered "safer" in my eyes as it's very explicit while the call to new DateTime is implicit about the date format and must perform parsing and guessing of the intended format (explained over at https://www.php.net/manual/en/datetime.formats.php). Another thing about new DateTime is the terriable error handling as you must catch a \Exception which means you catch "what ever" exception that can exist, even \ErrorException since \ErrorException extends \Exception.

Fixes #29030
Alternative PR (closed) #33813

fixes symfony#29030

hultberg · 2021-10-05T18:16:14Z

I targeted the wrong branch so the milestone is now wrong.

ro0NL · 2021-10-05T18:54:58Z

There are no error handling when creating the date time from default context as the logic just continues on which might give the impression to the developer that the denormalization with the specific format worked correctly. I was not able to find a way to handle any error and keeping BC.

We could preserve $defaultDateTimeErrors, and hint about it when the constructor fallback fails too.

At some point the new DateTime call in denormalize should be deprecated since creating a date time with createFromFormat is considered "safer" in my eyes as it's very explicit while the call to new DateTime is implicit about the date format

We could argue the formatted value in a raw payload is already validated, before being passed to a deserializer :)

That said, i think the datetime-constructor-as-fallback is a generally fine approach, for what is an implementation detail. If you get a DateTime, your value is denormalized.

edit: it also enables robustness-principle by default :)

hultberg · 2021-10-05T19:23:53Z

@ro0NL

We could argue the formatted value in a raw payload is already validated, before being passed to a deserializer :)

Can you elaborate on that the raw payload is already validated? Are there a "normal apporch" to use a validator with the raw payload? In my real world example I deserialize some JSON payload into a object then I validate it with symfony validator. I have had to override DateTimeNormalizer to catch any errors which are later cough by a custom date validator I made.

We could preserve $defaultDateTimeErrors, and hint about it when the constructor fallback fails too.

That seems like a good solution to at the least provide some hints about the default context format failing.

ro0NL · 2021-10-05T19:55:22Z

Are there a "normal apporch" to use a validator with the raw payload?

Im still on a mission to shift the paradigm :) #43047 (comment)

"validation before hydration" benefits object design IMHO.

nicolas-grekas · 2021-10-28T15:22:49Z

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

@@ -113,6 +113,16 @@ public function denormalize($data, $type, $format = null, array $context = [])
            throw new NotNormalizableValueException(sprintf('Parsing datetime string "%s" using format "%s" resulted in %d errors: ', $data, $dateTimeFormat, $dateTimeErrors['error_count'])."\n".implode("\n", $this->formatDateTimeErrors($dateTimeErrors['errors'])));
        }

+        $defaultDateTimeFormat = $this->defaultContext[self::FORMAT_KEY] ?? null;


why don't you patch L97 to this instead?
$dateTimeFormat = $context[self::FORMAT_KEY] ?? $this->defaultContext[self::FORMAT_KEY] ?? null;

we allow createFromFormat from defaultContext[FORMAT_KEY] to fail due BC, but generally we can't assume the default format fits all payloads

Should we trigger a deprecation in case the fallback path is triggered, so we can remove it in Symfony 7+ to enforce the defaultContext[FORMAT_KEY] is honored?

Symfony 7 is released. Is it planned to remove it?

Not yet as far as I can see. The first step to remove something in the next major release would be to deprecate it in a minor release before.

ogizanagi

With additional deprecation as mentioned in https://github.com/symfony/symfony/pull/43329/files#r812202044

fabpot · 2022-07-23T15:19:34Z

@hultberg Are you still interested in finishing this PR? It looks like we are almost there.

ogizanagi · 2022-07-28T08:39:34Z

Well, this targets 4.4 as a bug fix, so we cannot add the deprecation I suggested. So this should be ready to merge as is.

ogizanagi · 2022-07-28T10:04:39Z

I added the deprecation and opened the discussion in #47095

nicolas-grekas · 2022-07-28T12:55:25Z

Thank you @hultberg.

This comment has been minimized.

Sign in to view

hultberg marked this pull request as ready for review October 5, 2021 18:05

hultberg requested a review from dunglas as a code owner October 5, 2021 18:05

carsonbot added Status: Needs Review Bug Serializer labels Oct 5, 2021

carsonbot added this to the 5.4 milestone Oct 5, 2021

hultberg force-pushed the eh/ticket-29030 branch from 608f56f to 569bf07 Compare October 5, 2021 18:11

[Serializer] Respect default context in DateTimeNormalizer::denormalize

763ea05

fixes symfony#29030

hultberg force-pushed the eh/ticket-29030 branch from 674a7bd to 763ea05 Compare October 5, 2021 18:15

hultberg changed the base branch from 5.4 to 4.4 October 5, 2021 18:15

derrabus modified the milestones: 5.4, 4.4 Oct 5, 2021

nicolas-grekas reviewed Oct 28, 2021

View reviewed changes

ogizanagi approved these changes Jun 23, 2022

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Jun 23, 2022

ogizanagi mentioned this pull request Jul 28, 2022

[Serializer] Deprecate using datetime construct as fallback on default format mismatch #47095

Closed

chalasr approved these changes Jul 28, 2022

View reviewed changes

nicolas-grekas merged commit 57e5141 into symfony:4.4 Jul 28, 2022

This was referenced Jul 29, 2022

Release v4.4.44 #47114

Merged

Release v5.4.11 #47115

Merged

This was referenced Jul 29, 2022

Release v6.0.11 #47116

Merged

Release v6.1.3 #47117

Merged

hultberg deleted the eh/ticket-29030 branch January 8, 2024 07:30

alexndlm mentioned this pull request Nov 22, 2024

[Serializer] Trigger deprecation when could not parse date with default format #58966

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Serializer] Respect default context in DateTimeNormalizer::denormalize #43329

[Serializer] Respect default context in DateTimeNormalizer::denormalize #43329

hultberg commented Oct 5, 2021 •

edited

Loading

This comment has been minimized.

hultberg commented Oct 5, 2021

ro0NL commented Oct 5, 2021 •

edited

Loading

hultberg commented Oct 5, 2021

ro0NL commented Oct 5, 2021

nicolas-grekas Oct 28, 2021

ro0NL Oct 28, 2021

ogizanagi Feb 22, 2022 •

edited

Loading

misaert Dec 29, 2023

xabbuh Dec 30, 2023

ogizanagi left a comment

fabpot commented Jul 23, 2022

ogizanagi commented Jul 28, 2022

ogizanagi commented Jul 28, 2022

nicolas-grekas commented Jul 28, 2022

[Serializer] Respect default context in DateTimeNormalizer::denormalize #43329

[Serializer] Respect default context in DateTimeNormalizer::denormalize #43329

Conversation

hultberg commented Oct 5, 2021 • edited Loading

This comment has been minimized.

hultberg commented Oct 5, 2021

ro0NL commented Oct 5, 2021 • edited Loading

hultberg commented Oct 5, 2021

ro0NL commented Oct 5, 2021

nicolas-grekas Oct 28, 2021

Choose a reason for hiding this comment

ro0NL Oct 28, 2021

Choose a reason for hiding this comment

ogizanagi Feb 22, 2022 • edited Loading

Choose a reason for hiding this comment

misaert Dec 29, 2023

Choose a reason for hiding this comment

xabbuh Dec 30, 2023

Choose a reason for hiding this comment

ogizanagi left a comment

Choose a reason for hiding this comment

fabpot commented Jul 23, 2022

ogizanagi commented Jul 28, 2022

ogizanagi commented Jul 28, 2022

nicolas-grekas commented Jul 28, 2022

hultberg commented Oct 5, 2021 •

edited

Loading

ro0NL commented Oct 5, 2021 •

edited

Loading

ogizanagi Feb 22, 2022 •

edited

Loading