8000 [Serializer] Fix denormalizing date intervals having both weeks and days by oneNevan · Pull Request #52626 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Serializer] Fix denormalizing date intervals having both weeks and days #52626

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.

Sign up for GitHub

By clicking “Sig 8000 n 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

Jump to bottom
Merged
merged 1 commit into from
Nov 20, 2023
Merged

[Serializer] Fix denormalizing date intervals having both weeks and days #52626

merged 1 commit into from
Nov 20, 2023

Conversation

oneNevan
Copy link
Contributor
Q A
Branch? 5.4
Bug fix? yes
New feature? no
Deprecations? no
Issues Fix #47629
License MIT

Added support for date periods having both W and D period designators combined together (which is valid duration for ISO 8601-2). Starting from PHP 8 this is supported by \DateTimeInterval, see https://bugs.php.net/bug.php?id=61366 for reference

@oneNevan oneNevan requested a review from dunglas as a code owner November 17, 2023 12:46
@carsonbot carsonbot added this to the 5.4 milestone Nov 17, 2023
mtarld
mtarld approved these changes Nov 20, 2023
View reviewed changes
Copy link
Contributor
@mtarld mtarld left a comment

Choose a reason for hiding this comment

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

So, to make it work, we need to update the format.

Maybe it could be great to create as well a PR targeting 7.1 to update the format const value, WDYT?

nicolas-grekas
nicolas-grekas approved these changes Nov 20, 2023
View reviewed changes
Copy link
Member
@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

LGTM once comments are fixed.

@oneNevan oneNevan force-pushed the issue-47629-date-interval-normalizer branch from 7744b34 to c88d49e Compare November 20, 2023 09:47
@nicolas-grekas
Copy link
Member

Thank you @oneNevan.

@nicolas-grekas nicolas-grekas merged commit b2c68ce into symfony:5.4 Nov 20, 2023
@oneNevan oneNevan deleted the issue-47629-date-interval-normalizer branch November 20, 2023 09:58
@oneNevan
Copy link
Contributor Author
oneNevan commented Nov 20, 2023 •
edited
Loading

So, to make it work, we need to update the format.

Maybe it could be great to create as well a PR targeting 7.1 to update the format const value, WDYT?

@mtarld Regarding this, I agree it does make sense, but there is one thing to consider - adding %wW to FORMAT_KEY constant by default would break normalization as DateInterval::format does not support weeks, php docs does not provide it either https://www.php.net/manual/en/dateinterval.format.php - no weeks available :)
Which means we can only use W when denormalizing values from period strings into DateInterval instance.

So, it looks like if we want to support weeks out of the box, we would have to split FORMAT_KEY constant, into two constants: one for normalizer (without %wW), and another one for denormalizer (with %wW).

At this point I wanted to fix the regular expression at least, so that devs could use custom format if they deal with periods having weeks (as I do), but I'm not sure if it worth changing the current default format to support it out of the box, do you think it makes sense?

@mtarld
Copy link
Contributor
mtarld commented Nov 20, 2023

Indeed, it sounds like an edge case finally. I think that we might keep it like that, thank you for that feedback 🙂

This was referenced Nov 26, 2023
Merged
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@mtarld mtarld mtarld approved these changes

@dunglas dunglas Awaiting requested review from dunglas dunglas is a code owner

Assignees
No one assigned
Labels
Bug Serializer Status: Reviewed
Projects
None yet
Milestone
5.4
Development

Successfully merging this pull request may close these issues.
4 participants
@oneNevan @nicolas-grekas @mtarld @carsonbot
0