brick / date-time

Date and time library for PHP
MIT License
335 stars 30 forks source link

UtcDateTime #24

Open solodkiy opened 4 years ago

solodkiy commented 4 years ago

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:

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?

BenMorel commented 4 years ago

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

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 commented 3 years ago

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

solodkiy commented 3 years ago

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.

BenMorel commented 3 years ago

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.

solodkiy commented 3 years ago

Could we reopen this issue to attract some attention?

BenMorel commented 3 years ago

Sure!

tigitz commented 2 years ago

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.

solodkiy commented 2 years ago

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.

tigitz commented 2 years ago

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.

mabar commented 2 years ago

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?

tigitz commented 2 years ago

@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);
    }
pscheit commented 2 years ago

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.

tigitz commented 2 years ago

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())

solodkiy commented 2 years ago

Still think that extending is better option here :)

mabar commented 2 years ago

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

tigitz commented 2 years ago

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

pscheit commented 2 years ago

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

tigitz commented 2 years ago

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.

solodkiy commented 2 years ago

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 commented 2 years ago

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.