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

UtcDateTime #24

Open
solodkiy opened this issue Apr 5, 2020 · 20 comments
Open

UtcDateTime #24

solodkiy opened this issue Apr 5, 2020 · 20 comments

Comments

@solodkiy
Copy link
Contributor

solodkiy commented Apr 5, 2020

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?

@BenMorel
Copy link
Member

BenMorel commented Apr 5, 2020

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
Copy link
Member

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

@solodkiy
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

Could we reopen this issue to attract some attention?

@BenMorel
Copy link
Member

Sure!

@BenMorel BenMorel reopened this May 30, 2021
@tigitz
Copy link
Contributor

tigitz commented Apr 24, 2022

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
Copy link
Contributor Author

solodkiy commented Apr 24, 2022

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
Copy link
Contributor

tigitz commented Apr 24, 2022

It's understandable but like @BenMorel pointed out here: #8 (comment) this is not ideal.

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

@mabar
Copy link

mabar commented May 8, 2022

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
Copy link
Contributor

tigitz commented May 9, 2022

@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
Copy link

pscheit commented Jun 9, 2022

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
Copy link
Contributor

tigitz commented Jun 9, 2022

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
Copy link
Contributor Author

solodkiy commented Jun 9, 2022

Still think that extending is better option here :)

@mabar
Copy link

mabar commented Jun 9, 2022

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 phpstan/phpstan#7440

@tigitz
Copy link
Contributor

tigitz commented Jun 9, 2022

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
Copy link

pscheit commented Jun 9, 2022

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
Copy link
Contributor

tigitz commented Jun 9, 2022

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
Copy link
Contributor Author

solodkiy commented Jun 9, 2022

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
Copy link
Contributor Author

solodkiy commented Jun 9, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants