[Serializer] Trigger deprecation when could not parse date with default format #58966
Conversation
|
@nicolas-grekas Could you have a look? |
src/Symfony/Component/Serializer/Normalizer/DateTimeNormalizer.php
Outdated
Show resolved
Hide resolved
|
Looks like the tests must be updated to not trigger the deprecation. |
| @@ -126,6 +126,8 @@ public function denormalize(mixed $data, string $type, ?string $format = null, a | |||
| if (false !== $object = $type::createFromFormat($defaultDateTimeFormat, $data, $timezone)) { | |||
| return $object; | |||
| } | |||
|
|
|||
| trigger_deprecation('symfony/serializer', '7.3', \sprintf('A "%s" will be thrown when a date could not be parsed using the default format "%s".', NotNormalizableValueException::class, $defaultDateTimeFormat)); | |||
There was a problem hiding this comment.
I think we should trigger a deprecation here only if the fallback manages to instantiate it. The case where the fallback throws an exception (already turned into a NotNormalizableValueException below) should not trigger a deprecation.
There was a problem hiding this comment.
As I understand, in 8.0, the fallback should be removed, right?
There was a problem hiding this comment.
yes. And that's my point: if the fallback also fails, you already have the same behavior than what would happen in 8.0 and we should not trigger a deprecation in that case as the behavior will not change. Try submitting foobar as value to reproduce that case.
There was a problem hiding this comment.
I have rechecked and now realise that fallback should not be removed in 8.0. Fallback should still be used when datetime_format is set to null.
There was a problem hiding this comment.
My point is still not solved. You still trigger a deprecation for not normalizable values (passing abcdefgh as date for instance), while there is no behavior change for that case.
There was a problem hiding this comment.
After removing return new $type($data, $timezone); (line 133/131), it will throw NotNormalizableValueException when we pass a valid date but the format does not match $defaultDateTimeFormat.
For example, if $defaultDateTimeFormat = 'Y-m-d H:i:s' and we pass 2025-03-10 it will work.
In Symfony 8, it should throw an exception.
As I understand, the behaviour will change.
There was a problem hiding this comment.
I'm not talking about the case of passing a valid date not matching the format.
Your current implementation is also triggering the deprecation when passing a totally invalid date. That's because you trigger the deprecation before trying to use the fallback, without considering whether that fallback succeeds or fails.
There was a problem hiding this comment.
I think I finally understood you. Look again, please.
66a1ede to
663eec5
Compare
|
@mtarld I have done with your comments. |
6ef37ea to
b6f425d
Compare
Relates to #43329 (comment)