feat(datetime): Introduce DateTime component

[azjezz]

This is the first draft of the upcoming DateTime component, and would act as a base, not a final propsal.

Please let me know your thoughts, what do you like, what do you dislike, what is there to add/remove.

closes #27

NOTE: this PR is lacking tests, and does not pass static analysis.

[azjezz]

@veewee let me know what you think of this inital draft :)

[veewee]

Hello,

What a great addition!
I went through the code and added some initial comments here and there.

Most of the functionalities seem to be covered already, I don't have a wishlist of missing features at this moment. They will surely come once I start using this library though.

My head is pretty full at the moment, so I'll come back later to hopefully give some more thoughts on the component from my end. But I already wanted to share these initial comments.

Great job BTW! :)

[coveralls]

Pull Request Test Coverage Report for Build 9280415415

Details

• 603 of 690 (87.39%) changed or added relevant lines in 28 files are covered.
• 8 unchanged lines in 5 files lost coverage.
• Overall coverage decreased (-1.5%) to 97.244%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/Psl/Async/OptionalIncrementalTimeout.php	7	8	87.5%
src/Psl/IO/streaming.php	0	1	0.0%
src/Psl/DateTime/Internal/high_resolution_time.php	19	21	90.48%
src/Psl/DateTime/TemporalConvenienceMethodsTrait.php	33	35	94.29%
src/Psl/DateTime/Duration.php	157	160	98.13%
src/Psl/DateTime/Internal/format_rfc3339.php	14	17	82.35%
src/Psl/DateTime/Timezone.php	9	13	69.23%
src/Psl/DateTime/DateTime.php	110	123	89.43%
src/Psl/DateTime/Exception/InvalidArgumentException.php	0	13	0.0%
src/Psl/DateTime/DateTimeConvenienceMethodsTrait.php	64	84	76.19%

Files with Coverage Reduction	New Missed Lines	%
src/Psl/IO/streaming.php	1	68.57%
src/Psl/Type/Internal/ArrayKeyType.php	1	88.89%
src/Psl/Type/Internal/NumType.php	1	50.0%
src/Psl/Type/Internal/NonEmptyVecType.php	2	92.86%
src/Psl/Type/Internal/ScalarType.php	3	20.0%

Totals
Change from base Build 8956559137:	-1.5%
Covered Lines:	4975
Relevant Lines:	5116

---

💛 - Coveralls

[azjezz]

I feel like the current API is "good enough" for a first iteration now, though it's just lacking a bit more in testing.

Things I'm unsure about:

• Do we need ZonedTemporalInterface?
• Should TemporalInterface be renamed into UnzonedTemporalInterface?
• Should we split DateTimeInterface into UnzonedDateTimeInterface and ZonedDateTimeInterface? (The same goes for DateTime -> UnzonedDateTime + ZonedDateTime)
• Regarding the FormatPattern enum, are the current patterns sufficient, or should we add or remove some?
• About purity: many methods are not pure because they need to use Timezone::default() and Locale::default(), which are mutation-free but rely on system settings (INI configs) to retrieve the default values ( therefore, not pure ). Should we instead use Timezone::UTC / Locale::English to make those methods pure? Is it worth it?
  • If so, should we adjust Timezone::default() to return Timezone::UTC and introduce Timezone::system() to retrieve the system default?
  • Similarly, should Locale::default() return Locale::English, and should there be a Locale::system() to retrieve the system default?

[azjezz]

additional comment by @veewee on Discord:

I have the feeling that splitting it up into 2 concepts introduces more issues than it solves.
The initial discussion was about making Timezone optional or required.
I don't see the benefit in having them both, cause that ends up in 2 different ways of using the component.

So maybe to summarize:

If we add timezones required, the component will result in less bugs given you need to specify and therefore think about the Timezone constanntly.

Which brings me to adding it optionally: results in an easier to use API by assuming the default. When writing cross Timezone code it relies on UTC (or system) and it accepts a way to override the Timezone.

When creating cross Timezone code, the default Timezone gets repeated frequently throughout the codebase anyways.

Which brings me to the question: what benefit does making required have over optional?

It is very explicit and verbose, which makes it an annoying repeating thing.
So do we want the component to be very explicit or rather implicit?

[veewee]

I was trying out a test-case that failed on leap day in one of our projects.
It seems to result in a fatal error:

$at = \Psl\DateTime\DateTime::parse('2024-02-29 09:00:00', FormatPattern::SqlDateTime)->minusYears(18);
echo $at->toString(DateStyle::Full, locale: \Psl\Locale\Locale::DutchBelgium) . PHP_EOL;

