-
Notifications
You must be signed in to change notification settings - Fork 8k
Feature - DateTime::createFromImmutable() method #1145
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
Feature - DateTime::createFromImmutable() method #1145
Conversation
`DateTime` class to mirror the current `DateTime::createFromMutable()`
new method in the reflected method list
d1b3006 to
1de1b6f
Compare
|
Can one of the admins verify this patch? |
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.
The */ should be on the previous line, at the end.
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.
I was just following the file's current style, which has the ending comment marker on a new line. Examples:
- https://github.com/Rican7/php-src/blob/429b0bb38a32b85e2f56d7669724f1c5aa902baa/ext/date/php_date.c#L2592-L2594
- https://github.com/Rican7/php-src/blob/429b0bb38a32b85e2f56d7669724f1c5aa902baa/ext/date/php_date.c#L3741-L3743
- https://github.com/Rican7/php-src/blob/429b0bb38a32b85e2f56d7669724f1c5aa902baa/ext/date/php_date.c#L4089-L4091
|
Travis is 💚 |
|
Oh wow! Thank you so much! My first official PHP source-code contribution! 😊 |
|
Reverted following ML discussion and Derick's objections here: http://markmail.org/message/3aesaaoqv6ihiw53#query:+page:1+mid:ukwizupev32ld5tg+state:results |
|
@smalyshev @php-pulls Shouldn't this one be reopened? |
|
What has happened to this PR? I guess noone cared enough to make an RFC? :( The arguments in http://markmail.org/message/3aesaaoqv6ihiw53#query:+page:1+mid:ukwizupev32ld5tg+state:results do not make any sense. This method is not intended for emulating immutability in PHP <5.5. So now, instead of clean: DateTime::createFromImmutable($dateTime)one has to write a nonsense like this: DateTime::createFromFormat(\DateTime::ISO8601, $dateTime->format(\DateTime::ISO8601), $dateTime->getTimezone())Is this really what you indirectly suggest, @derickr? |
|
+1 On 26 August 2015 at 18:44, Michael Moravec notifications@github.com
|
|
ping @derickr Your reason is not exactly valid. I want to use DateTimeImmutable everywhere in my application. However there are some libraries then historically require DateTime instead of DateTimeInterface. I need to convert the DateTimeImmutable I work with normally to DateTime when calling them. |
|
Hi, I opened #2484 as a follow-up, because I still think this is a legitimate feature and the objections against did not make much sense. |
Introduction
The DateTimeImmutable class, added in version 5.5, was a very convenient addition to the bundled date/time extension. Unfortunately, when migrating legacy codebases or when using an application that needed to mix immutable/mutable instances, the
DateTimeImmutableclass fell short in convenience by not providing a clear/clean way to build an immutable instance from a mutable instance.This problem was solved in version 5.6 with the addition of a convenient factory method:
DateTimeImmutable::createFromMutable(). Unfortunately, however, this addition created a one-sided convenience where a similar problem has been unresolved: Its still just as difficult to create mutableDateTimeinstances from an immutableDateTimeImmutableinstance.Arguably, creating
DateTimeinstances from the immutable representations is something that an application would actually do more often, as any current (or legacy) applications and libraries are more likely to have function/method API's that requireDateTimeinstances as parameters. While ideally the parameter types of new libraries would beDateTimeInterface(or at leastDateTimeImmutable), that option is only available to libraries and applications that support PHP 5.5 as a minimum version... which is currently not the majority.Without this factory method available to the
DateTimeclass, its quite cumbersome to translate back and forth between the types, and honestly is quite the deterrent to even using theDateTimeImmutableclass because of this. For example, if I was using a new library or application that dealt withDateTimeImmutableobjects, but still had to interact with current/older libraries, I'd need to do work like this quite often:As you can imagine, doing the above for multiple calls around an application/library is quite cumbersome, and often leads to potentially buggy user-land implementations to keep the code "DRY" or even worse: simply deters a library/application author from using the new
DateTimeImmutableobject despite its wonderful advantages.Proposal
This PR proposes to introduce a single new method on the
DateTimeclass with the following signature:The resulting object would contain the same date/time value as the provided immutable parameter, however modifications to the resultant
DateTimeobject would only modify that instance and not the immutable instance that was given as an argument. In other words, this simply copies state. It would be quite similar to a type-casted clone (if that were possible).This proposal was designed to mirror the currently accepted and in-use
DateTimeImmutable::createFromMutable()method. It contains a similar signature and would compliment the aforementioned method well.Backward Incompatible Changes
None. This is a new static method on the concrete
DateTimeclass (not on the Interface), so no extending classes would have to worry about breaking here.Proposed PHP Version(s)
PHP 7
Affected PHP Functionality
To SAPIs
None
To Existing Extensions
None
To Opcache
None