Allow time to be fetched from a universal clock ⏱

[dpi]

What a coincidence I've been putting together an OAuth project for Drupal, and in order to complete unit tests I needed a way to mock time. Thanks for the timely implementation of #852 @ramsey @kevinquinnyo

Purpose

Allow time to be fetched from a universal clock and eliminate usages of time() and strtotime() where possible.

Implementing a clock, which is useful for the Drupal Authman project. In Drupal we consider the now-time throughout the lifetime of a request to be the time at the start of the request, not the true current time. So for integration with Drupal I expect this method the proposed clock feature to be used always. For integration with other projects, its also necessary to obey time travel when other projects request it.

Implementation notes

• Eliminated remaining calls to time() and strtotime() where possible. Only 2x calls to time() remain where the true current time is required for getting and testing default time.
• Added \League\OAuth2\Client\Token\AccessTokenInterface::getTimeNow to interface, which was defined in #852
• Rewrote comments such as "Set the time now. This should only be used for testing purposes.".
• Switched tests to always simulate time where possible, to eliminate possible random test failures.
• Given that the work in #852 was designed for tests only, not for public API, I defer to maintainers to whether to remove the static time method stored in \League\OAuth2\Client\Token\AccessToken::$timeNow in favor of the clock.

Clock

Inspiration for the clock comes from a few examples I'm familiar with, such as two popular Clock projects on Packagist, and Drupal.

• https://github.com/kreait/clock-php/blob/main/src/Clock/SystemClock.php
• https://github.com/lcobucci/clock/blob/2.1.x/src/SystemClock.php
• Drupal's https://git.drupalcode.org/project/drupal/-/blob/9.2.x/core/lib/Drupal/Component/Datetime/TimeInterface.php

[codecov[bot]]

Codecov Report

Merging #866 (d286c6a) into master (419d163) will not change coverage. The diff coverage is 100.00%.

@@             Coverage Diff             @@
##              master      #866   +/-   ##
===========================================
  Coverage     100.00%   100.00%           
- Complexity       180       187    +7     
===========================================
  Files             20        21    +1     
  Lines            464       483   +19     
===========================================
+ Hits             464       483   +19     

Impacted Files	Coverage Δ	Complexity Δ
src/Provider/AbstractProvider.php	100.00% <100.00%> (ø)	67.00 <2.00> (+3.00)
src/Provider/Clock.php	100.00% <100.00%> (ø)	1.00 <1.00> (?)
src/Token/AccessToken.php	100.00% <100.00%> (ø)	30.00 <1.00> (+3.00)

[kevinquinnyo]

I like it

Given that the work in #852 was designed for tests only, not for public API, I defer to maintainers to whether to remove the static time method stored in \League\OAuth2\Client\Token\AccessToken::$timeNow in favor of the clock.

IMO if this approach is taken, the static method should just be removed in favor of your approach.

1. It was only recently merged, so it's likely no one is utilizing this in their downstream tests (not even me and I authored the PR that added it)
2. It would only affect test code, not production code on the off-chance there were a BC issue.
3. This project already seems to have a lot of BC related juggling as-is, so it seems like a good idea not to introduce yet another one
4. Explicit dependency injection is usually favorable to hidden dependencies (like the static method to fake the time)

I'm not a maintainer here either, so I'll also punt, just my opinion

[dpi]

I think a lot of this hinges on whether maintainer prefers previous time and clock to co-exist:

• Remove or keep multiple time methods
• Add a public getClock method? Per above I dont see a valid rationale, yet.

[ramsey]

Add a public getClock method? Per above I don't see a valid rationale, yet.

IMO, the rationale for a getClock() method is in your purpose statement for this:

Allow time to be fetched from a universal clock and eliminate usages of time() and strtotime() where possible

Requiring setClock() doesn't enforce anything, since any implementation could treat that as a no-op, especially since properties (i.e., protected $clock) aren't part of the contract. If we want to force implementations to respect the contract, then we need to define getClock(): Clock that must return a Clock instance. Then, wherever the Clock should be used in the parent classes, we use getClock() instead of $this->clock. The parent shouldn't care how the Clock is set, as long as getClock() returns a Clock.

[shadowhand]

The goal is this PR is excellent, and I definitely support the concept of an internal clock. The implementation just needs a little work to be logical.

[dpi]

I dont have bandwidth or motivation to work on this right now, but i just want to make-everyone-aware of PSR-20 https://github.com/php-fig/fig-standards/blob/master/proposed/clock.md, which in summary is an interface, with a now(), method, returning a \DateTimeImmutable instance. So whatever happens here should aim to be compliant with this, which is basically a standardisation of what other clock projects have arrived at.

Because of this I see it making sense to eventually make PSR be the interface to depend on, so that could mean defining an interface here in oauth2-client, then later making the interface here extend the one from PSR-20.

[kevinquinnyo]

I dont have bandwidth or motivation to work on this right now

Understandable. I had a little bit of extra time this afternoon so I figured I'd take a stab at it. I've attempted to apply the code review changes as a PR to your branch @dpi here: https://github.com/dpi/oauth2-client/pull/1

@ramsey @shadowhand let me know if I've missed anything or if I'm off the mark with those changes.

I think one thing I didn't address is this comment: https://github.com/thephpleague/oauth2-client/pull/866#discussion_r671718866

[stof]

This should use the PSR-20 ClockInterface