Add DateTimePlus::getPhpDateTime() for situations that require a DateTimeInterface

I'm working & review the proposed solution from the DrupalConEu at Vienna.

I agree with @joelstein. I just fixed the DocBlock comment which was confusing in the return statement.
The method return a \DateTime and not DateTime only.

I have checked the patch: it applies and the code style is correct.

I would suggest to change the method name to be getPhpDateTime. This is more descriptive for example when used in this combination:

$timestamp_typed_data->getDateTime()->getPhpDateTime();

DateTimeInterface::getDateTime is used in:

• \Drupal\Core\TypedData\Plugin\DataType\DateTimeIso8601::getDateTime
• \Drupal\Core\TypedData\Plugin\DataType\Timestamp::getDateTime

Back to Needs work to discuss (and change).

Completely agree with @sutharsan.

I think getPhpDateTime() & even the previous getDateTimeObject() are both quite confusing, the getPhpDateTime() is quite better but I don't like the idea of an acronym into the Class name, maybe something like :

• getPrimitiveDateTime() would do the job, but the DateTime in PHP is not a primitive so .... ?
• Or a simple toDateTime() ?

MomentJS has faced the same kind of problem and resolve it with a toDate(). http://momentjs.com/docs/#/displaying/as-javascript-date/

I don't know, maybe the getPhpDateTime() is the more self-explaining method.

Edited
Plus, when calling somes methods (E.g modify) of a DrupalDateTime it return a DateTime object, so when used in a Service you better use the hint DateTime instead of DrupalDateTime. It make sens to add a feature to retrieve a DateTime.

Let's get this issue going again, we only need to agree on the name, right?

My vote goes to getPhpDateTime.

Reasoning:
- At first, getDateTimeObject seems to make sense, but that's only because I already know that the protected property is named DateTimePlus::$dateTimeObject. We shouldn't assume a developer has that kind of knowledge. Also, DateTimePlus is already an object that contains date and time info, so getDateTimeObject doesn't make clear how it is different from what you already have.
- Methods that look like toSomeType are usually conversions from some type to another, eg. an object to a string. That is not the case here: we're not converting the object, we're getting a property.
- In fact the most logical name IMHO would be getDateTime because it gets a DateTime object, but as Sutharsan pointed out, that name is (over) used already, and it's too late to rename those getDateTime methods now. The way I see it, getPhpDateTime is most similar, only a little more explicit in which kind of date-and-time object it is returning.

@marcvangend, thanks for your explanation and your vote. With 2 votes +1 and one undecided, I conclude that we will name the method getPhpDateTime.

Needs work to change the name and add a (unit) test.

@sutharsan working on it - submit my patch during the afternoon. (UTC+1).

Here a rerolled patch with a test testGetPhpDateTime & the renamed method as getPhpDateTime.

Thanks for adding the test.
Not use if adding a method needs a change record.

One small remark on the documentation:

