date-time icon indicating copy to clipboard operation
date-time copied to clipboard

UtcDateTime

Open solodkiy opened this issue 4 years ago • 20 comments

I have a goal to change all of internal date time operations in my project to UTC dates and now looking for the simplest solution. I think that one of approach is to have class which represent such date concept like UTC date time and use it in all internal code and in db. Convert to LocalDateTime and ZonedDateTime only when it relay necessary.

In this lib we have:

  • ZonedDateTime - real point in time, and can be used, but we always need to think about time zones (is it set correctly to this object?) and also you need always convert it to UTC before saving in DB if you want UTC time there which is not very convenient and can lead to mistakes.
  • LocalDateTime - can represent UTC time, but also can not represent. Looks like it should be used for interactions with other systems (api which expect date in Europe\Moscow timezone) and to render for user.
  • Instant - looks very similar for my goal, but has not very convenient interface.

What do you think about adding new UtcDateTime class? UtcDateTime is represent point absolute point in time (sounds similar to Instant, yeah) And have all methods of LocalDateTime and some more like easy convert to ZonedDateTime(UTC) or Instant. Or maybe we should just add a simple methods to convert ZonedDateTime to and from UTC LocalDateTime? Something like $zoned->getUtcLocal();

What to you think about it?

solodkiy avatar Apr 05 '20 19:04 solodkiy

Hi, what you're calling UtcDateTime really is ZonedDateTime with a UTC timezone; I can see no advantage in introducing another class.

  • LocalDateTime is useful when you need to represent a date, without having to think about the timezone. It cannot represent a point in time without a timezone.
  • Instant is really just an abstraction around a timestamp. It represents a point in time, but cannot represent a date and time without a timezone.
  • ZonedDateTime represents both a date and a time, and a point in time. It can be used with the UTC timezone to do what you want.

You can convert between LocalDateTime and ZonedDateTime easily:

$localDateTime = LocalDateTime::parse('2020-04-01T00:00:00');

$utcDateTime = $localDateTime->atTimeZone(TimeZone::utc()); // ZonedDateTime
$localDateTime = $utcDateTime->getDateTime(); // LocalDateTime

Let me know if that solves it for you!

BenMorel avatar Apr 05 '20 20:04 BenMorel

Closing due to lack of feedback. Feel free to comment if you think this should be reopened.

BenMorel avatar Nov 20 '20 22:11 BenMorel

I got your point about UtcDateTime really is ZonedDateTime with a UTC timezone and made some try about it. It still work in progress but you can check the main idea: UtcDateTime extend ZonedDateTime so it can be used anywhere where could ZonedDateTime. Also you cannot create ZonedDateTime with UTC timezone anymore, so it looks correct in types point. https://github.com/brick/date-time/pull/36/files

Why I still think that we need separate object for UTC? Type checking! There are many places (work with db for example) where I want to be sure that I have strictly UTC time. This type could guarantee it for me.

solodkiy avatar May 30 '21 04:05 solodkiy

Sorry, I commented on your PR before seeing your comment here. While I get your point about type checking, TBH I still don't think it deserves its own subclass. While this project is not a perfect port of Java's date-time, it's still worth noting that they haven't followed this route either.

I'll let this settle a bit before taking a final decision, but I don't think I'll follow you on this one!

Other people may take this time to voice their opinions, too.

BenMorel avatar May 30 '21 21:05 BenMorel

Could we reopen this issue to attract some attention?

solodkiy avatar May 30 '21 22:05 solodkiy

Sure!

BenMorel avatar May 30 '21 22:05 BenMorel

Hello,

We have more or less the same use case. Part of our application must preserve UTC as a timezone to comply with business rules.

We introduced an UTCDateTime decorating the ZonedDateTime and made sure every public methods result in an UTC timezone. And it works for us.

class UTCDateTime
{
    private function __construct(private ZonedDateTime $zonedDateTime)
    {
    }

    public static function of(LocalDateTime $dateTime) : self
    {
        return new self(ZonedDateTime::of($dateTime, TimeZone::utc()));
    }

    public static function fromDateTime(\DateTime|\DateTimeImmutable $dateTime) : self
    {
        // Implicitly convert the datetime object to UTC timezone
        $dateTime->setTimezone(new \DateTimeZone('UTC'));

        return new self(ZonedDateTime::fromDateTime($dateTime));
    }

    public function withTime(LocalTime $time) : self
    {
        return new self($this->zonedDateTime->withTime($time));
    }

    public static function parse(string $text, ?DateTimeParser $parser = null) : self
    {
        return new self(ZonedDateTime::parse($text, $parser)->withTimeZoneSameInstant(Timezone::utc()));
    }

   // and so on...
}

So I guess I'm on @BenMorel side here, makes me wonder why ZonedDateTime is not final too btw.

tigitz avatar Apr 24 '22 19:04 tigitz

The main reasons I use extension against decoration is Liskov Substitution Principle. I use fork of brick/date-time with this class (and some other improvements) in production and for now happy with the result. Extension allows me to pass UtcDateTime in any method that expects ZonedDateTime. Thats include any of original brick/date-time methods, or any methods that wrote programmers unfamiliar with my fork.

solodkiy avatar Apr 24 '22 20:04 solodkiy

It's understandable but like @BenMorel pointed out here: https://github.com/brick/date-time/issues/8#issuecomment-526817214 this is not ideal.

Decorating and providing a toZonedDateTime() would also allow what you describe. Giving you the best of both world.

tigitz avatar Apr 24 '22 21:04 tigitz

Part of our application must preserve UTC as a timezone to comply with business rules.

Why don't you just use Instant? Imho timezone should be used only when it does matter. Instant is the exact same moment, anywhere in the world, just like when UTC is used.

