Type hint for \DateTimeInterface instead of \DateTime

[jameshalsall]

This change allows userland code to pass instances of \DateTimeImmutable as well as \DateTime by using \DateTimeInterface in type hints instead of the more strict \DateTime.

[magento-cicd2]

All committers have signed the CLA.

[schmengler]

Thank you for your contribution! Did you try to actually exchange DateTime with DateTimeImmutable after the change? I'm still reviewing possible problems (Magento might use methods that are only present in DateTime)

[adragus-inviqa]

Magento might use methods that are only present in DateTime

@schmengler - there are no methods on \DateTime which aren't available in \DateTimeImmutable. It's the other way around, actually (the DateTimeImmutable::createFromMutable() method).

Though there might be subtle changes/bugs in M2's behaviour, that would just point out the fact that mutable states create problems.

[orlangur]

Nice! :) Was thinking of such change and wasn't aware there is even PR proposed already.

The only thing missing as to me is static test enforcement, so that \DateTime never ever used in type-hinting and PHPDoc. We already have some legacy tests checking obsolete ZF classes are not used/used only in particular place.

[schmengler]

setTimezone is not part of DateTimeInterface

[jameshalsall]

I think the only thing we can do here for best DX is to internally convert the DateTimeImmutable to DateTime and enforce the return type hint of DateTime always

[schmengler]

there are no methods on \DateTime which aren't available in \DateTimeImmutable. It's the other way around, actually (the DateTimeImmutable::createFromMutable() method).

Yes, but there are two problems:

1. The "setters" in DateTimeImmutable work different than those in DateTime
2. For this reason, they are not part of DateTimeInterface

Type hinting against DateTimeInterface is perfect where only the interface methods are used (see http://php.net/manual/en/class.datetimeinterface.php), like formatting methods.

But you cannot replace DateTime with DateTimeInterface where methods like setTimestamp() are used. It's possible to make these methods compatible with DateTime and DateTimeImmutable with code like

    $date = $date->setTimezone($tz);

but then check explicitly for DateTime and DateTimeImmutable, otherwise the code will break with userland implementations and test dummies.

Can you please review your changes again? I found at least one occurence of setTimezone after a check for DateTimeInterface (see inline comment)

[jameshalsall]

I'll take another look at this and revert the typehints where it would break code

[jameshalsall]

Haven't forgotten about this :)

[Vinai]

Please also add a <preference for="DateTimeInterface" type="DateTime"/>. I think app/etc/di.xml would be the right place.
Using DateTime instead of DateTimeImmutable as the default preference would be better for backward compatibility.
Also, I'm missing tests. Maybe #4489 can serve as an example (even though it's been ages).

[jameshalsall]

This has been updated now.

@schmengler - the test failures are unrelated.

[ishakhsuvarov]

@jameshalsall I believe test failures are related, since the tests in question are green on the develop branch. I've also ran builds in the internal CICD and see the same result.
However, I could not yet reproduce these failures on my local environment, may be the issue is somehow related to the timezone settings, not sure yet. Let us know if you have any ideas

[ishakhsuvarov]

Actually, test failures identical to the build are reproducible if you run the whole suite of unit tests, for instance bin/magento dev:tests:run unit. I will continue investigating the cause of this problem.

[schmengler]

Thanks for chiming in @ishakhsuvarov

[ishakhsuvarov]

@jameshalsall @schmengler
Looks like the issue which affected multiple tests in the suite was located in the \Magento\Framework\Stdlib\Test\Unit\DateTime\TimezoneTest as it was calling the date_default_timezone_set inside the dataProvider, which led to the incorrect rollback of the timezone setting after test execution.

Since DataProvider seems to be executed before setUp I had to slightly shuffle the order of things in the unit test. Currently it does not look as good and logical as it should be, however I've pushed it to make sure builds are passing. We can improve it afterwards.

[jameshalsall]

@ishakhsuvarov thanks for fixing, I'm pretty sure I had addressed that previously but I may have removed it by accident