Skip to content
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.

By clicking “Sign 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

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

Merged

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

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?

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
8 of 11 checks passed
@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

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 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants