Skip to content

[HttpKernel] Add DateTimeValueResolver #45589

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

Merged
merged 1 commit into from
Mar 25, 2022

Conversation

codedmonkey
Copy link
Contributor

@codedmonkey codedmonkey commented Feb 28, 2022

Q A
Branch? 6.1
Bug fix? no
New feature? yes
Deprecations? no
Tickets #44705
License MIT
Doc PR symfony/symfony-docs#16562

This PR replaces the DateTimeParamConverter from the SensioFrameworkExtraBundle with a value resolver in the Symfony Core.

Note that the behavior of the resolver is enabled by default when using the Symfony framework.

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.

I agree with @derrabus' comment

@codedmonkey
Copy link
Contributor Author

codedmonkey commented Mar 22, 2022

I think the tests are failing because of Daylight Saving Time 🥇

Edit: Probably something else now.

@codedmonkey codedmonkey force-pushed the datetime-resolver branch 2 times, most recently from 394e940 to c39a39b Compare March 23, 2022 19:47
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.

I fixed the remaining issue, added a line to the changelog and fixed the priority of the other resolvers.

@fabpot
Copy link
Member

fabpot commented Mar 25, 2022

Thank you @codedmonkey.

@fabpot fabpot merged commit dfc7c08 into symfony:6.1 Mar 25, 2022
@GromNaN
Copy link
Member

GromNaN commented Mar 26, 2022

Some tests are failing depending on the default timezone. #45859

fabpot added a commit that referenced this pull request Mar 27, 2022
This PR was merged into the 6.1 branch.

Discussion
----------

[HttpKernel] Fix timezone influenced tests

| Q             | A
| ------------- | ---
| Branch?       | 6.1
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | #45589 (comment)
| License       | MIT
| Doc PR        | -

`DateTimeValueResolverTest` fails depending on the default timezone and the current time.

The culprit is the combinaison of `strtotime` + `new DateTime`.
https://github.com/symfony/symfony/blob/24304ad77874f03f9971fd9e29c0ef0776b0f5b9/src/Symfony/Component/HttpKernel/Controller/ArgumentResolver/DateTimeValueResolver.php#L67-L69

```
There were 3 failures:

1) Symfony\Component\HttpKernel\Tests\Controller\ArgumentResolver\DateTimeValueResolverTest::testFullDate
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'2012-07-21'
+'2012-07-20'

/home/runner/work/symfony/symfony/src/Symfony/Component/HttpKernel/Tests/Controller/ArgumentResolver/DateTimeValueResolverTest.php:53

2) Symfony\Component\HttpKernel\Tests\Controller\ArgumentResolver\DateTimeValueResolverTest::testCustomClass
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'2016-09-08'
+'2016-09-07'

/home/runner/work/symfony/symfony/src/Symfony/Component/HttpKernel/Tests/Controller/ArgumentResolver/DateTimeValueResolverTest.php:100

3) Symfony\Component\HttpKernel\Tests\Controller\ArgumentResolver\DateTimeValueResolverTest::testDateTimeImmutable
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'2016-09-08'
+'2016-09-07'

/home/runner/work/symfony/symfony/src/Symfony/Component/HttpKernel/Tests/Controller/ArgumentResolver/DateTimeValueResolverTest.php:116
```

Commits
-------

9a0e99c Fix timezone influenced tests
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Jun 6, 2022
… (codedmonkey)

This PR was merged into the 6.1 branch.

Discussion
----------

[Serializer] Add information about DateTimeValueResolver

Adds documentation for symfony/symfony#45589.

I think it would be a good idea to explain how to use the `MapDateTime` attribute, but I opted to keep it brief.

Commits
-------

9c910d7 Add information about DateTimeValueResolver
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.

8 participants