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

New PSR for ClockInterface #1224

Merged
merged 58 commits into from
Mar 24, 2021
Merged
Show file tree
Hide file tree
Changes from 37 commits
Commits
Show all changes
58 commits
Select commit Hold shift + click to select a range
31f93b6
Create Clock PSR
cseufert Mar 4, 2021
faa28e1
Added Clock PSR Meta
cseufert Mar 4, 2021
0065811
added method to retreieve current timezone
cseufert Mar 4, 2021
d6c75b2
Updated timezone return types
cseufert Mar 4, 2021
9974dfb
Update proposed/clock-meta.md to fix typos
cseufert Mar 4, 2021
0c66162
Update proposed/clock-meta.md to fix typos
cseufert Mar 4, 2021
abb943b
Update proposed/clock-meta.md to fix typos
cseufert Mar 4, 2021
0df6d63
Update proposed/clock-meta.md to fix typos
cseufert Mar 4, 2021
072514e
Update purpose of document
cseufert Mar 4, 2021
2bc8e21
DateTime is out
cseufert Mar 4, 2021
fed0313
Update clock.md fixed typo
cseufert Mar 4, 2021
9c3d13f
Update clock.md update code formatting
cseufert Mar 4, 2021
82f07eb
Update proposed/clock-meta.md
cseufert Mar 8, 2021
c8bcefe
Update proposed/clock-meta.md
cseufert Mar 8, 2021
5415f5f
Simplify the ClockInterface
cseufert Mar 8, 2021
3386929
Include example implementations
cseufert Mar 8, 2021
4395fc3
Added names for sponsors and working group
cseufert Mar 9, 2021
d1bf6cf
Updated Working Group Members
cseufert Mar 10, 2021
506b1fc
added Luis to WG
cseufert Mar 11, 2021
380089c
Update proposed/clock.md
cseufert Mar 11, 2021
5c17173
Update proposed/clock-meta.md
cseufert Mar 11, 2021
650f6d2
Update proposed/clock-meta.md
cseufert Mar 15, 2021
05f2fba
Update proposed/clock-meta.md
cseufert Mar 16, 2021
18585b5
escape package names
cseufert Mar 18, 2021
8d3e388
clean up relevant links format
cseufert Mar 18, 2021
0ad441a
code formatted interface name
cseufert Mar 18, 2021
078ec7b
Update proposed/clock-meta.md
cseufert Mar 18, 2021
2e13cb5
Update proposed/clock-meta.md
cseufert Mar 18, 2021
a79da23
Update proposed/clock-meta.md
cseufert Mar 18, 2021
0d7157c
Update proposed/clock-meta.md
cseufert Mar 18, 2021
4f13a8e
Update proposed/clock.md
cseufert Mar 18, 2021
fa9323d
Update proposed/clock.md
cseufert Mar 18, 2021
17f364a
Update proposed/clock.md
cseufert Mar 18, 2021
8a701b2
Update proposed/clock-meta.md
cseufert Mar 18, 2021
509163c
Update proposed/clock-meta.md
cseufert Mar 18, 2021
8b34135
Update proposed/clock-meta.md
cseufert Mar 18, 2021
6bd1916
Update clock-meta.md
cseufert Mar 18, 2021
0338682
Added entrace vote link
cseufert Mar 22, 2021
2734e16
Update proposed/clock-meta.md
cseufert Mar 24, 2021
c279903
Update proposed/clock-meta.md
cseufert Mar 24, 2021
6ada868
Update proposed/clock-meta.md
cseufert Mar 24, 2021
4b53d45
Update proposed/clock-meta.md
cseufert Mar 24, 2021
92c4244
Update proposed/clock-meta.md
cseufert Mar 24, 2021
3a8c4b1
Update proposed/clock-meta.md
cseufert Mar 24, 2021
87c4002
Update proposed/clock-meta.md
cseufert Mar 24, 2021
5e05b77
Update proposed/clock-meta.md
cseufert Mar 24, 2021
9f8115b
Update proposed/clock-meta.md
cseufert Mar 24, 2021
1ced885
Update proposed/clock.md
cseufert Mar 24, 2021
12e1c1c
Update proposed/clock.md
cseufert Mar 24, 2021
2de88c5
Update proposed/clock.md
cseufert Mar 24, 2021
9401671
Re-ordered WG member list
cseufert Mar 24, 2021
3688544
Update Reference example implemtations
cseufert Mar 24, 2021
9ef42e5
Fix Order
cseufert Mar 24, 2021
8826633
Removed timezones from usage patterns
cseufert Mar 24, 2021
8e9aace
Update symfony details & added Chronos details
cseufert Mar 24, 2021
66fac09
Update proposed/clock-meta.md
cseufert Mar 24, 2021
81cb449
Update proposed/clock-meta.md
cseufert Mar 24, 2021
3caffc3
Update proposed/clock-meta.md
cseufert Mar 24, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
155 changes: 155 additions & 0 deletions proposed/clock-meta.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,155 @@
# Clock Meta Document

## 1. Summary

The purpose of using the `ClockInterface` is to provide a standard way to access the system
time, that would allow interopability when testing code that relies on the current time
rather than relying installing PHP extensions or use hacks like re-declaring the `time()`
cseufert marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
rather than relying installing PHP extensions or use hacks like re-declaring the `time()`
rather than relying on installing PHP extensions or using hacks like re-declaring the `time()`

function in other namespaces.

## 2. Why Bother?

There are currently a few libraries that do provide the functionality on packagist, however
cseufert marked this conversation as resolved.
Show resolved Hide resolved
there is no interopability between these different libraries, as they ship with their own
clock interfaces. Symfony has a TimeMock library which uses namespace hacks to override the
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: Symfony does not have a TimeMock library. Symfony provide a package named symfony/phpunit-bridge with a Symfony\Bridge\PhpUnit\ClockMock class:

The Symfony\Bridge\PhpUnit\ClockMock class provided by this bridge allows you to mock the PHP’s built-in time functions time(), microtime(), sleep(), usleep() and gmdate(). Additionally the function date() is mocked so it uses the mocked time if no timestamp is specified.

Other functions with an optional timestamp parameter that defaults to time() will still use the system time instead of the mocked time. This means that you may need to change some code in your tests. For example, instead of new DateTime(), you should use DateTime::createFromFormat('U', time()) to use the mocked time() function.

For reference, see

Copy link
Contributor

Choose a reason for hiding this comment

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

@cseufert

Can you adjust this, please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated this to be more accurate

`time()`, `date()`, `microtime()`, etc functions, however this does not solve mocking calls to
cseufert marked this conversation as resolved.
Show resolved Hide resolved
`new \DateTime()`
cseufert marked this conversation as resolved.
Show resolved Hide resolved

Pros:

* Consistent interface to get the current time
cseufert marked this conversation as resolved.
Show resolved Hide resolved
* Easy to mock the wall clock time for repeatablility.

Cons:

* Extra overhead and developer effort to get the current time, not as simple as
calling `time()` or `date()`.

## 3. Scope

### 3.1 Goals

* Provide a simple and mockable way to read the current time
cseufert marked this conversation as resolved.
Show resolved Hide resolved
cseufert marked this conversation as resolved.
Show resolved Hide resolved
* Allow interoperability between libraries when reading the clock
cseufert marked this conversation as resolved.
Show resolved Hide resolved

### 3.2 Non-Goals

* This PSR does not provide a recommendation on how and when to use the concepts
described in this document, so it is not a coding standard.
cseufert marked this conversation as resolved.
Show resolved Hide resolved
* This PSR does not provide a recommendation on how to handle timezones when
retrieving the current time. This is left up to the implementation.

## 4. Approaches

### 4.1 Chosen Approach

We have decided to formalize the existing practices, used by several other packages
out in the wild. Some of the popular packages providing this functionality are:
`lcobucci/clock`, `kreait/clock`, `ergebnis/clock`, and `mangoweb/clock`. Some providing
interfaces, and some relying on overloading (extending) the Clock class to mock the
current time.
cseufert marked this conversation as resolved.
Show resolved Hide resolved


### 4.2 Example Implementations

