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

DateInterval no longer respects start/end class types #18057

Open
jurchiks opened this issue Mar 14, 2025 · 6 comments
Open

DateInterval no longer respects start/end class types #18057

jurchiks opened this issue Mar 14, 2025 · 6 comments

Comments

@jurchiks
Copy link

jurchiks commented Mar 14, 2025

DateInterval iterated types no longer mirror parameter types

<?php
class TestClass extends DateTimeImmutable {}

$interval = new DatePeriod(
    new TestClass(),
    new DateInterval('P1D'),
    new TestClass('+5 days'),
);

var_dump(
    $interval->start, // TestClass
    $interval->end, // TestClass
);

foreach ($interval as $date) {
    var_dump($date); // TestClass in PHP 7.4-, DateTimeImmutable in PHP 8.0+
}

https://3v4l.org/VIQbo#v7.4.33 // TestClass
https://3v4l.org/VIQbo#v8.0.30 // DateTimeImmutable

This used to be consistent in PHP7.4 and below, start, end AND iterated values were all of the same class,
but since PHP 8.0, they are no longer consistent for some reason, and it's weird, because there should be no issue with compatibility having it the way it used to be. It used to be objectively better.

I only noticed it now because I had to update a legacy project from 7.3 to 8.3. Now I will also have to manually wrap any code that iterates over DatePeriod.

PHP Version

PHP 8.0+

Operating System

No response

@liuuniverse
Copy link

It seems that this issue has been fixed in the 8.4 version during the subtle comparison. In fact, when calling this code in PHP versions lower than 8.4, you can avoid this issue by adding the third parameter DatePeriod::INCLUDE_END_DATE
$interval = new DatePeriod( new TestClass(), new DateInterval('P1D'), new TestClass('+5 days'), DatePeriod::INCLUDE_END_DATE );

Image

@jurchiks
Copy link
Author

No, it is clearly not fixed:
https://3v4l.org/VIQbo#v8.4.5 - still returns object(DateTimeImmutable)
And even with DatePeriod::INCLUDE_END_DATE it's still the same:
https://3v4l.org/G2OBR#v8.4.5

@michilehr
Copy link

For me, it appears that DatePeriod is broken since 8.4.1.

Before, the end date is not included by default. Starting from 8.4.1, it is.

See https://3v4l.org/Gud6a

@jurchiks
Copy link
Author

Well that's just great... That change is not in the changelog either...

@liuuniverse
Copy link

got it ,I seem to understand what you mean. The official explanation for this issue can be found in #9401. You can take a look

@jurchiks
Copy link
Author

So basically they broke it permanently, but in a weird way, and with no changelog. How could they not include these changes in the changelog? Makes no sense, they're BC breaks!
I find it very odd that they're saying:

To make that work, DateTimeInterface would have to declare the ::__construct() signature, but we can't do that now.

Both DateTime and DateTimeImmutable have the same constructor signature, they should have thought of that when they added the interface.

Carbon does not implement its own constructor, Chronos (which I use) extends it and calls the parent constructor. I'm sure almost all custom extensions of DateTime/DateTimeImmutable do the same, because screw implementing all that logic yourself. I'm also sure that PHP devs can scan GitHub to check how often do custom extensions override __construct() without calling parent::__construct().

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

No branches or pull requests

5 participants