Fixed bug #80047 (DatePeriod doesn't warn with custom DateTimeImmutable)

[derickr]

@kylekatarnls — Would this be a better solution, return the original base class for the iterator created DateTime/DateTimeImmutable objects? It doesn't suffer from the broken-constructor issue, and neither does it invalidate LSP (as it says nothing about iterator-returned objects).

This would need to be cherry picked into the 8.0.22 and 8.1.9 branches by the release managers (/cc @carusogabriel, @ramsey, @patrickallaert).

[kylekatarnls]

Fine for me 👍 So the warning will appear only after PHP 8.2.0, right?

[derickr]

@kylekatarnls I don't see the point of a warning in 8.2 any more either.

What I will likely do is make an RFC to tidy up inheritance with deprecations for PHP 8.3, with potential breaking changes in 9.0.

[kylekatarnls]

Ah cool, fine for me.

What would be the deprecation in 8.3?

I think having CustomDateTimeImmutable and CustomDateTime instances in the output (depending on start date) would be the more intuitive behavior. (We do this step manually as of now in CarbonPeriod), basically we cast date objects like this:

new CustomDateTime($date->format('Y-m-d H:i:s.u'), $date->getTimezone());

[cmb69]

Makes sense to me. Thank you!

This would need to be cherry picked into the 8.0.22 and 8.1.9 branches by the release managers (/cc @carusogabriel, @ramsey, @patrickallaert).

Why? Is this fix so urgent, that we can't wait four more weeks, so we have it in an RC?

[derickr]

@cmb69 It's because the wrong fix (ie, BC breaking) is in the current RC.

[cmb69]

It's because the wrong fix (ie, BC breaking) is in the current RC.

Ah, in this case the fix should indeed be cherry-picked into the release branches.

[Lloople]

This introduced a bug in our production apps since the linux repository downloaded the version 8.1.9 automatically and this has a breaking change if you typehint the result of the interval

$period = new DatePeriod(new Carbon('2022-09-09'), new DateInterval('P1D'), new Carbon('2022-09-15'));

collect($period)->each(fn(Carbon $date) => $date->dayOfWeek)
//                           👆
//                      This works in PHP 8.1.8 but not in 8.1.9

Should we disable auto-update for small versions (hotfixes)? I thought the smallest number would never introduce breaking changes

[cmb69]

downloaded the version 8.1.9 automatically

That version is not yet released, though. ;)

Anyhow, we do our best not to introduce incompatibilities in revisions, but fixing this bug didn't leave us much choice. Note though, that there never was a guarantee that DatePeriod methods or properties would be of a more specific type than DateTimeInterface.

[kylekatarnls]

@Lloople using:

$period = new CarbonPeriod(new Carbon('2022-09-09'), new DateInterval('P1D'), new Carbon('2022-09-15'));
$period->excludeEndDate();

collect($period)->each(fn(Carbon $date) => $date->dayOfWeek)

instead should help.

[dereuromark]

With this PHP 8.1 seems to switch behavior in the middle in a way that seems to cause regressions.

Existing code:

        $applicationDate = new \Cake\I18n\FrozenTime('2022-07-29');
        $terminationDate = new \Cake\I18n\FrozenTime('2022-08-29');
        $interval = \DateInterval::createFromDateString('1 week');
        $daterange = new \DatePeriod($applicationDate, $interval, $terminationDate);

        $periods = [];
        foreach ($daterange as $date) {
            if ($date == $daterange->start) {
                $startDate = $date;
            } else {
                $startDate = $date->startOfYear();
            }

            if ($date == $daterange->end) {
                $endDate = $date;
            } else {
                $endDate = $date->endOfYear(); // <----- Error happens over here
            }
        }

After 8.1.9 it now fails since $date is now DateTimeImmutable "only" instead of the custom FrozenTime object that contains those methods.

[Error] Call to undefined method DateTimeImmutable::endOfYear()

Sounds like quite the BC breaking behavior for a 8.1 patch series.

[cmb69]

@dereuromark, see https://bugs.php.net/80047 for an explantions why we needed to fix the current behavior, and why we could not do it in a "correct" (and BC preserving) way. I'm aware that this change is bad, but actually nobody should extend DateTime(Immutable) anyway. These classes should have been final.

[kylekatarnls]

Work-around:

        foreach ($daterange as $rawDate) {
            $date = new \Cake\I18n\FrozenTime($rawDate->format('Y-m-d H:i:s.u', $rawDate->getTimezone());

[dereuromark]

Couldnt this be internal behavior of the above object?

[kylekatarnls]

@derickr I would love that too. 😇

It sounds feasible as get_class($period->start) is still Cake\I18n\FrozenTime in the above case, so it would be something like:

public function current(): DateTImeInterface
{
    $date = ...;

    $expectedClass = get_class($this->start);

    if (get_class($date) === $expectedClass) {
        return $date;
    }

    return new $expectedClass($date->format('Y-m-d H:i:s.u', $date->getTimezone());
}

but in C.

[cmb69]

PHP can't return your original class, as calling userland constructors from internals land constructors is not reliable.

The problem is that the signature of the subclass' constructor is not know (well, there is reflection, but still, which argument values should be passed?) To make that work, DateTimeInterface would have to declare the ::__construct() signature, but we can't do that now. There might be a way out via an additional interface which declares ::__construct(), but that would need to be implemented by subclasses, and generally feels overkill.

The DateTime/DateTimeImmutable classes should have been "final", which would have meant Carbon and others should have used composition instead of inheritance.

[mbardelmeijer]

We were bitten by this change as well unfortunately on multiple applications. With Carbon it quite common in Laravel applications, and passing Carbon in and expecting Carbon out would feel normal.

Wouldn't it have been better to include this bug fix from 8.2.0 and up?

https://3v4l.org/tpo2U