[Serializer] Deprecate using datetime construct as fallback on default format mismatch #47095
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
(rebase unlocked) |
88c8ff8 to
55be0a1
Compare
src/Symfony/Component/Serializer/Tests/Normalizer/DateTimeNormalizerTest.php
Show resolved
Hide resolved
| @@ -113,6 +113,8 @@ public function denormalize(mixed $data, string $type, string $format = null, ar | |||
| if (false !== $object) { | |||
| return $object; | |||
| } | |||
|
|
|||
| trigger_deprecation('symfony/serializer', '6.2', 'Relying on a datetime constructor as a fallback when using a specific default date format (`datetime_format`) for the DateTimeNormalizer is deprecated. Respect the "%s" default format.', $defaultDateTimeFormat); | |||
There was a problem hiding this comment.
What will the code look like in 7.0? Can we add a hint as a comment here?
There was a problem hiding this comment.
Depending on how we answer the questions in PR description, In 7.0, the fallback path will be either totally removed or disabled by default.
Suggested change
| trigger_deprecation('symfony/serializer', '6.2', 'Relying on a datetime constructor as a fallback when using a specific default date format (`datetime_format`) for the DateTimeNormalizer is deprecated. Respect the "%s" default format.', $defaultDateTimeFormat); | |
| trigger_deprecation('symfony/serializer', '6.2', 'Relying on a datetime constructor as a fallback when using a specific default date format (`datetime_format`) for the DateTimeNormalizer is deprecated and will be removed in 7.0. Respect the "%s" default format.', $defaultDateTimeFormat); |
There was a problem hiding this comment.
I'd got with 3 when format is auto.
3752548 to
8937432
Compare
|
PR updated to add the |
8937432 to
8bcd45b
Compare
8bcd45b to
41a1d56
Compare
|
This didn't get traction so I guess this isn't much desired. On my side at least I don't see why this would be an improvement. I'd prefer closing. |
|
I think I agree with @nicolas-grekas here. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is a follow-up to #43329 and aims to enforce the configured default format is always respected, instead of silently fallback to the
\DateTime&\DateTimeImmutableconstructors.As of today, the only way to enforce this format is to set it locally through the context.
For now, the PR just adds the deprecation in case the fallback path is reached. One could argue this might be enough, since not respecting the expected format as an end user of an API might be considered as an issue in the first place.
Still:
Do we need a better upgrade path?
e.g:
\DateTimeNormalizer::USE_DATETIME_CONSTRUCT_AS_FALLBACK => false,and trigger a deprec if this flag is not set. Default 6.2:
true, 7.0:falseDo we want to keep the ability to use the datetime constructs as fallbacks?
e.g:
\DateTimeNormalizer::USE_DATETIME_CONSTRUCT_AS_FALLBACK => false,Then, this behavior should be consistent whenever a default format or a context one is used (there is no fallback right now if set from the context)
Do we want to keep the datetime constructs call whenever the default format is
null?Then, one should be able to "unset" the default format by passing in the context:
(currently fallbacks to the default format) or use a special value to do so (
php-datetime-string?).