php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login

go to bug id or search bugs for

Bug #80047 DatePeriod doesn't support custom DateTimeImmutable
Submitted: 2020-09-03 12:01 UTC Modified: 2022-12-09 16:22 UTC
Votes:1
Avg. Score:3.0 ± 0.0
Reproduced:1 of 1 (100.0%)
Same Version:1 (100.0%)
Same OS:0 (0.0%)
From: oognic at gmail dot com Assigned: derick (profile)
Status: Closed Package: Date/time related
PHP Version: 7.2.33 OS:
Private report: No CVE-ID: None
View Add Comment Developer Edit
Welcome! If you don't have a Git account, you can't do anything here.
You can add a comment by following this link or if you reported this bug, you can edit this bug over here.
(description)
Block user comment
Status: Assign to:
Package:
Bug Type:
Summary:
From: oognic at gmail dot com
New email:
PHP Version: OS:

 

 [2020-09-03 12:01 UTC] oognic at gmail dot com 
Description:
------------
If we create a custom class that inherits from DateTimeImmutable, this class can be given as parameter to DatePeriod.

However, when iterating or trying to get the start or end dates, DatePeriod will try to return instances of our custom class but without using its constructor, which will produce incoherent objects.

See this PR for more details: https://github.com/thecodingmachine/safe/pull/227/files

Test script:
---------------
<?php

class CustomDateTimeImmutable extends \DateTimeImmutable {

    private $foo;

    public function __construct($time = "now", $timezone = NULL)
    {
        parent::__construct($time, $timezone);
        $this->foo = "foo"; 
    }
    
    public function getFoo()
    {
        return $this->foo;
    }

}

$datePeriod = new \DatePeriod(new CustomDateTimeImmutable('2020-01-01'), new \DateInterval('P1D'), (new CustomDateTimeImmutable('2020-01-03'))->modify('+1 day'));

var_dump($datePeriod->getStartDate()->getFoo());

$strings = [];
foreach ($datePeriod as $date) {
    var_dump($date->getFoo());
}



Expected result:
----------------
"foo"
"foo"
"foo"
"foo"

Actual result:
--------------
NULL
NULL
NULL
NULL

Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2022-06-24 10:22 UTC] derick@php.net
-Status: Open +Status: Closed -Assigned To: +Assigned To: derick
 [2022-06-24 10:22 UTC] derick@php.net 
The fix for this bug has been committed.
If you are still experiencing this bug, try to check out latest source from https://github.com/php/php-src and re-test.
Thank you for the report, and for helping us make PHP better.

The fix that I have committed is to *now* allow inherited objects here.

The DatePeriod iterator needs to construct DateTime/DateTimeImmutable objects which can only done by copying internal data, which can't be recreated by calling the constructor of an inherited class. This data needs to be directly injected into a (new) DateTime/DateTimeImmutable object.
 [2022-06-25 16:53 UTC] git@php.net 
Automatic comment on behalf of derickr
Revision: https://github.com/php/php-src/commit/973c3f6e241227ffc14c3608c774d7636b798cec
Log: Fixed #80047: DatePeriod doesn't warn with custom DateTimeImmutable
 [2022-07-24 18:50 UTC] kylekatarnls at gmail dot com 
Hi Derick, you say "The fix that I have committed is to *now* allow inherited objects here." but it actually does the opposite, which is not addressing the issue but creating an other one.

This change will forbid sub-classes of DateTime/DateTimeImmutable it would be quite tough to handle this breaking change as of PHP 8.2, but if you release in 8.0.22, there is nothing we can do to prevent our libraries doing this not to blow up when users will update PHP (as they won't necessarily update in the same time their libraries, such as Carbon which construct DatePeriod with Carbon instances (sub-classes of DateTime) and so thousands apps that might potentially use this).

From Liskov substitution principle (SOLID) accepting a parameter T should mean acceptance of any class implementing T (if it's an interface) or extending T (if it's a class).

IMHO, fixing as suggested in this ticket (casting to the source class) would be the only correct fix. If it can't be done, I think we should live with this inconsistent output and users would have to re-create the correct object from the raw DateTime or DateTimeImmutable (this is what we do in Carbon), but restricting the input is far worse.
 [2022-07-25 10:47 UTC] derick@php.net 
Hi Kyle,

That *now* was meant to be *not* in my text.

There are a few issues here:

- PHP can't return your original class, as calling userland constructors from internals land constructors is not reliable.
- The DateTime/DateTimeImmutable classes should have been "final", which would have meant Carbon and others should have used composition instead of inheritance.
- Casting to the "source" class isn't really the right solution either, but indeed probably the best one (although not really 100% accurate either, of course) — and it would also probably break BC for 8.0/8.1.

I would therefore suggest that for 8.0/8.1, I back out this change and replace it with a "cast to original source", but leave it in place for PHP 8.2, as it makes it much more clear that the DatePeriod iterator can't deal with inherited classes.
 [2022-07-25 11:38 UTC] cmb@php.net 
> The DateTime/DateTimeImmutable classes should have been "final", […]

I agree.  So maybe we should be consequent, and deprecate
subclassing these classes, and make them final in the future.
Overall, that might still be better than having *some*
restrictions, and sometimes strange behavior.
 [2022-07-28 10:49 UTC] git@php.net 
Automatic comment on behalf of derickr
Revision: https://github.com/php/php-src/commit/001e7dbb044fd77a5f771043541dfaa3d3dc8435
Log: Fixed bug #80047 (DatePeriod doesn't warn with custom DateTimeImmutable)
 [2022-12-09 15:49 UTC] radek0410 at gmail dot com 
Current solution broke our code because in loop we pass current date to another method where property is typed to our class.

Our object only enrich DateTimeImmutable with some methods like: isFirstDayOfAWeek, getStartOfNextWeek, getEndOfSundayThisWeek. Contructor is not overwritten so everything worked well.

Is there any chance to revert this change?
 [2022-12-09 16:22 UTC] cmb@php.net 
> Is there any chance to revert this change?

No.  See the related discussion on Github[1] for the reasons and
some possible workarounds.

[1] <https://github.com/php/php-src/pull/9174#issuecomment-1203744010>ff
 
PHP Copyright © 2001-2023 The PHP Group
All rights reserved. 		Last updated: Wed Feb 01 22:03:43 2023 UTC