Edit. oh I see, you wrote it in the first post, you don't like the interface. What is wrong about it?

mabar avatar May 08 '22 19:05 mabar

@mabar It could but when you manipulate ISO 8601 formatted date from your database and web context, using Instant would force you into superfluous conversion.

// From POST request
$date = '2022-01-01T13:30:00+02:00'

// storing logic in a datetime column, transform a `ZonedDateTime` to an `Instant` and back to a `ZonedDateTime`
ZonedDateTime::parse($date)->getInstant()->withTimeZone(TimeZone::utc())

// While using an UTCDateTime is clearer. It still is a ZonedDateTime, no manual `Instant` conversion needed.
UTCDateTime::parseAndConvert($date)

Also comparison would be more tedious too

class Delivery
{
    public ZonedDateTime $expected;
    public Instant $done;

    public function isLate(): bool
    {
        return $this->expected->getInstant()->isBefore($this->done);
    }

Instead of

class Delivery
{
    public ZonedDateTime $expected;
    public UTCDateTime $done;

    public function isLate(): bool
    {
        return $this->expected->isBefore($this->done);
    }

tigitz avatar May 09 '22 12:05 tigitz

I'd really like to see this in the library as well. The usecase really is typehinting vs having all classes accept ZonedDateTime and then doing an assertion if it's converted to UTC, or convert it to UTC automatically.

pscheit avatar Jun 09 '22 02:06 pscheit

FYI, we've talked about it with @BenMorel some weeks ago, he got the point but he's still undecided and needs to process the implications.

In the meantime, take a look at my implementation of this in my own fork.

https://github.com/tigitz/date-time/blob/main/src/UTCDateTime.php

It introduces a ZonedDateTimeInterface implemented by both ZonedDateTime and UTCDateTime to achieve elegant code like: ZonedDateTime::now(TimeZone::of('Europe/Paris'))->isBefore(UTCDateTime::now())

tigitz avatar Jun 09 '22 10:06 tigitz

Still think that extending is better option here :)

solodkiy avatar Jun 09 '22 10:06 solodkiy

Perhaps generics could be used instead? Not just UTC, other timezones could be useful for various usecases too.

It seems that support for any timezone has to be fixed by phpstan, but otherwise it works well. https://phpstan.org/r/9a3280ad-7e5c-4ca6-9186-f69b3d161948


And here is the report https://github.com/phpstan/phpstan/issues/7440

mabar avatar Jun 09 '22 13:06 mabar

ZonedDateTime can handles multiple timezone while UTCDateTime only have one and should not care about timezone at all.

However if you rely on the ZonedDateTime api to represent an UTCDateTime you'll basically leak unnecessary timezone handling logic into the UTCDateTime. And create confusion at the api level.

Whether you extends it like @solodkiy example, or type it through generics like @mabar suggested.

For example:

final class UtcDateTime extends ZonedDateTime
{
    public static function now(TimeZone $timeZone = null, ?Clock $clock = null): ZonedDateTime
    {
        if ($timeZone === null) {
            $timeZone = TimeZone::utc();
        }
        if (!$timeZone->isEqualTo(TimeZone::utc())) {
            throw new \InvalidArgumentException('Create UtcDateTime with not UTC timezone is not supported');
        }
        return parent::now($timeZone, $clock);
    }

From a design perspective it makes no sense to have a TimeZone parameter to pass in UtcDateTime::now(), you shouldn't care about passing this value nor have the possibility to, it's supposed to be implicit as the class name infers !

And this problem is multiplied by the amount of method where ZonedDateTime expect a certain TimeZone.

It basically corrupts class api and makes it confusing, just because it's trying to design a narrow UTCDateTime concept on the shoulder of a much broader ZonedDateTime concept.

It's a basic example of why the composition over inheritence principle exists.

If you take a look at my implementation it does everything @solodkiy version can do without suffering from the same design problems.

https://github.com/tigitz/date-time/blob/main/src/UTCDateTime.php#L79

tigitz avatar Jun 09 '22 14:06 tigitz

timezone while UTCDateTime only have one and should not care about timezone at all.

interesting, cause that's why I initially decided to pass "LocalDateTime" (in my Project) everywhere and just assume it would be correctly set in UTC, but now I feel like this isnt "safe" enough

pscheit avatar Jun 09 '22 14:06 pscheit

interesting, cause that's why I initially decided to pass "LocalDateTime" (in my Project) everywhere and just assume it would be correctly set in UTC, but now I feel like this isnt "safe" enough

Indeed, "2pm" LocalDateTime means "it's 2pm whether you're in Canada, Spain or whatever". It helps designing a datetime representation where you're basically saying "In my use case, 2pm in Canada is equivalent to 2pm in Spain. I want to represent 2pm without any timezone attached".

That means that LocalDateTime is not an instant, a point in time, it can be multiple instants, 2pm in Canada, 2pm in Spain etc.

However 2pm UTCDateTime is an instant, it's 2pm in UTC, 4pm in Spain, etc.

tigitz avatar Jun 09 '22 14:06 tigitz

From a design perspective it makes no sense to have a TimeZone parameter to pass in UtcDateTime::now(), you shouldn't care about passing this value nor have the possibility to, it's supposed to be implicit as the class name infers

And this problem is multiplied by the amount of method where ZonedDateTime expect a certain TimeZone.

All of this methods is static factory and in it's a shame that php don't allow to make static methods in inheritor with different signature. I hope in some php version it will be allowed.

solodkiy avatar Jun 09 '22 15:06 solodkiy

It basically corrupts class api and makes it confusing, just because it's trying to design a narrow UTCDateTime concept on the shoulder of a much broader ZonedDateTime concept.

It only break static factory methods. Everything else is fine in my opinion.

solodkiy avatar Jun 09 '22 15:06 solodkiy