FormattedDateDiff::$maxAge never applied to caches

@andrewbelcher your analysis looks spot on. Nice find. Looks like a patch and some tests are needed.

@andrewbelcher let's see what test bot says - your patch looks good. Because even if mine fail it would still need your added coverage.

So it looks like we're missing proper test coverage to toRenderable too. Given that the test only patch in #3 passed.

Nice catch. Let's make sure both new tests fail w/o the fix. This patch is small, and easy to review, but it helps to post an interdiff between different versions.

1. +++ b/core/lib/Drupal/Core/Datetime/FormattedDateDiff.php
   @@ -53,7 +53,7 @@ public function getString() {
      /**
       * @return int
       */
   -  public function getMaxAge() {
   +  public function getCacheMaxAge() {
   

   This should now be an {@inheritdoc}.

2. +++ b/core/tests/Drupal/Tests/Core/Datetime/DateTest.php
   @@ -393,6 +399,22 @@ public function providerTestFormatDiff() {
   +  public function testToRenderable() {
   +    $expected = [
   +      '#markup' => '10 minutes',
   +      '#cache' => [
   +        'max-age' => 60,
   +      ],
   +    ];
   +    $formatted_date_diff = new FormattedDateDiff($expected['#markup'], $expected['#cache']['max-age']);
   +    $this->assertArrayEquals($expected, $formatted_date_diff->toRenderable());
   +  }
   +
   

   Not sure if this is a valid test.

   I think this after this hunk:

   +++ b/core/tests/Drupal/Tests/Core/Datetime/DateTest.php
   @@ -270,7 +271,12 @@ public function testformatDiff($expected, $max_age, $timestamp1, $timestamp2, $o
   +    $build = [];
   +    CacheableMetadata::createFromObject($result)->applyTo($build);
   +    $this->assertEquals($max_age, $build['#cache']['max-age']);
   

   adding

   $build = $result->toRenderable();
   $this->assertEquals($max_age, $build['#cache']['max-age']);
   

   would be more appropriate since it relies on ::formatDiff() building something that can call ::toRenderable() and not building a FormattedDateDiff() object directly in which ::toRenderable() is essentially a pass-though getter. And add the additional @covers and some comments.

   ?

I kinda like the additional test method, so https://www.drupal.org/files/issues/2780549-16-testFormattedDateDiff.tes...

1. diff --git a/.idea/misc.xml b/.idea/misc.xml
   new file mode 100644
   index 0000000..8662aa9
   --- /dev/null
   +++ b/.idea/misc.xml
   @@ -0,0 +1,4 @@
   +<?xml version="1.0" encoding="UTF-8"?>
   +<project version="4">
   +  <component name="ProjectRootManager" version="2" />
   +</project>
   \ No newline at end of file
   diff --git a/core/lib/Drupal/Core/Datetime/FormattedDateDiff.php b/core/lib/Drupal/Core/Datetime/FormattedDateDiff.php
   

   Looks like this got in by accident.

2. +++ b/core/tests/Drupal/Tests/Core/Datetime/DateTest.php
   @@ -251,6 +252,8 @@ public function testFormatTimeDiffSince() {
   +   * @covers \Drupal\Core\Datetime\FormattedDateDiff::__construct::getCacheMaxAge
   +   * @covers \Drupal\Core\Datetime\FormattedDateDiff::__construct::toRenderable
       */
   

   I'm not sure if we normally allow this syntax, though phpunit didn't complain about it. I think we just need the two lines w/o eg,

    * @covers \Drupal\Core\Datetime\FormattedDateDiff::getCacheMaxAge
    * @covers \Drupal\Core\Datetime\FormattedDateDiff::toRenderable
   

3. +++ b/core/tests/Drupal/Tests/Core/Datetime/DateTest.php
   @@ -270,7 +273,23 @@ public function testformatDiff($expected, $max_age, $timestamp1, $timestamp2, $o
   +    $result = $this->dateFormatter->formatDiff($timestamp1, $timestamp2, $options);
   +    $this->assertEquals($expected_object, $result);
   +
   +    // Test applying the cacheability data to an existing build.
   +    $build = [];
   +    CacheableMetadata::createFromObject($result)->applyTo($build);
   +    $this->assertEquals($max_age, $build['#cache']['max-age']);
   +
   +    // Test converting the result to a render array.
   +    $build = $result->toRenderable();
   +    $expected_build = [
   +      '#markup' => $expected,
   +      '#cache' => [
   +        'max-age' => $max_age,
   +      ],
   +    ];
   +    $this->assertArrayEquals($expected_build, $build);
   

   Even though the two approaches are similar, this is the approach that makes the most sense to me for the long run.

With those two small adjustments, this is RTBC.

+++ b/core/tests/Drupal/Tests/Core/Datetime/DateTest.php
@@ -393,6 +394,34 @@ public function providerTestFormatDiff() {
+   * Tests FormattedDateDiff.
+   *
+   * @covers \Drupal\Core\Datetime\FormattedDateDiff
+   */
+  public function testFormattedDataDiff() {
+    $string = '10 minutes';
+    $max_age = 60;
+    $object = new FormattedDateDiff($string, $max_age);
+
+    // Test conversion to a render array.
+    $expected = [
+      '#markup' => $string,
+      '#cache' => [
+        'max-age' => $max_age,
+      ],
+    ];
+    $this->assertArrayEquals($expected, $object->toRenderable());
+
+    // Test retrieving the formatted time difference string.
+    $this->assertEquals($string, $object->getString());
+
+    // Test applying cacheability data to an existing build.
+    $build = [];
+    CacheableMetadata::createFromObject($object)->applyTo($build);
+    $this->assertEquals($max_age, $build['#cache']['max-age']);
+  }

We are starting to get into scope creep on this now; the ::getString() coverage isn't really related to the bug at hand. I preferred the other approach, but am not necessarily opposed adding this coverage in this issue if we adding a new test method here which consolidates the coverage of these methods (a committer may disagree and push back on this).

This part

+++ b/core/tests/Drupal/Tests/Core/Datetime/DateTest.php
@@ -393,6 +394,34 @@ public function providerTestFormatDiff() {
+   * Tests FormattedDateDiff.
+   *
+   * @covers \Drupal\Core\Datetime\FormattedDateDiff
+   */

needs to be updated, though. We don't have @covers for whole classes in core, just individual methods. This needs to be

+   * @covers \Drupal\Core\Datetime\FormattedDateDiff::getCacheMaxAge()
+   * @covers \Drupal\Core\Datetime\FormattedDateDiff::getString()
+   * @covers \Drupal\Core\Datetime\FormattedDateDiff::toRenderable()

If we make that change, and just post one version of a new patch (ie, just update 2780549-23-testFormattedDateDiff.patch and post a test-only version), then I think we are good and I will set RTBC.

Ok, looks good. Thanks.

So theres a question of whether FormatterDateDiff::getMaxAge() is public API. I don't think so. This was just an error of implementation and maintaining a BC layer feels pointless.

Committed and pushed abe6951 to 8.3.x and f465ed3 to 8.2.x. Thanks!

Discussed with @catch we agreed that the getMaxAge() is dead code.

If we want this to go inot 8.1.x then we should reroll leaving getMaxAge() as a bc layer for the tiny chance that someone is using this as a public API.

I actually do think it would be good to make the old, incorrect method a wrapper for the correct one and deprecate it, in all branches, similar to what @andrewbelcher suggests.

Ok, so it sounds like the plan should be

- roll the patch against 8.1.X
- write change record
- apply final patch to 8.1.X
- revert f465ed3, apply new patch to 8.2.X
- revert f465ed3, apply new patch to 8.3.X

?

Made followup to handle this, #2783491: Add BC and Deprecate FormattedDateDiff::getMaxAge(), so the commit log is more better.

Here's a patch the adds the (tested) BC layer.

I decided that the plan to revert was better because if we're going to add a BC layer it should be in the same commit as the fix.

$ ../vendor/bin/phpunit tests/Drupal/Tests/Core/Datetime/DateTest.php
PHPUnit 4.8.11 by Sebastian Bergmann and contributors.

...............................................................  63 / 102 ( 61%)
.......................................

Time: 530 ms, Memory: 8.00Mb

OK (102 tests, 222 assertions)

Assuming this patch actually runs and come up green, the new version is good; the test passes locally for me.

The comment on the deprecated method matches CacheableDependencyInterface::getCacheMaxAge(). The BC shim calls the replacement function instead of just doing the same thing. The shim has test coverage that looks correct. The deprecation docs look good.

Made draft CR.

Committed and pushed 1bd3a04 to 8.3.x and b845102 to 8.2.x and 76b777f to 8.1.x. Thanks!

Since we've added the BC and this issue would result in caching issues on site cherry-picked to 8.1.x too.