I wanted to test a "is the user eighteen years old today" case on a leap day.

PHP Fatal error:  Uncaught ValueError: -1 is not a valid backing value for enum Psl\DateTime\Month in /psl/src/Psl/DateTime/DateTimeConvenienceMethodsTrait.php:456
Stack trace:
#0 /psl/src/Psl/DateTime/DateTimeConvenienceMethodsTrait.php(456): Psl\DateTime\Month::from(-1)
#1 /psl/src/Psl/DateTime/DateTimeConvenienceMethodsTrait.php(385): Psl\DateTime\DateTime->minusMonths(216)
#2 /psl/test.php(14): Psl\DateTime\DateTime->minusYears(18)
#3 {main}
  thrown in /psl/src/Psl/DateTime/DateTimeConvenienceMethodsTrait.php on line 456

Fatal error: Uncaught ValueError: -1 is not a valid backing value for enum Psl\DateTime\Month in /psl/src/Psl/DateTime/DateTimeConvenienceMethodsTrait.php:456
Stack trace:
#0 /psl/src/Psl/DateTime/DateTimeConvenienceMethodsTrait.php(456): Psl\DateTime\Month::from(-1)
#1 /psl/src/Psl/DateTime/DateTimeConvenienceMethodsTrait.php(385): Psl\DateTime\DateTime->minusMonths(216)
#2 /psl/test.php(14): Psl\DateTime\DateTime->minusYears(18)
#3 {main}
  thrown in /psl/src/Psl/DateTime/DateTimeConvenienceMethodsTrait.php on line 456

Instinctively, I first tried to do : Duration::years(18) but it seems like Duration stops at weeks.
Does it make sense to add more methods to duration (months, years) - or is it considered not possible because of dynamic days per year? If so : Maybe the "total*" methods should be based on a timestamp? Like getTotalSeconds(Timestamp $timestamp); so that it can deal with these dynamic durations?

That way the implementation for plusMonths could be moved to Timestamp making it always work in exactly the same fashion?

[azjezz]

fixed.

[azjezz]

Duration::years(18) but it seems like Duration stops at weeks.
Does it make sense to add more methods to duration (months, years)

Yes, that is not possible due to leap years, and months have different length, e.g we don't know if Duration::month(1) should return 28 days, 29, 30, or 31.

[azjezz]

same for years.

[veewee]

I tried the fix with leap days:

$at = \Psl\DateTime\DateTime::parse('2024-02-29 09:00:00', FormatPattern::SqlDateTime)->minusYears(18);
echo $at->toRfc3339();

This results in:

2006-03-01T09:00:00+00:00

Which is also what PHP is giving you with regular DateTime.
However,
I'dd rather expect it to be 2006-02-28T09:00:00+00:00 - since march did not start yet.

This is how brick/date-time does it:

https://github.com/brick/date-time/blob/4682c4e1c3cd0b39cce6ac2d6c5eee60de27bde2/src/LocalDate.php#L817-L819

Example:

$today = new DateTimeImmutable('2024-02-29');
$nextYear = $today->modify('+1 year');
echo $nextYear->format('Y-m-d') . PHP_EOL; // 2025-03-01

$today = \Brick\DateTime\LocalDate::fromNativeDateTime($today);
$nextYear = $today->plusYears(1);

echo $nextYear->__toString() . PHP_EOL;  // 2025-02-28

(It's based on java's : JSR 310 (Date and Time API))

Java example:

import java.time.*;
import java.time.format.DateTimeFormatter;
import java.time.format.FormatStyle;
import java.time.temporal.ChronoUnit;
import java.time.temporal.TemporalAdjusters;

LocalDate leapDay = LocalDate.of(2024, Month.FEBRUARY, 29);
Period twoYears = Period.ofYears(2);
LocalDate added = leapDay.plus(twoYears);

System.out.println("👋 Hello, " + added);

