Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[Clock] AddClock class andnow() function#48642

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
nicolas-grekas merged 1 commit intosymfony:6.3fromnicolas-grekas:clock-now
Dec 22, 2022

Conversation

@nicolas-grekas
Copy link
Member

@nicolas-grekasnicolas-grekas commentedDec 14, 2022
edited
Loading

QA
Branch?6.3
Bug fix?no
New feature?yes
Deprecations?no
TicketsFix#48564
LicenseMIT
Doc PR-

See discussion on#48564.

This PR adds 2 static methods and one function:

namespaceSymfony\Component\Clock;Clock::get(): ClockInterface;Clock::set(PsrClockInterface$clock): void;now(): \DateTimeImmutable;

It also wires this global clock as a service so that injecting theClockInterface or usingnow /Clock::get() returns the same time.

Last but not least, this PR also provides aClockSensitiveTrait to help write test cases that rely on the clock. This trait provides oneself::mockTime() method and it restores the global clock after each test case.

mockTime() accepts either a string (eg'+1 days' or'2022-12-22'), a DTI instance, or a boolean (to freeze/restore the global clock).

ro0NL, ging-dev, and kbond reacted with thumbs up emojichapterjason and ging-dev reacted with thumbs down emojiKocal, jvasseur, chapterjason, ging-dev, sukei, and BafS reacted with confused emoji
@nicolas-grekasnicolas-grekasforce-pushed theclock-now branch 2 times, most recently frombe99e56 toe2e654fCompareDecember 14, 2022 16:45
Copy link
Member

@chalasrchalasr left a comment

Choose a reason for hiding this comment

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

Not sure that hijacked enum is better than a regular class with private+final construct, but it works and made me laugh :)

kaznovac reacted with thumbs up emojichapterjason reacted with laugh emoji
@nicolas-grekas
Copy link
MemberAuthor

Less boilerplate FTW (+ newInstanceWithoutConstructor won't allow bypassing ;) )

@nicolas-grekasnicolas-grekas changed the title[Clock] Add Clock::now()[Clock] Add Clock::now(), get() and with()Dec 14, 2022
@nicolas-grekas
Copy link
MemberAuthor

nicolas-grekas commentedDec 14, 2022
edited
Loading

Thanks to various comments, I figured out a better API for this global clock.

There is no setter anymore.

To use a custom clock, one now has to callClock::with($clock, $callable, ...$arguments). This ensures the previous clock is restored after$callable has completed.

@upyx
Copy link
Contributor

Do not add it to the API, please! I'm begging you! 🙏 It is easy to achieve by simple wrapper when it isreally needed.

Static calls to some global state can be useful in some code, but most of the time it leads to maintenance problems and bugs. People would use it without reason if that was in the API.

The feature looks likeLaravels Facades. Every time I've seen them used, they are used wrong. Not because the Facades are something bad, but because developers (especially not very experienced) do bad things with them. Facades are constantly overused/abused just because it is convenient, they are described in guides, and "they all do that, why I shouldn't?"

The good answer to how to inject time (and other dependencies) into Doctrines' entities is repository. Repositories are the best place to provide entities, including new ones. If it's difficult in your case or doesn't suit your needs, there are many other options#48564 (comment) But...Please, do not add static calls to the API.

chapterjason, zmajciki, Kocal, jvasseur, kaznovac, andrew-demb, KevinVanSonsbeek, sukei, and BafS reacted with thumbs up emoji

@GromNaN
Copy link
Member

I plead guilty to having aClock singleton class on my project. It is indeed the simplest way to replace thenew DateTime(); even if a dependency injection properly done would be clearer.

With this implementation, I don't see how I could override the current time for functional testing. I can redefine theclock service, but how do I overrideClock::now()? Encapsulate the transaction like this?

$client =static::createClient();$crawler = Clock::with($mockClock,fn() =>$client->request('GET','/'));

@nicolas-grekas
Copy link
MemberAuthor

nicolas-grekas commentedDec 14, 2022
edited
Loading

@GromNaN you nailed it :)

@nicolas-grekas
Copy link
MemberAuthor

nicolas-grekas commentedDec 15, 2022
edited
Loading

I addedClock::get([$newClock]): ClockInterface back since I realized that forcing a closure around might not fit all styles - eg a pre+post event listener.

@nicolas-grekasnicolas-grekas changed the title[Clock] Add Clock::now(), get() and with()[Clock] AddClock class andnow() functionDec 19, 2022
@nicolas-grekas
Copy link
MemberAuthor

nicolas-grekas commentedDec 19, 2022
edited
Loading

Thanks for the feedback. PR updated. It now provides integration with services + phpunit.

PR description updated.

alessandro-podo reacted with thumbs up emoji

true === $when => new MockClock(),
$when instanceof ClockInterface => $when,
$when instanceof \DateTimeImmutable => new MockClock($when),
default => new MockClock(now()->modify($when)),
Copy link
Member

Choose a reason for hiding this comment

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

wouldn't this be confusing ifnow() is not a native clock currently ?

Copy link
MemberAuthor

@nicolas-grekasnicolas-grekasDec 19, 2022
edited
Loading

Choose a reason for hiding this comment

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

If it's not a native clock, it's usually a mock clock. This behavior is desired to me, to do eg:

self::mockTime();// freeze//...self::mockTime('+1 days');// frozen time + 1 day

ro0NL reacted with thumbs up emoji
Comment on lines +17 to +18
* Note that you should prefer injecting a ClockInterface or using
* ClockAwareTrait when possible instead of using this function.
Copy link
Contributor

Choose a reason for hiding this comment

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

image

chapterjason reacted with laugh emoji
nicolas-grekas added a commit that referenced this pull requestJan 19, 2023
…lt (nicolas-grekas)This PR was merged into the 6.3 branch.Discussion----------[Clock] Make ClockAwareTrait use the global clock by default| Q             | A| ------------- | ---| Branch?       | 6.3| Bug fix?      | yes| New feature?  | no| Deprecations? | no| Tickets       | -| License       | MIT| Doc PR        | -Let's make `ClockAwareTrait` compatible with `Clock::now()` from#48642 by default.Commits-------ec60013 [Clock] Make ClockAwareTrait use the global clock by default
@fabpotfabpot mentioned this pull requestMay 1, 2023
@BafS
Copy link
Contributor

Why is there anow() function exactly? If we have a singleton already it seems simple enough to doClock::get()->now(). Functions are not "lazily" imported (not autoloaded) and it can be added easily in userland if needed, in the other hands we need to add some rules to be sure we don't use this function in our code.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@stofstofstof left review comments

@chalasrchalasrchalasr approved these changes

@kbondkbondkbond approved these changes

@GromNaNGromNaNGromNaN approved these changes

@derrabusderrabusderrabus approved these changes

@ycerutoycerutoyceruto approved these changes

+5 more reviewers

@jdreesenjdreesenjdreesen left review comments

@ro0NLro0NLro0NL left review comments

@fancywebfancywebfancyweb approved these changes

@zmajcikizmajcikizmajciki left review comments

@fmatafmatafmata approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

6.3

Development

Successfully merging this pull request may close these issues.

[Clock] Access global system clock

15 participants

@nicolas-grekas@upyx@GromNaN@BafS@kbond@jdreesen@stof@ro0NL@fmata@derrabus@yceruto@fancyweb@zmajciki@chalasr@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp