Drupal DateTime have no render to json_encode

Thanks for identifying this. Really quick initial look.

1. +++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.php
   @@ -23,7 +23,7 @@
   +class DateTimePlus implements \JsonSerializable{
    
   

   Nit. Needs space before the opening brace.

2. +++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.php
   @@ -288,6 +288,15 @@ public function __construct($time = 'now', $timezone = NULL, $settings = array()
   +   * Renders the json format.
   

   How about "Returns a representation of the object for use in JSON serialization."

3. +++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.php
   @@ -288,6 +288,15 @@ public function __construct($time = 'now', $timezone = NULL, $settings = array()
   +  public function jsonSerialize() {
   +    return $this->render();
   +  }
   

   Not sure this is really the right thing to do, and it won't mirror the way json_encode(\new DateTime()) works, and it loses information about the object (like the langcode).

This will need test coverage when/if we decide if we want to do this. I think we need to do this, but I would prefer core extends the way json_encode(\new DateTime()) works, rather than just using a rendered string.

I think this format should be OK.

Setting to NW because we need tests to prevent a regression.

Here is my Test Patch that i have created.

I have made the test patch for this issue which checks if serializing DateTimePlus returns empty object or not. Here is my code:-

 public function testIsSerilizable(){
    $t = json_decode(json_encode(new DateTimePlus()),true);
    $t = array_filter($t);
    $this->assertSame(empty($t),false);
  }

A simple test for this could be checking weather the json_encode(new DateTimePlus()) is empty or not. Here is the code that i wrote for testing.

  public function testIsSerilizable(){
    $t = json_decode(json_encode(new DateTimePlus()),true);
    $t = array_filter($t);
    $this->assertSame(empty($t),false);
  }

A simple test for this could be checking weather the json_encode(new DateTimePlus()) is empty or not. Here is the code that i wrote for testing.

  public function testIsSerilizable(){
    $t = json_decode(json_encode(new DateTimePlus()),true);
    $t = array_filter($t);
    $this->assertSame(empty($t),false);
  }

Thanks for the test. Setting NR for testbot. Will comment later. Also, make sure you post an interdiff with a new patch.

Thanks for picking this up.

+++ b/core/tests/Drupal/Tests/Component/Datetime/DateTimePlusTest.php
@@ -665,5 +665,12 @@ public function providerTestInvalidDateDiff() {
-
+   /*
+  Check if DateTimePlus is serilizable
+  */
+  public function testIsSerilizable(){
+    $t = json_decode(json_encode(new DateTimePlus()),true);
+    $t = array_filter($t);
+    $this->assertSame(empty($t),false);
+  }
 }

OK, ran this locally and it does fail on its own, and pass with #5, which does demonstrate the problem. However, I am uncomfortable with this approach for commit. If we change how JSON serialization works, then this will still pass. I think we should just do the `json_encode` on known DateTimePlus objects (probably want to use a provider), and check the results.

This also has multiple coding standard problems that need to be addressed (we can't commit any patches that violate them). From a quick look

- the deleted line needs to be there (blank link between methods)
- there needs to be a blank line after the last method in the class, and the closing brace
- there needs to be a space between the method () and the opening brace
- boolean literals need to be TRUE / FALSE (ie, capitalized).
- the method needs to be in proper docblock format

For new patches, let's post the test-only patch along with the fix+test. Just make sure the test-only one is added first, and the full one is added second. This will preserve the issue status, assuming all is OK. (see https://www.drupal.org/contributor-tasks/write-tests)

Once we have the nearly done, we will need a short change record to announce it and document the format.

mpdonadio, Just wanted to know how can i access dateTimeObject, from my test function....

Updated my test function, to check weather after serialization the data is correct. Also i tried to follow coding standards this time.

 public function testIsSerilizable() {
   $a = new DateTimePlus("1970-01-01 00:00:00","UTC",array("langcode"=>"EN"));
   $t = json_decode(json_encode($a),TRUE);
   $t = array_filter($t);
   $this->assertSame(empty($t),FALSE);
   $l = json_decode(json_encode($t),TRUE);
   $this->assertEquals("1970-01-01 00:00:00.000000", $l["datetime"]["date"]);
   $this->assertEquals("UTC", $l["datetime"]["timezone"]);
   $this->assertEquals("langcode","EN");
 }

I have also attached the interdiff

@denutkarsh, this is what I was thinking of. Let's add some more combinations of timezone and langcode to the provider. Looks like core convention is for the langcode value to be lower case. Think we can just do a full patch at this point.

Hit me up on IRC (or anyone there) if you need help.

@mpdonadio Something like this ( i have just added one more set right now), we have to add more sets like this to make this test more function, is that right?

@mpdonadio Can you tell me why the path i posted is showing that it has 2 fails, while yours shows "PHP 5.5 & MySQL 5.5 21,372 pass".

I have added more sets in this new test patch.

There was php fatal error in previous patch, here is the new one..

This doesn't need a reroll. That is just for when patches don't apply. The fails are from known spurious failures. I'll post a review of the latest patch soon.

OK, we only need to test against 8.3.x. This cannot go into 8.2.x per the change policy, since it is a feature request.

1. +++ b/core/tests/Drupal/Tests/Component/Datetime/DateTimePlusTest.php
   @@ -666,4 +666,101 @@ public function providerTestInvalidDateDiff() {
   +      [
   +        DateTimePlus::createFromTimestamp(1482876655),
   +        [
   +          'datetime' => [
   +            'date' => '2016-12-28 09:10:55.000000',
   +            'timezone_type' => 3,
   +            'timezone' => 'Australia/Sydney',
   +          ],
   +          'langcode' => NULL,
   +        ],
   +      ],
   +      [
   

   This section is redundant, can be deleted (it is tested above).

2. +++ b/core/tests/Drupal/Tests/Component/Datetime/DateTimePlusTest.php
   @@ -666,4 +666,101 @@ public function providerTestInvalidDateDiff() {
   +        DateTimePlus::createFromTimestamp(1300000000,'UTC'),
   

   I would prefer to us 1400000000 for all of the timestamps in the tests (here and below).

3. +++ b/core/tests/Drupal/Tests/Component/Datetime/DateTimePlusTest.php
   @@ -666,4 +666,101 @@ public function providerTestInvalidDateDiff() {
   +        DateTimePlus::createFromTimestamp(139000000,'Europe/Berlin',array('langcode' => 'EN')),
   

   Nits, but we need spaces after the comma between parameters. We can use array shorthand [] for the options, and I would prefer to use lowercase 'en' to be consistent with core usage. Actually, let's use a valid langcode other than 'en' to be safe (here and below).

4. +++ b/core/tests/Drupal/Tests/Component/Datetime/DateTimePlusTest.php
   @@ -666,4 +666,101 @@ public function providerTestInvalidDateDiff() {
   +        DateTimePlus::createFromTimestamp(1300000000,'UTC',array('langcode' => 'EN')),
   

   OK, for this test, we need to test the default timezone with a non-default langcode, so pass in NULL. Per #2498619: Unit tests should use a default timezone other that UTC, the expected TZ should be 'Australia/Sydney', just like with the first set returned.

If this fails anywhere else than DateTimePlusTest, hang tight and let me look at it; we have a bunch #2829040: [meta] Known intermittent, random, and environment-specific test failures going on right now.

@mpdonadio, thanks for reviewing the patch.

@mpdonadio, Ok, I have uploaded new improved patch. I have also checked it on simplytest.me and it seems to pass all the tests.

Unrelated random fail b/c #2828559: UpdatePathTestBase tests randomly failing. Will look at it when I can, or someone else can review it.

Sorry to push this back again over formatting, but there is too much to fix on commit.

1. +++ b/core/tests/Drupal/Tests/Component/Datetime/DateTimePlusTest.php
   @@ -666,4 +666,103 @@ public function providerTestInvalidDateDiff() {
   +  function providerTestSerialization() {
   

   I missed this, but this should be explicitly `public function providerTestSerialization`

2. +++ b/core/tests/Drupal/Tests/Component/Datetime/DateTimePlusTest.php
   @@ -666,4 +666,103 @@ public function providerTestInvalidDateDiff() {
   +        DateTimePlus::createFromTimestamp(
   +          1400000000
   +        ),
   

   Looks like this crept in with this patch, but the constructor calls should all be on one line. Here and below. In general, only multi-element arrays can be spread across multiple lines.

If we can fix those, I'll RTBC it.

@mpdonadio I just used multiline for the constructor to avoid 80 cols violations. Anyways here is the new patch

Realized that I have work in this, so will get @jhedstrom to set if RTBC if he thinks it looks good.

This looks really good. Great tests and a very straightforward implementation/fix.

According to #14 we need a change record.

Also the issue summary is missing the reason why we need this. Not all objects have to be json serializable - so what's the advantage to doing this change?

Took a stab at the CR.

@Julfabre, can you update the Issue Summary with a use case that describes why this is needed beyond what Drupal provides for Serialization as part of core? Thanks.

@Julfabre, thanks. I already made a change record for this. I need to discuss the two bullet points with others before we can proceed here. I thought all of the datetime fields already worked with the REST system.

First, it would increase the compatibility of Drupal's special object DateTimePlus with PHP's DateTime object.

This reason alone seems worth making this change for?

Patch needs to be rerolled. I'm inclined to get this in, as from what I can tell, our custom date time class should be serializable.

I'm not clear on why this doesn't cause more issues with existing serialization. One thought is that it's possible computed fields are always unset prior to serialization, so perhaps this doesn't appear when serializing an entity with a datetime field on it...

#49, another hypothesis is that all of our REST stuff uses the ISO string and not the serialized object.

Patch rerolled.

Since DateTimePlus is a component, and can theoretically be used completely independently of Drupal, our REST system, and other things, I think it makes sense to allow it to be serializable since PHP's \DateTime can be serialized.

+++ b/core/tests/Drupal/Tests/Component/Datetime/DateTimePlusTest.php
@@ -713,6 +713,92 @@ public function providerTestInvalidDateDiff() {
   /**
+   * Tests that DateTimePlus objects can be encoded as JSON.
+   */
+  public function testIsSerializable() {
+    // Create a DateTimePlus, and go through a encode/decode round-trip.
+    $date = DateTimePlus::createFromTimestamp(1400000000);
+    $decoded = json_decode(json_encode($date), TRUE);
+
+    // Make sure the decoded value isn't empty.
+    $decoded = array_filter($decoded);
+    $this->assertFalse(empty($decoded));
+  }
+
+  /**
+   * Tests the DateTimePlus JSON encoding format.
+   *
+   * @param \Drupal\Component\Datetime\DateTimePlus $date
+   *   The test date.
+   * @param object $expected
+   *   The expected value when $date undergoes a JSON encode/decode.
+   *
+   * @dataProvider providerTestSerialization
+   */
+  public function testSerialization($date, $expected) {
+    // Perform an encode/decode round trip so we can compare simple objects,
+    // rather than precise strings.
+    $actual = json_decode(json_encode($date), TRUE);
+    $this->assertEquals($expected, $actual);
+  }

I don't get why we need both testIsSerializable() and testSerialization(). testSerialization() is enough - the other test doesn't appear do anything new.

Also we should have a follow-up to fix the docs of \Drupal\Component\Datetime\DateTimePlus::__construct() - the debug option does not appear exist and the only thing that is used in $settings is $settings['langcode'] which all seems quite odd.

There's also an open question about what should happen if it's an object with errors. Ie. what happens if the creation of PHP's \Datetime object fails. I think the errors property should also be serialized.