-
Notifications
You must be signed in to change notification settings - Fork 753
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
Allow time to be fetched from a universal clock ⏱ #866
base: master
Are you sure you want to change the base?
Conversation
Codecov Report
@@ 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
|
I like it
IMO if this approach is taken, the static method should just be removed in favor of your approach.
I'm not a maintainer here either, so I'll also punt, just my opinion |
/** | ||
* Represents an implementation of a Clock. | ||
*/ | ||
class Clock |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason not to have something like ClockInterface
here to require the now()
method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Im all for it but interfaces seem to be used sparingly on this project... I wish providers had an interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think an interface is appropriate here, because the implementation of ClockInterface
should be final class ...
.
* | ||
* @return int | ||
*/ | ||
public function getTimeNow(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both of these methods added to the interface represent a BC break.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, of course
* | ||
* @return void | ||
*/ | ||
public function setClock(Clock $clock); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typically, we want interfaces to define things we can fetch, not necessarily how they are set. The way something is set could be left open to each implementation, but we'd want to ensure that all implementations have a way to get the clock (i.e., getClock(): Clock
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, I dont understand what you're saying here. If this is about clock interfaces, im all for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm saying I would prefer to define a getClock()
method on the AccessTokenInterface
, rather than a setClock()
method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes to this interface don't make sense. An access token does not have a "clock", it has an expiry date/time. I think the only reasonable modification here would be AccessTokenInterface::getExpiresDate()
that returns DateTimeImmutable
. Given that getExpires
returns a UNIX timestamp already, it makes no sense for the access token to have a separate clock.
* | ||
* @return int | ||
*/ | ||
public function getTimeNow(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you choose to add getClock(): Clock
, and the Clock has a now(): DateTimeInterface
method, then is there any reason to have getTimeNow()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont see how both getTimeNow
and a theoretical public getClock()
method would be useful for access tokens. See also my note about whether you'd prefer for the previously implemented time functionality (getTimeNow
/setTimeNow
/resetTimeNow
/\League\OAuth2\Client\Token\AccessToken::$timeNow
) to be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you add a getClock()
method, then I agree that getTimeNow()
is no longer necessary.
} elseif (isset($this->clock)) { | ||
return $this->clock->now()->getTimestamp(); | ||
} else { | ||
return time(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you implement a getClock(): Clock
method that must always return a Clock, then there's no need to check for isset($this->clock)
. Instead, you can do:
if (self::$timeNow) {
return self::$timeNow;
}
return $this->getClock()->now()->getTimestamp();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better for self::$timeNow
to be a DateTimeImmutable
, rather than an integer.
* | ||
* @return \DateTimeImmutable | ||
*/ | ||
public function now() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this method should be getTime()
instead of now()
, so that Clock could return any DateTime object that an implementer wants to set (fixed, now, etc.)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used now
as thats the same method implemented by the reference clock projects linked in summary. Also a getTime
is strictly speaking not appropriate as you're getting more than just time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now()
is my preference.
I think a lot of this hinges on whether maintainer prefers previous time and clock to co-exist:
|
IMO, the rationale for a
Requiring |
/** | ||
* Represents an implementation of a Clock. | ||
*/ | ||
class Clock |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think an interface is appropriate here, because the implementation of ClockInterface
should be final class ...
.
* | ||
* @return \DateTimeImmutable | ||
*/ | ||
public function now() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now()
is my preference.
} elseif (isset($this->clock)) { | ||
return $this->clock->now()->getTimestamp(); | ||
} else { | ||
return time(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better for self::$timeNow
to be a DateTimeImmutable
, rather than an integer.
* | ||
* @return void | ||
*/ | ||
public function setClock(Clock $clock); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes to this interface don't make sense. An access token does not have a "clock", it has an expiry date/time. I think the only reasonable modification here would be AccessTokenInterface::getExpiresDate()
that returns DateTimeImmutable
. Given that getExpires
returns a UNIX timestamp already, it makes no sense for the access token to have a separate clock.
/** | ||
* @var \DateTimeImmutable|null | ||
*/ | ||
protected $time = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why wouldn't this be initialized via __construct
?
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. |
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 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. |
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: dpi#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: #866 (comment) |
This should use the PSR-20 ClockInterface |
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()
andstrtotime()
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
time()
andstrtotime()
where possible. Only 2x calls to time() remain where the true current time is required for getting and testing default time.\League\OAuth2\Client\Token\AccessTokenInterface::getTimeNow
to interface, which was defined in Allow time to be set for test purposes #852\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.