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

New PSR for ClockInterface #1224

merged 58 commits into from
Mar 24, 2021

Conversation

cseufert
Copy link
Contributor

@cseufert cseufert commented Mar 4, 2021

Proposed new PSR for defining a ClockInterface that reads the clock.

@cseufert cseufert requested a review from a team as a code owner March 4, 2021 12:23
Copy link
Contributor

@heiglandreas heiglandreas left a comment

Choose a reason for hiding this comment

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

To ease usage with DateTimes I would rather only have one single method in that interface to retrieve a DateTimeImmutable. From a usability point we should not encourage people to handle timestamps or microtimes as integers as that inevitably leads to ...interesting results and usually is the reason for time-related problems. Whether we are talking about DST, Timezones, Leapseconds or other such things.

That is the reason why I would propose only one method dateTime that returns a DateTimeImmutable-instance. If it is really necessary I could also think about a DateTimeInterface but using DateTimeImmutable should by now be the prefered object.

If someone actually needs a timestamp or a microtime there is always the possibility to use DateTimeImmutable::format('U') or DateTimeImmutable::format('U.u') to get that value.

The Timezone can equally be retrieved using DateTimeImmutable::getTimezone().

That will make the interface much easier to implement as one does not need to implement methods that one does not need in their implementation just because the interface defines them.

@Chi-teck
Copy link

Chi-teck commented Mar 4, 2021

there is always the possibility to use DateTimeImmutable::format('U') or DateTimeImmutable::format('U.u') to get that value

Could it be a performance issue? Anyway, that probably needs benchmarks on DateTimeImmutable::format('U') vs time().

@heiglandreas
Copy link
Contributor

there is always the possibility to use DateTimeImmutable::format('U') or DateTimeImmutable::format('U.u') to get that value

Could it be a performance issue Anyway, that probably needs benchmarks on DateTimeImmutable::format('U') vs timestamp().

Well. If such a performance impact is an issue, then we should probably think about whether abstracting things into Interfaces is such a good idea.

Yes. That for sure can have a performance impact. But depending on the implementation the code that will then be executed on the implementing class might be return $this->dateTime->format('U');.

And standards that the FIG propose should not think about performance impacts but about interoperability and allowing best possible usage under a multitude of possibilities.

Copy link
Contributor

@rafaelbernard rafaelbernard left a comment

Choose a reason for hiding this comment

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

This is such a great proposal, on my point of view. I left some suggestions for typos.

proposed/clock-meta.md Outdated Show resolved Hide resolved
proposed/clock-meta.md Outdated Show resolved Hide resolved
proposed/clock-meta.md Outdated Show resolved Hide resolved
proposed/clock-meta.md Outdated Show resolved Hide resolved
proposed/clock.md Outdated Show resolved Hide resolved
@shadowhand
Copy link
Contributor

I agree with @heiglandreas, a single ClockInterface::getDateTime() would be sufficient and more consistent.

proposed/clock-meta.md Outdated Show resolved Hide resolved
@Riimu
Copy link

Riimu commented Mar 4, 2021

I think this would be a very good idea to standardize. I've recently (read: past few years) been working on large legacy code base and one really common source of flaky tests is time. Mocking time is not something a lot of people intuitively seem to think about and it creates a whole bunch of issues due to assumptions that people make. For example, common incorrect assumption is that $a = time(); $b = time(); $b >= $a.

In my opinion, a standardized time interface, even if just simple getDateTime(): DateTimeImmutable would go far promoting the idea that time should be testable and it would allow library makers to implement common way of reading time, and thus common way of mocking time. Just recently, I had to investigate how to mock time for ramsey/uuid, since I needed a non flaky way to test timestamp based UUID generation.

From implementation point of view, I think a very simple interface would be best. The more methods there are, the more complicated it gets to mock it. If there is just a single method that returns DateTimeImmutable, that would be the easiest way. Additionally, that could encourage the usage of DateTimeImmutable across different code bases.

