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

[HttpKernel]  Resolve DateTime value using the Clock #48098

Merged
merged 1 commit into from
Dec 22, 2022

Conversation

GromNaN
Copy link
Member

@GromNaN GromNaN commented Nov 3, 2022

Q A
Branch? 6.3
Bug fix? no
New feature? yes
Deprecations? no
Tickets -
License MIT
Doc PR TODO

In order to mock the current time in functional test cases by injecting a Symfony\Component\Clock\MockClock as clock service. This is necessary when a DateTimeInterface argument is not nullable and we expect the current date time by default.

Example when 2 routes are configured on the same controller with an optional parameter.

class TvGridController
{
    #[Route('/', name: 'prime_now')]
    #[Route('/{date}', name: 'prime_date']
    public function prime(\DateTimeInterface $date)
    {
        return new Response('Grid for date: '.$date->format('Y-m-d'));
    }
}

@carsonbot
Copy link

Hey!

Thanks for your PR. You are targeting branch "6.2" but it seems your PR description refers to branch "6.3".
Could you update the PR description or change target branch? This helps core maintainers a lot.

Cheers!

Carsonbot

@carsonbot carsonbot changed the title [HttpKernel] Resolve DateTime value using the Clock [HttpKernel]  Resolve DateTime value using the Clock Nov 3, 2022
@GromNaN GromNaN modified the milestones: 6.2, 6.3 Nov 3, 2022
@GromNaN
Copy link
Member Author

GromNaN commented Nov 3, 2022

From php doc:

If format does not contain the character ! then portions of the generated date/time which are not specified in format will be set to the current system time.

It looks like we cannot change the reference date when using DateTime::createFromFormat. This means we cannot use the clock when format is specified.

@GromNaN GromNaN force-pushed the clock-valud-resolver branch 2 times, most recently from dcf32eb to c3bc63c Compare December 4, 2022 22:40
Copy link
Member Author

@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

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

Rebased. Ready for a last review.

@GromNaN
Copy link
Member Author

GromNaN commented Dec 5, 2022

I tried to use the new ClockAwareTrait from #48362, but I think we should not have symfony/clock as a required dependency to use this class.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Dec 22, 2022

Now that #48642 is merged, what about leveraging Clock::get()->now()?
Making clock a required dep might be legit. Because this static class is just a wrapper around PSR-20, it doesn't create any strong coupling to the clock implementations in the package so it might be fine.

@GromNaN
Copy link
Member Author

GromNaN commented Dec 22, 2022

Dependency injection is simpler here since this is already a service, and this is easy to test. I don't see the benefit of the global object: that adds a required dependency to symfony/clock and the tests would required a call to mockTime.

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 thanks (small rebase needed)

@GromNaN GromNaN force-pushed the clock-valud-resolver branch 2 times, most recently from 9319d95 to acf7674 Compare December 22, 2022 11:43
@nicolas-grekas
Copy link
Member

(tests fail)

@GromNaN
Copy link
Member Author

GromNaN commented Dec 22, 2022

Yes sorry, that's fixed.

@nicolas-grekas
Copy link
Member

Thank you @GromNaN.

@nicolas-grekas nicolas-grekas merged commit dfcb811 into symfony:6.3 Dec 22, 2022
@GromNaN GromNaN deleted the clock-valud-resolver branch December 22, 2022 16:11
fabpot added a commit that referenced this pull request Jan 11, 2023
…olver (nicolas-grekas)

This PR was merged into the 6.3 branch.

Discussion
----------

[HttpKernel] Use TZ from clock service in DateTimeValueResolver

| Q             | A
| ------------- | ---
| Branch?       | 6.3
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

Fixing #48098 + making tests green by using legit TZ names.

Commits
-------

1acf90e [HttpKernel] Use TZ from clock service in DateTimeValueResolver
@fabpot fabpot mentioned this pull request May 1, 2023
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