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

DateIntervalNormalizer does not support weeks and days together #47629

Closed
oneNevan opened this issue Sep 20, 2022 · 3 comments
Closed

DateIntervalNormalizer does not support weeks and days together #47629

oneNevan opened this issue Sep 20, 2022 · 3 comments

Comments

@oneNevan
Copy link
Contributor

Symfony version(s) affected

5.4.12,6.1.4

Description

Since PHP 8.0 DateInterval::construct supports periods containing both D and W period designators

echo (new \DateInterval('P3W2D'))->format('%d');
// outputs '2' prior PHP 8.0
// outputs '23' in PHP >= 8.0

The problem is the built-in DateIntervalNormalizer does not support it, so it throws exception if both weeks and date exist in source period string as current regular expression in method isISO8601 does not allow it

        if (!$this->isISO8601($data)) {
            throw new UnexpectedValueException('Expected a valid ISO 8601 interval string.');
        }

How to reproduce

// PHP >= 8.0
$normalizer = new \Symfony\Component\Serializer\Normalizer\DateIntervalNormalizer();
$object = $normalizer->denormalize('P2W6D', \DateInterval::class, null, [
    'dateinterval_format' => '%rP%yY%mM%wW%dDT%hH%iM%sS',
]);
// expected: $object instanceof \DateInterval
// actual: \Symfony\Component\Serializer\Exception\UnexpectedValueException thrown

Possible Solution

I believe a possible solution would be to update regular expression used in isISO8601 method for PHP versions >= 8.0, like this:

    private function isISO8601(string $string): bool
    {
        if (PHP_VERSION_ID >= 80000) {
            return preg_match('/^[\-+]?P(?=\w*(?:\d|%\w))(?:\d+Y|%[yY]Y)?(?:\d+M|%[mM]M)?(?:\d+W|%[wW]W)?(?:\d+D|%[dD]D)?(?:T(?:\d+H|[hH]H)?(?:\d+M|[iI]M)?(?:\d+S|[sS]S)?)?$/', $string);
        }

        return preg_match('/^[\-+]?P(?=\w*(?:\d|%\w))(?:\d+Y|%[yY]Y)?(?:\d+M|%[mM]M)?(?:(?:\d+D|%[dD]D)|(?:\d+W|%[wW]W))?(?:T(?:\d+H|[hH]H)?(?:\d+M|[iI]M)?(?:\d+S|[sS]S)?)?$/', $string);
    }

Additional Context

No response

@carsonbot
Copy link

Hey, thanks for your report!
There has not been a lot of activity here for a while. Is this bug still relevant? Have you managed to find a workaround?

@carsonbot
Copy link

Just a quick reminder to make a comment on this. If I don't hear anything I'll close this.

@oneNevan
Copy link
Contributor Author

oneNevan commented Jul 4, 2023

this is still an issue, let's keep it open, I think I can open a PR for it, maybe sometime this week

@carsonbot carsonbot removed the Stalled label Jul 4, 2023
nicolas-grekas added a commit that referenced this issue Nov 20, 2023
…weeks and days (oneNevan)

This PR was merged into the 5.4 branch.

Discussion
----------

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

| 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

Commits
-------

c88d49e [Serializer] Fix denormalizing date intervals having both weeks and days
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants