Change only Chronos to extend DateTimeImmutable once again #417
Conversation
othercorey
force-pushed
the
extend-datetimeimmutable
branch
8 times, most recently
from
6d236a3
to
d3582ca
Compare
othercorey
force-pushed
the
extend-datetimeimmutable
branch
from
d3582ca
to
8f86c7d
Compare
|
Removed unnecessary Chronos type declaration from formatter methods. |
| @@ -998,7 +970,7 @@ public function format(string $format): string | |||
| */ | |||
| public function getOffset(): int | |||
| { | |||
| return $this->native->getOffset(); | |||
| return parent::getOffset(); | |||
There was a problem hiding this comment.
Is there much point in retaining methods like these if they only call parent?
There was a problem hiding this comment.
Only if we want to ensure the type hints are available. I think most IDE's can figure this class out though.
I was thinking about commenting out the ones we don't need to wrap for types, but not delete them. We never know what PHP 9 will bring.
Up to you. It would make it possible for people to transition without forcing it on them, but we do want this to be official. However, the only feedback we've seen was initially negative (or at least not positive) to not extending DateTimeImmutable. CakePHP 5 deps need to be updated to only support this. |
othercorey
force-pushed
the
extend-datetimeimmutable
branch
from
8f86c7d
to
a767bc9
Compare
othercorey
force-pushed
the
extend-datetimeimmutable
branch
from
a767bc9
to
6db2131
Compare
| public function toNative(): DateTimeImmutable | ||
| { | ||
| return $this->native; | ||
| } |
There was a problem hiding this comment.
This method was REMOVED in a BUGFIX release?! This change would have broken production releases if our automated testing didn't catch it.
There was a problem hiding this comment.
The removal was a mistake, we'll get that fixed.
The original reasons for this change are described here: #410 (comment)