Skip to content

[HttpKernel]  Resolve DateTime value using the Clock #48098

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 “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

Jump to bottom
Merged
merged 1 commit into from
Dec 22, 2022
Merged

[HttpKernel]  Resolve DateTime value using the Clock #48098

merged 1 commit into from
Dec 22, 2022

Conversation

GromNaN
Copy link
Member

@GromNaN GromNaN commented Nov 3, 2022 •
edited
Loading

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 carsonbot added this to the 6.2 milestone Nov 3, 2022
@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 GromNaN force-pushed the clock-valud-resolver branch from de3a0fc to 60fe79a Compare November 3, 2022 15:32
@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 from ae845db to d2929e7 Compare November 26, 2022 20:12
@GromNaN GromNaN force-pushed the clock-valud-resolver branch 2 times, most recently from dcf32eb to c3bc63c Compare December 4, 2022 22:40
GromNaN
GromNaN commented Dec 4, 2022
View reviewed changes
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 GromNaN mentioned this pull request Dec 4, 2022
Merged
@GromNaN GromNaN force-pushed the clock-valud-resolver branch from c3bc63c to 76d71f6 Compare December 5, 2022 23:18
@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 •
edited
Loading

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 •
edited
Loading

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.

nicolas-grekas
nicolas-grekas approved these changes Dec 22, 2022
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 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 GromNaN force-pushed the clock-valud-resolver branch from acf7674 to 4917528 Compare December 22, 2022 14:45
@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 mentioned this pull request Dec 22, 2022
Closed
@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
@nicolas-grekas nicolas-grekas mentioned this pull request Jan 11, 2023
Merged
fabpot added a commit that referenced this pull request Jan 11, 2023
@fabpot
bug #48949 [HttpKernel] Use TZ from clock service in DateTimeValueRes…
f65e098 
…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
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

@stof stof stof left review comments

Assignees
No one assigned
Labels
Feature HttpKernel Status: Reviewed
Projects
None yet
Milestone
6.3
Development

Successfully merging this pull request may close these issues.
4 participants
@GromNaN @carsonbot @nicolas-grekas @stof