+++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.php
@@ -690,4 +690,16 @@ public function setDefaultDateTime() {
+   * Gets the PHP DateTime object this class wraps around.
+   *
+   * Getter to return the protected DateTime object.
+   *

To me, the second line does not add value to the first.

Answering my own question: https://www.drupal.org/core/gates#documentation

Change record - Required if the patch makes API or user interface changes (both BC breaks and new additions) that module or theme developers need to know about, or that will need to be documented outside of the patch.

Thanks @Sutharsan, rerolled & remove unnecessary comment.

By the way, how can I help for the "Needs change record" tag ?

Glad to see progress on this, good work!

@wengerk for the change record, on the right sidebar you will find a "add change notice" link, which will take you to the form to add one for the current issue.
Documentation on how to write one: https://www.drupal.org/contributor-tasks/draft-change-record
Also, looking at other change records helps if you want to see some examples: https://www.drupal.org/list-changes/drupal

@wengerk here the guidelines to draft a change record.
What the Change Record needs to document is the addition of this method to the DateTimePlus class, describing what it does and maybe adding a code example of the usage.
Also when you upload a new patch, adding an interdiff will help reviewers better understanding what your changes are compared to previous patches.

Amazing job here, thanks everyone!

Leaving Needs Work due the required change record, patch looks RTBC to me.

UPDATE: crossposted! :D

haha @gambry++

I wrote the API change record here https://www.drupal.org/node/2936388

PS: It's my first API change record hope I do it the right way.

Brilliant, thank you @wengerk -

• We have a CR
• We have tests
• All previous reviews have been addressed and the patch looks great.

RTBCing.

Adjusting issue title and summary to match actual solution.

Applied the issue summary template and added extra summary data.

@wengerk, thanks for the change record. I've made some small adjustments and added an example for working with a date field.

+++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.php
@@ -690,4 +690,14 @@ public function setDefaultDateTime() {
+   * Gets the PHP DateTime object this class wraps around.

I think this should be 'wrapped by this class' instead of 'this class wraps around'.

+++ b/core/tests/Drupal/Tests/Core/Datetime/DrupalDateTimeTest.php
@@ -212,4 +212,15 @@ public function testChainableNonCallable() {
+  /**
+   * Tests getter PHP DateTime.
+   *

Not sure this needs a comment other than the @covers? But should it use the fully qualified class name here?

I think this should be 'wrapped by this class'

Agree, modified.

Not sure this needs a comment other than the @covers?

Yes, @covers added, title removed.

But should it use the fully qualified class name here?

No clue what you mean. \DateTimeZone? Yes Further I see no FCN.

But should it use the fully qualified class name here?

Changed the comment there to specify that the test covers DateTimePlus, which is where the method actually exists. It's more general.

I also updated the change record to point to 8.6.x, and referenced the original New Datetime API change record.

I think we can RTBC this now.

Good catch. I agree with the FQCN in @covers, since the @coversDefaultClass of the test class is of a different class.

when I the use field's DrupalDateTime object because it doesn't implement DateTimeInterface.

Devil's advocate: Is there a reason we don't implement DateTimeInterface?

No, there isn't! We should use DateTimeInterface indeed.

Re #32, #33:

I'm no expert on dates, and I hope I'm understanding the question correctly, but in any case, here's my analysis:

DateTimeInterface is meant for classes that deal with DrupalDateTime objects, eg the getDateTime and setDateTime methods defined by the interface both return and expect a DrupalDateTime object.
So to me it'd make no sense to have DrupalDateTime itself implement DateTimeInterface.

For context, currently the two classes implement the interface it in core are:

• \Drupal\Core\TypedData\Plugin\DataType\Timestamp
• \Drupal\Core\TypedData\Plugin\DataType\DateTimeIso8601

Hey @Manuel Garcia, the suggestion is not for DateTimePlus to implement DateTimeInterface but for the getPhpDateTime() docblock to be @return \DateTimeInterface, instead of \DateTime.

Oh I see @gambry - well, again DateTimeInterface is for dealing with DrupalDateTime objects, and DateTimePlus has no knowledge of DrupalDateTime, so I don't think we can guarantee that getPhpDateTime() will be returning something implementing DateTimeInterface?

One could argue that the only thing extending DateTimePlus in core is indeed DrupalDateTime, but well, I don’t know. I'm going to get out of the way now, let other chip in here :)

Attached what we are trying to achieve.

getPhpDateTime() return a \DateTime object instance, however the correct way to document this is to tell IDE and developers we are returning an instance of its interface.

again DateTimeInterface is for dealing with DrupalDateTime objects,

\DateTimeInterface is a PHP interface, not a Drupal one.

and DateTimePlus has no knowledge of DrupalDateTime, so I don't think we can guarantee that getPhpDateTime() will be returning something implementing DateTimeInterface?

Yes, we are returning the protected $dateTimeObject, ...which now that I check it's actually documented as \DateTime as well.

@larowlan should we change the dockblock of $dateTimeObject too?
It appears to me we were doing correctly "something wrong" in 30. So we either change the docblock var type in there too, or we revert to patch in #30 and we fix both dockblocks in a follow up.

\DateTimeInterface is a PHP interface, not a Drupal one.

Confused by this comment, https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21TypedData... clearly uses DrupalDateTime for its setter, and its getter is specifying that it returns a DrupalDateTime object?

Hey @Manuel Garcia, I always assumed #32 was referring to \DateTimeInterface , not \Drupal\Core\TypedData\Type\DateTimeInterface.

However after giving a thorough reading #32 does say "implement", so probably referring to DateTimePlus implementing \DateTimeInterface.

I think this is a bigger question, probably for the DateTime maintainer. To fully implement \DateTimeInterface the class only need the __wakeup() method.

I'm so reverting this issue to the state it was at #32, adding the question to the IS tasks and pinging @mpdonadio and @jhedstrom to give their feedback.

Hey @Manuel Garcia, I always assumed #32 was referring to \DateTimeInterface , not \Drupal\Core\TypedData\Type\DateTimeInterface.

/me hides under a rock

..and we may not need component maintainers in here (a chat with Berdir was enough! Kudos++).

From DateTimeInterface PHP doc page:

Trying to implement DateTimeInterface raises a fatal error now. Formerly implementing the interface didn't raise an error, but the behavior was erroneous.

So we can't implement that.

Back to RTBC, current patch is again #30. If we want to change the doblocks from \DateTime to DateTimeInterface we can do it in a follow-up.

So what happens if DTP or DDT (or anything that extends these) overrides a method, and the returned \DateTime object is used directly instead of the overridden method?

I'm not sure what you mean @mpdonadio . The idea of making available the \DateTime object is for being able to use external libraries expecting a \DateTimeInterface object, and those libraries don't know (and don't have to know) anything about DTP, DDT or anything extending these.
And won't use nor be affected by any overridden method.

Said that because we are returning the \DateTime object, any change to that will affect of course the future behaviour of the decorating class the object belongs to, so either DTP, DDT or whatever.
This is a tool for the developer, and they should use it responsibly.

Let's say we need to create DrupalDateTime::getTimeZone() and DrupalDateTime::setTimeZone() for some reason. Something like this would then not behave as expected:

$datetime = $drupal_date_time->getPhpDateTime();
$datetime->setTimeZone(new \DateTimeZone('Europe\Berlin');

I guess another way to say this is that by adding this new method which allows modification, we are breaking the Proxy pattern, and I am wondering if this is really a subtle BC break. Perhaps

/**
 * Gets a clone of the proxied PHP \DateTime object wrapped by this class.
 *
 * @return \DateTimeInterface
 *   A clone of the PHP \DateTime object.
 */
public function getPhpDateTime() {
  return (clone) $this->dateTimeObject;
}

may be better.

Quick review

1. +++ b/core/tests/Drupal/Tests/Core/Datetime/DrupalDateTimeTest.php
   @@ -212,4 +212,13 @@ public function testChainableNonCallable() {
   +    $utc = new \DateTimeZone('UTC');
   

   We should do the test somewhere other than 'UTC'.

2. +++ b/core/tests/Drupal/Tests/Core/Datetime/DrupalDateTimeTest.php
   @@ -212,4 +212,13 @@ public function testChainableNonCallable() {
   +    $this->assertInstanceOf('DateTimeInterface', $date->getPhpDateTime());
   

   Can we add assertions to make sure the timestamps are the same and the time zones are the same?

I guess another way to say this is that by adding this new method which allows modification, we are breaking the Proxy pattern, and I am wondering if this is really a subtle BC break.

Yes. That's what I meant with the developer should use it responsibly. And returning the cloned object ensures things won't break.

However what if some libraries want to actually manipulate the \DateTime instance?
(with all the danger may come with)

Do you think something like this will still acceptable?

/**
 * Gets a clone of the proxied PHP \DateTime object wrapped by this class.
 *
 * @param bool $clone
 *   FALSE to return the original proxied \DateTime object. By default a 
 *   clone will be returned, to avoid proxy pattern breaks.
 *
 * @return \DateTimeInterface
 *   A clone of the PHP \DateTime object, or the original instance if $clone
 *   is FALSE.
 */
public function getPhpDateTime($clone = TRUE) {
  return $clone ? (clone) $this->dateTimeObject : $this->dateTimeObject;
}

I am going to polish #32 based on some of the thoughts above. Should have something tonight.

This addresses my concerns.

The test is on both DateTimePlusTest and DrupalDateTimeTest on purpose. The components are supposed to be usable outside of Drupal, so the test needs to live there, and we have been adding the equivalent tests to DrupalDateTimeTest to prevent regressions.

1. +++ b/core/tests/Drupal/Tests/Component/Datetime/DateTimePlusTest.php
   @@ -891,4 +891,34 @@ public function testChainableNonCallable() {
   +    // Test retrieving a cloned copy of the wrapped \DateTime object, and that
   +    // altering it does not change the DateTimePlus object.
   +    $date = DateTimePlus::createFromFormat('Y-m-d H:i:s', '2017-07-13 22:40:00', $new_york, ['langcode' => 'en']);
   +    $datetime = $date->getPhpDateTime();
   ...
   +    $date->setTimestamp(1400000000)->setTimezone($berlin);
   

   The comment correctly says "Altering it (\DateTime object) does not change the DateTimePlus object." however we are doing the opposite? Changing $date and verifying $datetime hasn't changed?

2. +++ b/core/tests/Drupal/Tests/Component/Datetime/DateTimePlusTest.php
   @@ -891,4 +891,34 @@ public function testChainableNonCallable() {
   +    // Test retrieving a reference to the wrapped \DateTime object, and that
   +    // altering it changes the DateTimePlus object.
   

   Same as above.

Also same thing on DrupalDateTimeTest changes.

With the clone in place that's pretty safe now. Thanks for that @mpdonadio

Nice catch. Renamed the local vars to make this read better, and added a few more assertions to make things more explicit.

Thanks. That looks perfect.

My only question is what's the benefit of having the test for both DateTimePlus and DrupalDateTime?
The method lives in DateTimePlus, so its test is the logic place where you would have it tested. But DrupalDateTime doesn't override the method, nor the proxied \DateTime object so I'm not sure what the benefit is?

Setting this to RTBC anyway as currently DrupalDateTimeTest behaves like this for all other tests, and it's more a personal knowledge question than an issue.

Also updating the CR to reflect #45 changes about the new $clone argument.

Thanks!!!

I prefer to start more conservative. At the moment we only have use-cases that need the clone. And to be honest if you need to update a DrupalDateTime object using the referenced \DateTime then just create a new DrupalDateTime object. We should always return the clone.

Wouldn't it be better to just make DateTimePlus implement the \DateTimeInterface? it's almost API compatible already. Then core should use everywhere \DateTimeInterface instead of DateTimePlus or DrupalDateTime and the code would be perfect (by perfect, I mean flexible and extensible).

See #42 about why we can't implement \DateTimeInterface.

Thanks for answering, I didn't know that! Hmm. I am very, very angry at Drupal 8 for implementing this terrible class which is DateTimePlus, and even angrier to DrupalDateTime (seriously, why Drupal, why... there is no good reason on earth) but now I'm also angry at PHP :)

#52 is addressed.

Oops.

The CR needs updates to reflect changes requested on #52 and addressed on #56 - #58.

Updated the CR.

Thanks a lot @mpdonadio!

Crediting people who reviewed the patch and influenced it.

Committed 8a5dba5 and pushed to 8.6.x. Thanks!