-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Implemented request #71520 (Adding the DateTime constants to the DateTimeInterface interface) #2483
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
Conversation
Note that this change is technically BC breaking, because constants defined in interfaces cannot be overridden. I don't think that's a blocker for landing in master though. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good, but I'd split up the test cases.
ext/date/tests/date_constants.phpt
Outdated
|
||
print "\n"; | ||
|
||
var_dump( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of adding to this test case, I think the additions should go into a new test case (or test cases).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made a separate test for each of DateTime/DateTimeImmutable/DateTimeInterface.
It does result in a fatal error thought, not sure how common it is that people define these constants themselves in their extending classes. The problem is, that any library that does will have a hard time working around this. |
…TimeInterface interface)
Updated.
Interesting, because the following code works just fine... <?php
class Foo extends DateTimeImmutable
{
const W3C = 'Y';
}
echo DateTimeInterface::W3C, PHP_EOL; // Y-m-d\TH:i:sP
echo Foo::W3C, PHP_EOL; // Y |
@Majkl578 Heh, https://3v4l.org/V7DiV vs https://3v4l.org/JZBZg. Looks like the check only kicks in if you change it directly in the class that implements the interface, but not if there's at least one class in between. So looks like this isn't a BC break after all. |
Merged as 637714c into master. Thanks! |
@Majkl578 not sure why that is the case, probably because an interface that is defined internally behaves differently than an interface in userland: https://3v4l.org/bNRQu This is of course bad, very bad in this case, but would change the game for this PR in terms of being a breaking change. |
@Fleshgrinder Actually, this seems to be a case pointed out by @nikic - it's only checked for direct descendants, even in user-land. See https://3v4l.org/m5Pfp. |
Now that’s weird behavior. 😅 Well, in that case this PR will be fine for everybody because it is using other magic to prevent implementation in userland. |
This change doesn't seem to be reflected in the documentation yet. See http://php.net/manual/en/class.datetimeinterface.php vs http://php.net/manual/en/class.datetime.php — please fix that. |
Documented via http://svn.php.net/viewvc?view=revision&revision=345663. |
Cf <php/php-src#2483>. We keep the old IDs, though, for BC reasons. git-svn-id: https://svn.php.net/repository/phpdoc/en/trunk@345663 c90b9560-bf6c-de11-be94-00142212c4b1
Cf <php/php-src#2483>. We keep the old IDs, though, for BC reasons. git-svn-id: http://svn.php.net/repository/phpdoc/en@345663 c90b9560-bf6c-de11-be94-00142212c4b1
Cf <php/php-src#2483>. We keep the old IDs, though, for BC reasons. git-svn-id: https://svn.php.net/repository/phpdoc/en/trunk@345663 c90b9560-bf6c-de11-be94-00142212c4b1
Cf <php/php-src#2483>. We keep the old IDs, though, for BC reasons. git-svn-id: https://svn.php.net/repository/phpdoc/en/trunk@345663 c90b9560-bf6c-de11-be94-00142212c4b1
Implements FR #71520.
The primary motivation should be obvious from the feature request. Currently it's not possible to properly use DateTimeImmutable formatting, because one needs to use DateTime anyway, since the constants are defined only on DateTime class.