```php
final class TimeZoneAwareClock implements \Psr\Clock\ClockInterface
Copy link

@kylekatarnls kylekatarnls Mar 16, 2021

Choose a reason for hiding this comment

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

I definitely feel very uncomfortable with this TimeZoneAwareClock as the first implementation shown as an example.

It feels like the normal case to implement it would be to provide a timezone, while this is a very bad practice to promote. time-zone-based code leads to a code you can't scale easily worldwide, you can't easily move consistently to an other place, not easily re-use.

The Non-Goals says it let the timezones handling up to implementation, but this example is yet very opinionated spreading the idea that picking any timezone is OK.

We should show first the UTCClock then simply show that timezone can be changed on the returned object:

$universalNow = (new UTCClock())->now();
$nowForMyChicagoUser = $universalNow->setTimezone(new DateTimeZone('America/Chicago'));

And we should encourage to use the first one to do any back-end stuff or calculation, while reserving the second kind only for the very output to be displayed to an end user.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kylekatarnls

Why not inject the timezone via constructor?

final class SystemClock implements Clock
{
    private \DateTimeZone $timezone;

    public function __construct(\DateTimeZone $timezone)
    {
        $this->timezone = $timezone;
    }

    public function now(): \DateTimeImmutable
    {
        return new \DateTimeImmutable(
            'now',
            $this->timezone
        );
    }

    public function freeze(): FrozenClock
    {
        return new FrozenClock($this->now());
    }
}

See https://github.com/ergebnis/clock/blob/2.2.0/src/SystemClock.php#L16-L37.

Choose a reason for hiding this comment

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

I don't see the point this is exactly the same as the current proposal for TimeZoneAwareClock and showing this to people is the same as promoting "You can use any timezone for your clock". AND. THAT'S. NOT. GOOD.

As the person who might have to call ->now() on an object implementing ClockInterface I want to get something reliable, not something in unknown, dynamic or even just some hard coded city timezone name.

If someone want a timezone they can do it outside of the clock using ->setTimezone() and as this PSR says it's not aimed to say how to deal with timezone, then it should not show clocks that handle it internally.

Sure users still will be able to do it. But we should encourage them to do it outside the clock. So you give the clock only the responsibility to tick the moment, then give to some other service the responsibility to tell you what numbers are given to this tick in a given timezone.

Copy link
Contributor

@heiglandreas heiglandreas Mar 16, 2021

Choose a reason for hiding this comment

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

You get something very reliable: A DateTimeImmutable Object that represents the current time!

What you will never be able to tell is, whether the current time was observed in Cairo, Sydney, Lima or London. That is something that your own business logic needs to handle anyhow.

The whole ClockInterface will be completely timezone agnostic and we are currently in the process of removing that missleading example!

Copy link
Contributor

Choose a reason for hiding this comment

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

Throwing my opinion in here since we’re still going back and forth on timezones.

+1 to encouraging UTC and leaving timezone manipulation to app implementation. IMHO if someone needs this they can create their own interface, the PSR should enforce best practice whenever possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

This PSR is timezone-agnostic.

Therefore we should not encourage anything in regards to UTC or any other timezone.

BestPractices in timezone-handling is to use DateTimeImmutable which we are enforcing. The internals of that DateTieImmutable-Instance are not our business.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perfect, thanks @heiglandreas

{
private \DateTimeZone $timeZone;

public function __construct(\DateTimeZone $timeZone)
{
$this->timeZone = $timeZone;
}

public function now(): \DateTimeImmutable
{
return new \DateTimeImmutable('now', $this->timeZone);
}
}

//

final class SystemClock implements \Psr\Clock\ClockInterface
{

cseufert marked this conversation as resolved.
Show resolved Hide resolved
public function now(): \DateTimeImmutable
{
return new \DateTimeImmutable();
}
}

//

final class UTCClock implements \Psr\Clock\ClockInterface
{
private TimeZoneAwareClock $inner;

public function __construct()
{
$this->inner = new TimeZoneAwareClock(new \DateTimeZone('UTC'));
}

public function now(): \DateTimeImmutable
{
return $this->inner->now();
}
}

//

final class FrozenClock implements \Psr\Clock\ClockInterface
{
private \DateTimeImmutable $now;

public function __construct(\DateTimeImmutable $now)
{
$this->now = $now;
}

public function now(): \DateTimeImmutable
{
return $this->now;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this not be return clone($this->now);

I know, it's a minor detail, but any call to now() should return a new DateTimeImmutable Object, even in a frozen clock.

Copy link
Contributor

Choose a reason for hiding this comment

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

@heiglandreas

Why?

It's a value object, so why bother returning different instances with the same value?

Copy link

@kylekatarnls kylekatarnls Mar 18, 2021

Choose a reason for hiding this comment

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

In unit test:

$someObject->updatedAt = $clock->now();

Now if you try to compare $someObject old state property values with new ones using ===, the test will assert the object is untouched. That may lead to wrong results.

So I guess the (clone) $this->now can make it a bit safer.

Mainly, the idea is to make the FrozenClock behave as closely as possible like the real one.

Copy link
Contributor

Choose a reason for hiding this comment

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

That was my reasoning. Each call to now() should return a new ValueObject IMO (whether the content is the same is a different story)

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return $this->now;
return clone $this->now;

}
}
```

