Good morning internals!
I would like to propose a small addition be made to the DateTime class.
https://github.com/php/php-src/pull/1145
This is my first contribution to PHP's core, beyond documentation edits.
I'm not sure on the formalities that need to be taken for something like
this to be considered as an addition, or if an email like this is even
appropriate, so I'd absolutely welcome any guidance here.
I just saw a possible opportunity for an addition and thought it would be
fun to try and contribute to PHP. :)
I'm not a C programmer by any means, so please forgive me if I've made any
"noob" mistakes here, haha.
Thank you all for your time and consideration!
- Trevor
Hi Trevor,
I had no look to the code as I am no C or internals guru.
I just wanted to add that I would prefer a generic
DateTimeInterface::createFromInstance(DateTimeInterface $copyFrom)
method, so you do not need to distinguish between DateTime and
DateTimeImmutable when creating a new instance. That should have be done
for the DateTimeImmutable, too. Otherwise you have to do the following:
function doSomethingWithNewDate(\DateTimeInterface $date) {
if ($date instanceof \DateTimeImmutable) {
$newDate = \DateTime::createFromImmutable($date);
} else {
$newDate = clone $date;
}
}
But if you want to create a new instance from a date, it should not
matter if the source date is mutable or immutable.
Thanks
Dennis
Am 05.03.2015 um 08:50 schrieb Trevor Suarez:
Good morning internals!
I would like to propose a small addition be made to the DateTime class.
https://github.com/php/php-src/pull/1145
This is my first contribution to PHP's core, beyond documentation edits.
I'm not sure on the formalities that need to be taken for something like
this to be considered as an addition, or if an email like this is even
appropriate, so I'd absolutely welcome any guidance here.I just saw a possible opportunity for an addition and thought it would be
fun to try and contribute to PHP. :)I'm not a C programmer by any means, so please forgive me if I've made any
"noob" mistakes here, haha.Thank you all for your time and consideration!
- Trevor
Good morning internals!
I would like to propose a small addition be made to the DateTime class.
When the factory method was added, we had this same discussion. And the
discussions lead to the conclusion that it does not make sense to have
this "createFromImmutable" factory method on DateTime. Basically with
the same reasons that people should type hint against the Interface
instead. To emulate that in PHP 5.5 and lower, users can just do:
class DateTimeInterface extends DateTime {}
I know, that's a bit of a hack.
I am against this addition, even though the patch looks OK.
cheers,
Derick
Good morning internals!
I would like to propose a small addition be made to the DateTime class.
When the factory method was added, we had this same discussion. And the
discussions lead to the conclusion that it does not make sense to have
this "createFromImmutable" factory method on DateTime. Basically with
the same reasons that people should type hint against the Interface
instead.
If you have a method that needs to modify the DateTime, then you must
choose between DateTime or DateTimeImmutable. (The interface is read
only.) That is, I think it is poor practice to repeat the following
over and over in every method:
function changeTheDate(DateTimeInterface $dt)
{
$immutable = new DateTimeImmutable($dt->format('c'));
$immutable->setTimeZone(...); // etc
return $someCalculation;
}
That function really needs a DateTimeImmutable, so the burden of
converting it ought to be on the caller. It might be able to optimize
the number of conversions, etc.
I almost always use DateTimeImmutables ... fewer side effects, etc.
However, many libraries and code I interface with require a DateTime
for such things. (Yes, perhaps they just incorrectly type hint for a
DateTime, but I cannot help that.) So I need to convert from
DateTimeImmutable to DateTime.
e.g.,
function foo(DateTime $dt) { }
$dt = new DateTimeImmutable;
foo(new DateTime($dt->format('c')); // the method I use
foo(new DateTime($dt)); // doesn't work
foo(DateTime::fromInstance($dt)); // doesn't exist
(It's possible that
DateTimeImmutable::fromInstance($dateTimeImmutable) could just return
the same object.)
So while not a pressing issue, I do think a more straightforward way
of converting would be good because the reality is that you're going
to need to do the conversions at some point if you interface with
other people's code, no matter how hard you try to use good practices.
--
Matthew Leverton