👋 Hello, 2026-02-28
(can be tested here : https://dev.java/playground/ )

[azjezz]

this is really depends on the library/api, there is no standard telling us to go a day forward, or a day back.

I suppose going back a day makes sense as it leaves you within the same month 🤔

[veewee]

Most bugs I've seen passing by on leap day were pretty much the example above : adding / subtracting years to leap day resulting in an unexpected day in march. I think it might be a better default. Can't really find much information about why this behaviour was implemented though.

[azjezz]

case for 28-02: we are still in february, the last day.
case for 01-03: 28 days have already passed in february, we are in the day after the 28th, since its not a leap year, its 01-03.

[veewee]

It's only an issue if you add a year or more on a leap day though.

So a better question to ask is: what should one expect if you add a year to a leap day, just like how should adding a month on the 31th of January behave

[veewee]

... which brings me to this:

$d = new \DateTimeImmutable('2024-01-31 09:00:00');
echo $d->add(\DateInterval::createFromDateString('1 month'))->format('Y-m-d');

2024-03-02

This is the approach your implementation follows as well:

$at = \Psl\DateTime\DateTime::parse('2024-01-31 09:00:00', FormatPattern::SqlDateTime)->plusMonths(1);
echo $at->toRfc3339();

2024-03-02T09:00:00+00:00%

BUT

Java / brick:

LocalDate leapDay = LocalDate.of(2024, Month.JANUARY, 31);
Period oneMonth = Period.ofMonths(1);
LocalDate added = leapDay.plus(oneMonth);

2024-02-29

To me, it makes sense to take the java path here as well.

[veewee]

I think these should become smarter by extending the Duration and perform the calculations as you do in DateTime. Otherwise this one will suffer from the same issues as you solved in e.g. plusMonths().

[azjezz]

could you explain further? i don't see what issue might happen here? 🤔

[veewee]

I think it comes down to this:

Currently, it is only possible to plus() or minus() a duration on Timestamp, but there is no way for adding years, months, ... since this is not part of Duration.

So currently that logic lives inside DateTime (which uses a Timestamp internally). On top of DateTime, there are added extra methods to add years and months specifically. At that location, the additional logic for calculating days in months / years are added.

So I was thinking : If DateTime uses a Timestamp internally: why not make adding years / months part of timestamp instead so that is possible too. That way, the logic in DateTime could be a proxy to the methods in Timestamp.

Optionally, the Duration could be extended to years / months if the total seconds logic took a Timestamp as parameter to base calculations upon. I also see that's not part of brick-time, so I suppose that is not the best idea there. However it is a bit strange that it's not possible to plus(Duration::years(1) but it is possible to plusYears(1) from a user's perspective.

Example from java in which it is possible to specify a period (duration) of 2 years and add that:

import java.time.*;
import java.time.format.DateTimeFormatter;
import java.time.format.FormatStyle;
import java.time.temporal.ChronoUnit;
import java.time.temporal.TemporalAdjusters;

LocalDate leapDay = LocalDate.of(2024, Month.FEBRUARY, 29);
Period twoYears = Period.ofYears(2);
LocalDate added = leapDay.plus(twoYears);

System.out.println("👋 Hello, " + added);

(can be ran here https://dev.java/playground/)

[azjezz]

we won't be able to do Duration::years(1), the main difference between Period in java and Duration in psl is that Duration could be converted to a total number of seconds ( or milliseconds, nanoseconds .. etc ),. while the Period object in java can't, it is mearly a data object, so if you do +500 days on a two year period, it won't be 3 years and 135 days, but 2 years and 500 days, as it won't be able to convert the days to proper months / years without having a reference timestamp. e.g:

import java.time.*;
import java.time.format.DateTimeFormatter;
import java.time.format.FormatStyle;
import java.time.temporal.ChronoUnit;
import java.time.temporal.TemporalAdjusters;


Period period = Period.ofYears(2).withDays(500);

System.out.println("👋 Period: " + period);

and the reason we want Duration to be convertable to seconds and fractions of seconds is that we want to use it for timeouts and such, Period in java cannot be used for this purpose, unless you have a reference point.

---

as for moving the logic of adding/removing years/months to Timestamp, that is also not possible, the reason being Timestamp could represent a monotomic time, adding / removing years from it makes no sense, as we do not have an actual timestamp reference to decide when a leap year should happen.

[veewee]

Thanks for the explanation. Maybe a few remarks til I stop about this :) :

• Brick's date-time package come shipped with Duration which is pretty much the same as what we got here. On top of that, it has a Period which can be used for date-ranges like years months. There the difference is made between plusPeriod and plusDuration. it might make sense to prepare the classes for that as well?

• About the monotomic timestamps : Does it make sense to have both monotomic and reguler Timestamps in the same class? I can imagine if you are doing performance testing, you don't really want to add days to the results there? Cause this would add days to unix epoch most likely?

[azjezz]

I believe this is enough as is. we could certainly improve it in the future, but i don't see any blockers that prevent us from shipping it for v3.