## 5. People

### 5.1 Editor

* Chris Seufert

### 5.2 Sponsor

* Chuck Burgess

### 5.3 Working group members

* Pol Dellaiera
* Ben Edmunds
* Jérôme Gamez
* Andreas Heigl
* Andreas Möller
* Luís Cobucci
Copy link
Contributor

Choose a reason for hiding this comment

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

@cseufert

What’s the order here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

totally random really, any suggestions?

Choose a reason for hiding this comment

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

Alphabetically would be my first choice

Copy link
Contributor

Choose a reason for hiding this comment

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

Alphabetical by last name. Move Luís Cobucci to the beginning and we're done ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


## 6. Votes

*

## 7. Relevant Links

* https://github.com/ergebnis/clock/blob/main/src/Clock.php
* https://github.com/icecave/chrono/blob/master/src/Clock/ClockInterface.php
* https://github.com/Kdyby/DateTimeProvider/blob/master/src/DateTimeProviderInterface.php
* https://github.com/kreait/clock-php/blob/main/src/Clock.php
* https://github.com/lcobucci/clock/blob/2.1.x/src/Clock.php
* https://github.com/mangoweb-backend/clock/blob/master/src/Clock.php
* https://martinfowler.com/bliki/ClockWrapper.html

## 8. Past contributors

Since this document stems from the work of a lot of people in previous years, we should recognize their effort:
cseufert marked this conversation as resolved.
Show resolved Hide resolved

*
_**Note:** Order descending chronologically._
72 changes: 72 additions & 0 deletions proposed/clock.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
Common Interface for Accessing the Clock
========================================

This document describes a simple interface for reading the system clock.

The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", "SHOULD",
"SHOULD NOT", "RECOMMENDED", "MAY", and "OPTIONAL" in this document are to be
interpreted as described in [RFC 2119][].

The final implementations MAY decorate the objects with more
functionality than the one proposed but they MUST implement the indicated
interfaces/functionality first.

[RFC 2119]: http://tools.ietf.org/html/rfc2119

# 1. Specification

## 1.1 Introduction

Creating a standard way of accessing the clock, would allow interopability
during testing, when testing behavior that has timing based side affects.
Common ways to get the current time include calling `\time()` or
`new DateTimeImmutable('now')` however this makes mocking the current time
cseufert marked this conversation as resolved.
Show resolved Hide resolved
impossible in some situations.

## 1.2 Definitions

* **Clock** - The clock is able to read the current time and date.

* **Timestamp** - The current time as an integer number of seconds since
Jan 1, 1970 00:00:00 UTC.

### 1.3 Usage

There are some common usage patterns, which are outlined below:

**Get the current timestamp**

This should be done by using the `getTimestamp()` method on the returned `\DateTimeImmutable` like so:
```php
$timestamp = $clock->now()->getTimestamp();
```

**Timezone**
cseufert marked this conversation as resolved.
Show resolved Hide resolved

Each implementation of the `ClockInterface` is free to return the time in the
timezone of that library authors choice. This could include but not be limited
cseufert marked this conversation as resolved.
Show resolved Hide resolved
to return the current PHP timezone (as the `DateTimeImmutable` constructor currently
cseufert marked this conversation as resolved.
Show resolved Hide resolved
does), return a timezone set at the creation of the `ClockInterface` implementation
instance, or always returning a fixed timezone (e.g. UTC).

# 2. Interfaces

## 2.1 ClockInterface

The clock interface defines the most basic operations to read the current time and date from the clock.
It MUST return the current time as a DateTimeImmutable.
cseufert marked this conversation as resolved.
Show resolved Hide resolved

~~~php
<?php

namespace Psr\Clock;

interface ClockInterface
cseufert marked this conversation as resolved.
Show resolved Hide resolved
{
/**
* Returns the current time as a DateTimeImmutable Object
*/
public function now(): \DateTimeImmutable;
cseufert marked this conversation as resolved.
Show resolved Hide resolved

}
~~~