There are few things, however, that I think need to be clearly defined in the standard interface:

  • What is the expected precision of time. Can consumers of the interface expect that the time returned by the interface is in microsecond precision, for example?
  • What is the timezone of the returned instance? Can consumers expect it to be UTC? Or would it be same as date_default_timezone_get()? Or would it be defined as "The timezone is the timezone expected by the end user"? Or should it there be no expectations about the timezone at all? IMO, defining it clearly as UTC would probably be wise, but that's just a hunch.

Regarding performance, I also think that it shouldn't really be a concern here. An interface cannot guarantee performance of the implementing code. Thus, consumer of the interface should not make assumptions about the performance. And if performance is of great importance, it's likely that there is an use case for hrtime(), which is a separate matter. (Should there be a standard timer interface? Probably not much need for that from interoperability stand point).

TL;DR; Time is definitely an major interoperability concern that should be seriously considered for standardization.

@heiglandreas
Copy link
Contributor

heiglandreas commented Mar 4, 2021

  • What is the expected precision of time. Can consumers of the interface expect that the time returned by the interface is in microsecond precision, for example?

When we return a DateTimeImmutable then in general we can have microsecond precision (as long as no one is using PHP5.3). So the precision is in the end left to the person handling the returned instance

  • What is the timezone of the returned instance? Can consumers expect it to be UTC? Or would it be same as date_default_timezone_get()? Or would it be defined as "The timezone is the timezone expected by the end user"? Or should it there be no expectations about the timezone at all? IMO, defining it clearly as UTC would probably be wise, but that's just a hunch.

