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

[Clock] Add Clock class and now() function #48642

Merged
merged 1 commit into from Dec 22, 2022

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Dec 14, 2022

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

See discussion on #48564.

This PR adds 2 static methods and one function:

namespace Symfony\Component\Clock;

Clock::get(): ClockInterface;
Clock::set(PsrClockInterface $clock): void;

now(): \DateTimeImmutable;

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

Last but not least, this PR also provides a ClockSensitiveTrait to help write test cases that rely on the clock. This trait provides one self::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).

Copy link
Member

@chalasr chalasr 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 :)

@nicolas-grekas
Copy link
Member Author

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

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

nicolas-grekas commented Dec 14, 2022

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 call Clock::with($clock, $callable, ...$arguments). This ensures the previous clock is restored after $callable has completed.

@upyx
Copy link
Contributor

upyx commented Dec 14, 2022

Do not add it to the API, please! I'm begging you! 🙏 It is easy to achieve by simple wrapper when it is really 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 like Laravels 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.

@GromNaN
Copy link
Member

GromNaN commented Dec 14, 2022

I plead guilty to having a Clock singleton class on my project. It is indeed the simplest way to replace the new 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 the clock service, but how do I override Clock::now()? Encapsulate the transaction like this?

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

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Dec 14, 2022

@GromNaN you nailed it :)

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Dec 15, 2022

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

@nicolas-grekas nicolas-grekas changed the title [Clock] Add Clock::now(), get() and with() [Clock] Add Clock class and now() function Dec 19, 2022
@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Dec 19, 2022

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

PR description updated.

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 if now() is not a native clock currently ?

Copy link
Member Author

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

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

nicolas-grekas added a commit that referenced this pull request Jan 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
@fabpot fabpot mentioned this pull request May 1, 2023
@BafS
Copy link
Contributor

BafS commented Jun 15, 2023

Why is there a now() function exactly? If we have a singleton already it seems simple enough to do Clock::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 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.

[Clock] Access global system clock