Closed natebrunette closed 5 years ago
Hi,
LocalDateTime
represents a different concept from ZonedDateTime
and PHP's DateTimeInterface
: a date and time without a time-zone; so it makes no sense to extend DateTimeImmutable
.
This is precisely what I dislike in PHP's implementation: a single class is used to represent many concepts (local date, local time, local date-time, date-time with a time zone, ...)
As for ZonedDateTime
, extending DateTimeImmutable
would create more problems than it solves, as ZonedDateTime
would suddenly inherit from methods that return a DateTimeImmutable
, which is very confusing, and some methods would conflict, such as getTimeZone()
.
For interoperability, what's necessary is a fromDateTime() and a toDateTime()
method. The former is already implemented, while the latter is not.
I'm leaving this issue open until toDateTime()
is implemented!
Fixed in 052c9d7 and released in version 0.1.6:
New method:
ZonedDateTime::toDateTime()
: converts theZonedDateTime
to a nativeDateTime
object.
Note that I hesitated to add a toDateTimeImmutable()
method as well, but because it's trivial, I think we should leave this to the user:
$dateTime = $zonedDateTime->toDateTime();
$dateTimeImmutable = \DateTimeImmutable::createFromMutable($dateTime);
Why not return DateTimeImmutable
in method toDateTime()
? As DateTime
is broken implementation. It mutable value object. I do not see any point in using DateTime
class for anything.
Good question, I guess the choice I made was purely because of the naming: toDateTime()
would return a DateTime
.
I pretty much never use PHP's DateTime classes myself, so I'm not sure which of DateTime
and DateTimeImmutable
people use most and what they would expect here.
This can be changed in 0.9 if needed, but I'd love to hear other people's opinion here.
The main problem with DateTimeImmutable is absence of easy way to convert it to DateTime (many libs still expect this type).
Note that PHP 7.3 added a DateTime::createFromImmutable()
factory method (not documented yet), but this is not relevant anyway as brick/datetime targets PHP 7.1 at the moment.
@Solodkiy makes a good point here, and it makes sense to keep the DateTime
return type IMO.
Should we add toDateTimeImmutable()
methods then? I still think that it clutters the API with methods that just call DateTimeImmutable::createFromMutable()
.
Let me know what you think.
I still think that it clutters the API with methods that just call DateTimeImmutable::createFromMutable().
Yep, it is definitely hard to read and it is quite common use-case. It makes sense to me to implemented also toDateTimeImmutable()
as it is something cheap to do and makes it easy to cooperated with other PHP code (e.g. template filters, etc.)
I agree, I think that interoperability with native PHP date classes is worth the method.
Consider having LocalDateTime and ZonedDateTime extend PHP's DateTimeImmutable for interoperability with API's using PHP's native date objects.