Here also we should not encourage people to expect a certain timezone. The DateTimeImmutable instance hasa timezone attached and that can be used for whatever purpose people seem necessary. In real life you will also not necessarily know what timezone an Instance is in so your code needs to be able to handle different timezones. And IIRC DateTimeImmutable-instances of the same unix-timestamp are considered equal no matter the timezone (see https://3v4l.org/Z4gVl).

@cseufert
Copy link
Contributor Author

cseufert commented Mar 4, 2021

If someone actually needs a timestamp or a microtime there is always the possibility to use DateTimeImmutable::format('U') or DateTimeImmutable::format('U.u') to get that value.

The main issue I have with relying ::format('U') rather than a dedicated method that returns an int or float is the lack of strong typing. It relys on typecasting the string back to a number, which relies on the format being exactly correct. I am not sure event tools like Psalm are this smart yet.

cseufert added 3 commits March 5, 2021 09:27
Removed DateTime object as a return type, seems DateTimeImmutable is the way to go.
@cseufert
Copy link
Contributor Author

cseufert commented Mar 4, 2021

To ease usage with DateTimes I would rather only have one single method in that interface to retrieve a DateTimeImmutable. From a usability point we should not encourage people to handle timestamps or as integers as that inevitably leads to ...interesting results and usually is the reason for time-related problems. Whether we are talking about DST, Timezones, Leapseconds or other such things.

I mostly agree with this statement, however, handling HTTP Cache/Cookie/etc Max-Age are built around manipulating timestamps with integer offsets. Code example from GuzzleHttp SetCookie class:

        if (!$this->getExpires() && $this->getMaxAge()) {
            // Calculate the Expires date
            $this->setExpires(\time() + $this->getMaxAge());
        } elseif (null !== ($expires = $this->getExpires()) && !\is_numeric($expires)) {
            $this->setExpires($expires);
        }

Not sure if the simplicity of this can be expressed as easily with DateTimeImmutable.

@jeromegamez
Copy link
Member

(@cseufert invited me to share my opinion, so here I am 😅)

I also agree with previous statements that one method returning a DateTimeImmutable is all the interface should provide - timezone, timestamp, and microtime can be retrieved from the returned instance.

Since a clock always shows what it considers to be the current time (= now), I do think that now() as the method name would be semantically more appropriate than dateTime().

Regarding performance, I 100% agree with @heiglandreas. If an integer is needed, $clock->now()->getTimestamp() is probably a little more performant than (int) $clock->now()->format('U'), but this is indeed not the clock's concern anymore 🙈.

@heiglandreas
Copy link
Contributor

        if (!$this->getExpires() && $this->getMaxAge()) {
            // Calculate the Expires date
            $this->setExpires(\time() + $this->getMaxAge());
        } elseif (null !== ($expires = $this->getExpires()) && !\is_numeric($expires)) {
            $this->setExpires($expires);
        }

Not sure if the simplicity of this can be expressed as easily with DateTimeImmutable.

If this would be handled with a ClockInterface instance the code would need only a minor adaption:

if (!$this->getExpires() && $this->getMaxAge()) {
    // Calculate the Expires date
    $this->setExpires($this->clock->now()->getTimestamp() + $this->getMaxAge());
} elseif (null !== ($expires = $this->getExpires()) && !\is_numeric($expires)) {
    $this->setExpires($expires);
}

So the \time() would be replaced with $this->clock->now()->getTimestamp(). That's all.

(Always under the assumption that the good ideas of @jeromegamez are used to rename the function to now and to use getTimestamp() instead of the format('U');)

@drupol
Copy link
Contributor

drupol commented Mar 5, 2021

Hello, I'm following this thread as well and I like the idea!

I created also a project with a ClockInterface: https://github.com/loophp/nanobench/tree/master/src/Time

If you want to add it to the list, feel free.

Looking forward to see this happening!

proposed/clock-meta.md Outdated Show resolved Hide resolved
proposed/clock-meta.md Outdated Show resolved Hide resolved
proposed/clock-meta.md Outdated Show resolved Hide resolved
proposed/clock-meta.md Outdated Show resolved Hide resolved
proposed/clock-meta.md Outdated Show resolved Hide resolved
proposed/clock.md Outdated Show resolved Hide resolved
proposed/clock.md Outdated Show resolved Hide resolved
proposed/clock.md Outdated Show resolved Hide resolved
proposed/clock.md Outdated Show resolved Hide resolved
proposed/clock.md Outdated Show resolved Hide resolved
@Crell
Copy link
Contributor

Crell commented Mar 6, 2021

I'm inclined to agree with @localheinz above. Since every other possible format can be gotten off of a DateTimeImmutable instance, the smallest possible surface area would be to just have a now() method that returns a DTI.

I am also highly worried about the "current timezone" concept. Timezones are about one third of everything that makes date and time hard. Any such spec needs to be very careful what it does with them, as that's exactly the sort of place that inconsistencies between libraries can creep in that are both nominally spec-compliant.

My recommendation instead would be to allow now() to take a DateTimezone argument, and the DTI returned is in that timezone. If one is not provided, you get UTC. There is no global "current" timezone. There is UTC or what you ask for. That makes it neatly deterministic and predictable. (And, 95% of the time, UTC is what you want because to do otherwise is asking for trouble.)

@iansltx
Copy link
Contributor

iansltx commented Mar 6, 2021

One caveat on the time zone bit is you can't set typehints to "DTI in UTC", so that piece is by convention. Agreed that passing a DateTimeZone would be useful, which would also explicitly solve the I always want this in UTC use case.

@heiglandreas
Copy link
Contributor

As you get a DateTimeImmutable you can always use $clock->now()->setTimezone($expectedTimezone) to get whatever you want. Bus as already said above: comparing DateTimeInterfaces will always be based on UTC (logically, not necessarily technical) so the timezone is completely irrelevant and only adds an unnecessary complexity.
And if one retrieves the timestamp of a DateTime it will - by definition - always be UTC. So no need to do unnecessary Timezone arithmetics.

@heiglandreas
Copy link
Contributor

A timestamp is by definition Seconds since 1st of January 1970, 00:00:00 UTC.

So 1147483647 is definitely before 1147483947. That is, if people used appropriate tooling to get that timestamp in the first place.

And yes, timestamps are missing leap seconds but the algorithm on how to handle leap seconds in Unix-Timestamps is well defined.

For more information about Unix Timestamp check out https://en.m.wikipedia.org/wiki/Unix_time

Therefore using DateatimeImmutable with it's getTimestamp() method is the easiest way to get the unix-timestamp. Therefore the ClockInterface itself is timezone-agnostic

@kylekatarnls
Copy link

kylekatarnls commented Mar 27, 2021

Side note about leap second, it just means the timestamp 1230768000 last 2 seconds. If you use microsecond-precision timestamp (you should), then you can easily represent portion of this double-second to convert to date-time and you'll lost only 1 microsecond during this leap second, for all the rest of the time, timestamp <-> ISO-8601 UTC datetime string is a 1:1 conversion in both way, you don't loose anything, you always know which UTC moment is any timestamp value. And using microsecond-precision for both, you'll be quite safe for any use. Then as said like 10 times in this thread a timestamp is UTC since it's the time elapsed since 1st of January 1970, 00:00:00 UTC if timestamp = 0 give you anything else, it's NOT a timestamp, it's a number of second elapsed from an arbitrary moment you picked so nothing good to use as a standard.

A little advantage of DateTime(Immutable) can have over the float is the rounding. When add/sub/multiply intervals, a simple float can lost few microseconds in the battle due to the 32/64bit storage limitation. Having each unit separated in an object will help loosing less. So DateTimeImmutable stay IMO a correct choice.

But I'm still a bit sad, we just say "timezone-agnostic" as an excuse because we let the point that need the most to be standardized when mocking time unsolved and implicitly allow clock to generate those object with any timezone (and even in this spec a clock that would generate different timezone each time you call now() would be allowed since "timezone-agnostic" joker).

As said before, timezone should be changed outside the clock so the clock output is really standard and unambiguous.

UTCClock is perfect implementation and FrozenClock should also take an UTC datetime so you actually mock time with consistency. I really hope they will be implemented this way and so we'll trust this now() result.

@heiglandreas
Copy link
Contributor

heiglandreas commented Mar 27, 2021

Nope, Nope and Nope. Your Business-Logic has to take care of the Timezone. When you need UTC, convert the DateTimeImmutable explicitly to UTC (via setTimezone()), If you need a timestamp convert it explicitly (using getTimestamp()). That is what your Business-Logic needs to do!

There are valid use-cases for not using UTC (more than a lot of people might think) and the ClockInterface allows that. And you can never, ever trust DateTime input in terms of timezone. Therefore make sure, that your business-logic sanitizes that input. Even if you are using a UTC-Clock. The ClockInterface only asserts that you get a DateTimeImmutable, it does not assert which timezone it is in. As a new DateTimeImmutable() does not assert which timezone it is in. That is the requirement of your Business-Logic.

@Art4
Copy link

Art4 commented Mar 27, 2021

If you want to make sure that nobody assumes a wrong timezone from ->now() and everyone is forced to set the timezone every implementation should be like this TimezoneRouletteClock.

final class TimezoneRouletteClock implements \Psr\Clock\ClockInterface
{
    public function now(): \DateTimeImmutable
    {
        $names = \DateTimeZone::listIdentifiers(\DateTimeZone::ALL);
        shuffle($names);
        return new \DateTimeImmutable(
            'now',
            new \DateTimeZone(array_pop($names))
        );
    }
}

But imo this would be insane and we should define that the DTI must have the UTC Timezone.

@kylekatarnls
Copy link

kylekatarnls commented Mar 27, 2021

Repeating "nope" is quite weak as an argument. And obviously you'll work with timezones in your business-logic. That does not mean your clock should output objects with the final timezone already set. And rather than converting to UTC from any stupid timezone a clock can send you without you actually know it if this clock is in a third-party, it's way more logical to guarantee an UTC from the output point you want to be standard and reliable, and next in your app converting the other way to other timezones, when you're ready to display this date to an actual user you know is in this timezone.

This is even more important in PHP due to many open bug for years around timezones unexpected behavior as soon as you do comparison/calculation/interval handling.

Maintaining Carbon for 6 years let me not very confident about letting users pick wisely the timezone for the clock. What mostly can happen is having unknown timezone (default from php.ini / OS).

And if I receive in a function a ClockInterface parameter and know it can gives me any timezone so I will need every time to sanitize the object, then honestly, I don't think I won't recommend it as a proper standard to use.

@jeromegamez
Copy link
Member

jeromegamez commented Mar 27, 2021

Here's what I signed up for in context of this PSR:

It is about providing time consistently and ameliorating the interoperability between libraries, not about handling time itself.

Imagine the following scenario: you work on a project that uses two libraries that both handle time. Library A uses nesbot/carbon, library B uses brick/date-time, and my project has a service that integrates both of them.

When I'm running the application, there's no problem. I usenew DateTimeImmutable(), Library A uses Carbon::now() and Library B uses Instant::now(), all are now.

Now, when I want to test all of this, I need to make a new DateTimeImmutable(...), Carbon::setTestNow(...) for Library A and hope that Library B allows me to give it Instant::now(new FixedClock(new Instant(...)));

A common clock interface could solve that: I define a clock in my project and pass it to library A and library B, and all use Clock::now(). What now() returns remains in my control, and what the libraries do with now() remains in their control.

Library A could do Carbon::createFromImmutable($clock->now()), library B could do Instant::of($clock->now()->getTimestamp()) - but we all use the same clock for interoperability.

As for the timezones... if my project uses Europe/Berlin, Library A needs to work with GMT, and Library B with UTC, it's not of the concern of the clock anymore:

  • My project: $now = $clock->now()->setTimezone('Europe/Berlin');
  • Lib A: $now = Carbon::createFromImmutable($clock->now()->setTimezone('Etc/GMT');
  • Lib B: $now = Instant::of($clock->now()->getTimestamp())

As many have said before: the timezone is not the concern of the clock. The clock is also not a DateTime library. Its only job is to be an interface to provide interoperability for different libraries/projects/frameworks to access the same kind of now.

@BenMorel
Copy link

I'm all with @kylekatarnls.

Sure, the ClockInterface is not a date-time library, so it should be used by date-time libraries, which can extract only the timestamp if they want, and still force the user to provide their timezone explicitly.

However, be sure that people will also use the ClockInterface directly, and if the implementation of a stupid clock is return new DateTimeImmutable('now'), then you have done nothing but repeat the errors of the past.

Maintaining Carbon for 6 years let me not very confident about letting users pick wisely the timezone for the clock. What mostly can happen is having unknown timezone (default from php.ini / OS).

And if I receive in a function a ClockInterface parameter and know it can gives me any timezone so I will need every time to sanitize the object, then honestly, I don't think I won't recommend it as a proper standard to use.

Same experience for me, maintaining brick/date-time.

Note that if what you want is an output guaranteed to be in UTC timezone, you may as well just return the timestamp as I suggested. It's a 1-1 mapping between both representations. If you're returning a DateTimeImmutable, you can't guarantee anything related to the timezone.

@heiglandreas You may manage to convince me that a Clock should return a time in a given timezone (after all, you have an opportunity to set this "default" timezone in your DI container, which is still better that OS / php.ini / date_default_timezone_set()), but please don't try to convince me that your Clock returning a DateTimeImmutable is timezone-agnostic. It's not, by definition. I can guarantee that people will use the interface directly and rely on the timezone it returns.

@kylekatarnls
Copy link

kylekatarnls commented Mar 28, 2021

To precise my position, I'm 99% in favor of the current proposal. I could be 100% convinced with just one more simple sentence like the one proposed by @Art4:

The DateTimeImmutable must have the UTC Timezone

So if anyone would claim that $clock1->now() > $clock2->now() gives incorrect result when both clocks are in different timezones, then we could refer to the PSR recommandation.

@heiglandreas
Copy link
Contributor

@BenMorel:

I can guarantee that people will use the interface directly and rely on the timezone it returns.

Then people are using the interface wrongly and we need to address that specifically. That is what I mean with "Timezone-agnostic". The interface does not care about the timezone, that is in the domain of the user to make sure the timezone is what they expect it to be.

If that is by use of convention because people use a UTCClock-implementation or use date_default_set_timezone('UTC') or they use an explicit setTimezone() or getTimestamp() call on the DateTimeImmutable is completely up to the user. I recommend the explicit way, but I can understand that other people may have different ideas.

Wherever someone relies on the timezone of a DateTimeImmutable, that is an accident waiting to happen.

@kylekatarnls: But $clock1->now() > $clock2->now() is comparing the DateTimeImmutables based on their UTC value. Irrelevant in which timezone they are. That is exactly the feature!

@Art4
Copy link

Art4 commented Mar 29, 2021

@heiglandreas Our goal is that no one relies on the timezone of the returned DTI. But if we don't define that the returned DTI MUST be in UTC ("timezone agnostic"), then there will be implementations that have a timezone parameter in __construct() and then equip the DTI accordingly. And with that, you have a clock where users will rely on the timezone. Like in the implementation suggestions in meta.md, that were deleted for this reason.

If we define that the DTI must be in UTC, then we make such implementations (rightly) impossible. A user is then forced to set a timezone (different from UTC) in his buisness logic.

It is somewhat like the paradox of tolerance: to keep tolerance you must be intolerant to the intolerant.

@heiglandreas
Copy link
Contributor

heiglandreas commented Mar 29, 2021

@Art4: Timezone-agnostic does not mean UTC. Timezone-agnostic means it is irrelevant, which timezone the DTI is in.

It means the Users code, the business-logic has to make sure - explicitly or implicitly - that the DTI is used as is appropriate for the business-logic.

That can mean to either use getTimestamp() or to setTimezone() on the DTI to be absolutely sure that what you get is what you expect. Alternatively you can decide to go by convention and use a UTCClock that will always return a DTI with a timezone of "UTC". Or use date_default_set_timezone to make sure all new DTIs are created in UTC as well.

People shall never relly on the timezone of a DTI. The timezone always needs to be checked in the business-logic!

@Art4
Copy link

Art4 commented Mar 29, 2021

@Art4: Timezone-agnostic does not mean UTC. Timezone-agnostic means it is irrelevant, which timezone the DTI is in.

It means the Users code, the business-logic has to make sure - explicitly or implicitly - that the DTI is used as is appropriate for the business-logic.

@heiglandreas One could argue that UTC is very much timezone agnostic. Please correct me but there is no place in the world where UTC applies. There are only places whose timezone sometimes correlates with UTC (depending on DST and other politic reasons). So if you care about a timezone you have to use ->setTImezone().

As you said:

People shall never relly on the timezone of a DTI. The timezone always needs to be checked in the business-logic!

I'm totally with you. But in the sentence before you said:

Alternatively you can decide to go by convention and use a UTCClock that will always return a DTI with a timezone of "UTC". Or usedate_default_set_timezone to make sure all new DTIs are created in UTC as well.

A convention could also be "always return timezone x/y". Imo allowing any clock that is not a UTCClock is the opposite of "timezone always needs to be checked in the business-logic".

@heiglandreas
Copy link
Contributor

In terms of timezones UTC is a timezone as any other. We made it "special" by convention. But all other timezones are equaly valid as they allow to transpose from one timezone to another one. The fact that we are so obsessed with UTC is that a lot if people fail to understand that any DTI created "now" will describe the same point in time, but not on earth. But depending on the use-case that inormation might be relevant but will get lost when we transpose it to UTC.

That's why it should be part of the domain logic to make sure that your DTI has the - for your business-case - correct timezone. Which is nothing that the ClockInterface can determine per se.

@bcremer
Copy link

bcremer commented Mar 29, 2021

I'm not sure if I'm doing anything wrong here.
But in the following example code the outcome can not be fixed with a call to setTimezone() but is dependend on the initial TZ:

<?php
$tz = new DateTimeZone('Europe/Berlin');
$utcTZ = new DateTimeZone('UTC');

// clock yields localtime
$startDate = new DateTimeImmutable('2021-03-28 02:30:00', $tz);
// DST transition
$endDate = new DateTimeImmutable('2021-03-28 03:00:00', $tz);

var_dump($endDate->getTimestamp() - $startDate->getTimestamp()); // int(-1800)
var_dump($endDate->setTimezone($utcTZ)->getTimestamp() - $startDate->setTimezone($utcTZ)->getTimestamp()); // int(-1800)

// clock yields UTC
$startDate = new DateTimeImmutable('2021-03-28 02:30:00', $utcTZ);
$endDate = new DateTimeImmutable('2021-03-28 03:00:00', $utcTZ);
var_dump($endDate->getTimestamp() - $startDate->getTimestamp()); // int(1800)

https://3v4l.org/3BJLP

I know that a monotonic timer like hrtime would be the appropriate solution.

Edit:
Actually that should never happen with new DateTimeImmutable('now') as 2021-03-28 02:30:00 does not exists in Europe/Berlin.

@Art4
Copy link

Art4 commented Mar 29, 2021

Which is nothing that the ClockInterface can determine per se.

@heiglandreas Sure but there are a lot of PSRs that define behavior that could not determine per se like throwing Exceptions. But if an implementation violates the PSR this will be a bug (or you can call it "it is not following the PSR")

Maybe we should open a PR with proposing The DateTimeImmutable MUST have the UTC Timezone. and summaries all pros and cons from this discussion.

Btw: I've personally learned a lot from reading this discussion and would would like to thank everyone participating in this topic. ❤️ I changed my mind a few times (e.g. from "return DateTimeInterface" to "return DateTimeImmutable" and from "keep it totally timezone agnostic" to "force to UTC") and I'm open to change my mind another time for good reasons. Whatever the end result will be I'm convinced that it will be a good solution.

@heiglandreas
Copy link
Contributor

@bcremer this is an oddity that can happen when you create a DTI with a predefined DateTime. That case will never happen when you are using new DateTimmeImmutable() as the DTI will then be created with the correct current local datetime. And on this Sunday in europe/Berlin that would have meant that immediately after 2:00 o'clock you would have gotten 3:00 o'clock. That might become an issue for your test cases though, but in such cases I would go from a UTC date to Europe/Berlin and so get the right information.

Beware that this is still a bigger issue currently on the end of DST. But again: only if you arecreating the DTI with a dateTime, not when initializing it with "now".

@Jean85
Copy link
Member

Jean85 commented Mar 29, 2021

Thank you all for this interesting (and civil!) debate. I would just like to add a single thought.

Up until now it seems that @heiglandreas is debating his "TZ agnostic" point of view against two of the lib maintainers. This highlights that IMHO both sides are right, but Andreas is right on the PSR.

Basically, what I mean is that the PSR should embody the "TZ agnostic" stance, and each maintainer could then go ahead and force their libraries to return the DTI with the TZ that they like, and be opinionated in their on way. This is the beauty of PSRs and such approach: there's a common interface, but there's enough leeway for implementors to move in their preferred way.

Do you agree?

@BenMorel
Copy link

@Jean85 Whatever the outcome of the PSR, one can indeed write an adapter from the PSR's ClockInterface to the each lib's own Clock. But as it stands, I wouldn't be shipping a new version of brick/date-time that embraces the current interface as its main Clock interface I'm afraid, even though I'd be seduced by the idea of doing so.

To be fair, anything returning one of PHP's current date-time objects is out of the question for me, they're too big of a footgun to be used as is IMO.

Regarding the subject at hand, I'm starting to think that I misunderstood the debate. I thought we were debating whether a Clock's contract was to return a point in time + a timezone (allowing it to compute a local date + local time, just like your wall clock). I was even ready to take arguments in this direction, as even though I try to fight any kind of global state, your DI container is usually under your control so if a "default" timezone should come from somewhere, that would be the place.

But as I understand, everyone seems to agree with my initial view that what's important is not returning a local date-time (and therefore a timezone), but returning a point in time. Even @heiglandreas agrees that the timezone coming from DateTimeImmutable is "an accident waiting to happen".

So, please tell me, if you agree that we cannot rely on the timezone returned: why on earth ​do you want to return a DateTimeImmutable, and not just a timestamp? Honest question.

DateTimeImmutable - TimeZone = Timestamp

Or did I miss something?

@drealecs
Copy link
Contributor

I followed this topic and I think both version (with and without timezone) should be implemented as interfaces. There are applications that are timezone aware and there are other that are don't need to be.

Naming is not my strong point but I think we can have something like:

  • timezone aware: ClockInterface, returning DateTimeImmutable/DateTimeInterface
  • no timezone: CurrentTimeInterface, returning MomentInterface.
    A service class implementing ClockInterface should be timezone specific and not require a timezone value to the method used to obtain a DateTimeImmutable.
    There could be 2 method to move from the two value objects types, one requiring a timezone as a parameter, adding it as an information, the other basically dropping the timezone information.

The timezone not aware version could be designed in a separate PR but I think we can benefit the traction of the current discussion and fit it all in.

@heiglandreas
Copy link
Contributor

@BenMorel IIRC I said something along the lines of "Taking any DTI and not making sure that the timezone aligns with my BusinessLogic is an accident waiting to happen"

As I already said, a point in time is a snapshot of the earths rotation or a defined number of seconds based on an atomic clock. Whether that point in time was taken in Boston, Bratislava or Bejing is irrelevant. They are all the same point in time.

@shadowhand
Copy link
Contributor

shadowhand commented Mar 31, 2021

I haven't chimed in on this discussion, but there seems to be a lot of FUD going around about timezones. In order to do any kind of reliable date calculation, a timezone is a requirement. Whatever anyone's beef with DateTimeImmutable is, and yes I read all of the arguments, it is the right thing to return from ClockInterface, because:

  1. It is not just a number.
  2. Whether or not it is in UTC is irrelevant, because the timezone can be read/changed as necessary.
  3. Any kind of non-trivial date manipulation will need a DateTime object, and DateTimeImmutable is superior to DateTime.

My opinion is that the timezone not be declared by this PSR and be left to implementation, and that there only be one interface in this PSR:

public function now(): DateTimeImmutable;

@alanbem
Copy link

alanbem commented Apr 29, 2021

You should definitely look at https://github.com/aeon-php

/cc @norberttech

@kylekatarnls
Copy link

Hello, I'm interested implementing this interface in Carbon, but I would actually need it to be released as a psr/clock composer package. When is it supposed to happen?

@Jean85
Copy link
Member

Jean85 commented May 31, 2021

@kylekatarnls the working group is active on our Discord in the #psr-20 channel: https:://discord.gg/php-fig

Feel free to join and ask directly! IIRC they still have to create the package but I didn't see many more discussion happening, so the only other step forward would be an approval vote, once the code is ready and tested.

@remorhaz
Copy link

So how is this PSR going, what are the next steps?

@heiglandreas
Copy link
Contributor

According to the workflow the editor (@cseufert) and the sponsor needs to agree whether the PSR is ready for a Readiness Vote within the working group. Should that pass the Review-Phase starts which takes at least 4 weeks before an Acceptance Vote can take place.

But before all that can happen, the MR #1230 needs to be merged ;-)

@drupol
Copy link
Contributor

drupol commented Sep 6, 2021

Just for the record, I had to use a ClockInterface in a new package here: https://github.com/Champs-Libres/wopi-lib

@mnavarrocarter
Copy link

I see @BenMorel last made point. I would say that by returning a DateTimeImmutable we are coupling to an object of the PHP stdlib that we do not have control of, and this object could change in the future.

On the other side, there is huge value in terms of ease of use on returning a value object. This object is widely used by many people (even though I would argue that most projects use Carbon, Chronos, Brick or other flavour).

So, here are my two cents: we can have two interfaces. This is perfectly possible. If anyone sees a problem with using two interfaces, happy to discuss.

interface ClockInterface
{
     public function now(): \DateTimeImmutable
}

interface UnixClockInterface
{
    public function unixNow(): int
}

Implementors can implement both or just one. Developers type-hint to whatever they need. If they need DateTimeImmutable they will use the first one. If they just need a unix timestamp, they will use the second one.

Let's empower people so they can make their own choices and not to fall into a false dichotomy when it is perfectly possible to do both.

@mnavarrocarter
Copy link

Also, a RuntimeException should be documented because the underlying syscall to get the time could fail. This would be the case in some custom low level implementation.

@heiglandreas
Copy link
Contributor

As one can call $clockImplementation->now()->getTimestamp() there is no need to create a separate interface for getting a timestamp. Espechially as most of the already existing libraries are also using a DateTimeImmutable or an object derived from it for their (currently proprietary) Clock-Implementations.

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.