Time Ago summary does not render on Manage Display for Timestamp and Datetime fields

Run TestBot, run!

#4, core usage of this is consistent. That would be an out of scope change, though, of this issue.

This pains me to write, but we can't introduce usage of a global function in this patch. We need to inject the renderer service into DateTimeTimeAgoFormatter, and then use that instead of render() directly. I think a test for this is bug overkill, but we may get pushback on it.

Double checking that we just need to issue a CR for this in order to get into 8.1, but not may people in IRC right now.

I'll update the IS from the template, when I get time later, and we need a short beta eval.

Pretty sure this was caused by #2500525: Time ago/hence date/time formatting breaks caching; needs appropriate max age.

I think this is good to go. Once I update the IS and head from a framework manager on the proper steps to get this into 8.1 (b/c the container rebuild), I'll set RTBC.

I'm wondering whether this means we have an actual API break in \Drupal\Core\Datetime\DateFormatterInterface::format itself ... Other code could have used it in a similar way.

#9, yeah looking closer at #2500525: Time ago/hence date/time formatting breaks caching; needs appropriate max age, it looks like we should have written up a CR about it. I'll do that tomorrow (won't have time tonight).

OK, talked to @alexpott.

We need a short CR to announce the change to DateTimeTimeAgoFormatter, that we now inject the renderer service and that the patch requires a container rebuild as a result.

We need to add an empty hook_update_N w/ a comment mentioning the the change to DateTimeTimeAgoFormatter, that we now inject the renderer service, so the change requires a container rebuild (we should do a quick search through core/ for similar language). When the update process looks for hooks, it will find this, and at the end automagically do a drupal_flush_all_caches(), which contains a container invalidation.

... with an update hook we don't need to add a change record.

Looks like #2728811: DateTimeTimeAgoFormatter::settingsSummary() produces htmlspecialchars() warning in field UI is a duplicate of this, but it begs the question why we don't have a test failure. A problem like that should have been caught?

What about this? Still needs a test to show the bug.

And curious if the test fail is as simple as this.

Moving back to Needs Review, the fail in #23 is a known random fail in the branch #2724871: Random failure in \Drupal\migrate_drupal_ui\Tests\d7\MigrateUpgrade7Test.

Going to ping @catch or @alexpott whether they want a test to demonstrate this bug. I said earlier that we didn't need one, but I am on the fence now.

I don't think this really requires a test - we should have tests for all the constituent parts, and the bug itself is minor.

Just closed another issue as a duo of this. Which approach do we want to go with? #22 or #23? I think both are review-ready.

I think patch #23 is the way to go.

Applying patch #23 did nothing about the problem on my install.

Warning: htmlspecialchars() expects parameter 1 to be string, array given in Drupal\Component\Utility\Html::escape() (line 402 of core/lib/Drupal/Component/Utility/Html.php).
Drupal\Component\Utility\Html::escape(Array) (Line: 252)
Drupal\Component\Render\FormattableMarkup::placeholderEscape(Array) (Line: 224)
Drupal\Component\Render\FormattableMarkup::placeholderFormat('Future date: %display', Array) (Line: 204)
Drupal\Core\StringTranslation\TranslatableMarkup->render() (Line: 15)
Drupal\Core\StringTranslation\TranslatableMarkup->__toString()
implode('', Array) (Line: 594)
Drupal\Core\Template\TwigExtension->safeJoin(Object, Array, '') (Line: 44)
__TwigTemplate_f821c77039c217f90bf7a8f976486c27a1fbd3d077f048bd0be538f78a56fbe7->doDisplay(Array, Array) (Line: 381)
Twig_Template->displayWithErrorHandling(Array, Array) (Line: 355)
Twig_Template->display(Array) (Line: 366)
Twig_Template->render(Array) (Line: 116)
Drupal\Core\Template\TwigEnvironment->renderInline('{{ summary|safe_join("") }}', Array) (Line: 52)
Drupal\Core\Render\Element\InlineTemplate::preRenderInlineTemplate(Array)
call_user_func(Array, Array) (Line: 381)
Drupal\Core\Render\Renderer->doRender(Array, ) (Line: 195)
Drupal\Core\Render\Renderer->render(Array) (Line: 468)
Drupal\Core\Template\TwigExtension->escapeFilter(Object, Array, 'html', NULL, 1) (Line: 183)
__TwigTemplate_98cf6ee9c325be56e5f6e2b50b209532e42534eaeb86ca76b1fbc0eee1a03602->doDisplay(Array, Array) (Line: 381)
Twig_Template->displayWithErrorHandling(Array, Array) (Line: 355)
Twig_Template->display(Array) (Line: 46)
__TwigTemplate_3d8e9bca77977cbc4daac411f170a9413bcdd9963a172359b2366c70bfe6fdf3->doDisplay(Array, Array) (Line: 381)
Twig_Template->displayWithErrorHandling(Array, Array) (Line: 355)
Twig_Template->display(Array) (Line: 366)
Twig_Template->render(Array) (Line: 64)
twig_render_template('core/themes/stable/templates/admin/field-ui-table.html.twig', Array) (Line: 384)
Drupal\Core\Theme\ThemeManager->render('field_ui_table', Array) (Line: 435)
Drupal\Core\Render\Renderer->doRender(Array) (Line: 448)
Drupal\Core\Render\Renderer->doRender(Array, ) (Line: 195)
Drupal\Core\Render\Renderer->render(Array, ) (Line: 226)
Drupal\Core\Render\MainContent\HtmlRenderer->Drupal\Core\Render\MainContent\{closure}() (Line: 574)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 227)
Drupal\Core\Render\MainContent\HtmlRenderer->prepare(Array, Object, Object) (Line: 117)
Drupal\Core\Render\MainContent\HtmlRenderer->renderResponse(Array, Object, Object) (Line: 90)
Drupal\Core\EventSubscriber\MainContentViewSubscriber->onViewRenderArray(Object, 'kernel.view', Object) (Line: 111)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch('kernel.view', Object) (Line: 144)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 62)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 57)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 98)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 77)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 50)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 628)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

I can't reproduce #30; tested with and without and the warning goes away.

This fixes up a notice that pops up with PHP 7.

Just reviewed this issue.

Given the two approaches, I think #21 is the best. This is a minor bug, and #21 is a simple fix that avoids injecting any new services and directly addresses what caused it.

Manually tested #21 again, and think it is still good to go.

I came to this issue because I was seeing the same error with the "Checked" field from the core Aggregator module.
This is uses the TimestampAgoFormatter class. It is similar to the DateTimeTimeAgoFormatter but differs that it uses a timestamp.

Applying #21 on this class caused the following error:
PHP Fatal error: Call to a member function getTimestamp() on integer in /var/www/html/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampAgoFormatter.php on line 148, referer: http://v2.insideout.today/admin/config/services/aggregator/display

Applying #31 after changing the formatDate() function call to formatTimestamp() solved the issue with the field.

However applying the attached 2686409-33.patch on both classes fixes the issue.

Yeah, fixing that formatter here is probably appropriate, too.

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampAgoFormatter.php
@@ -140,8 +140,8 @@ public function settingsSummary() {
-    $summary[] = $this->t('Future date: %display', array('%display' => $this->formatTimestamp($future_date)));
-    $summary[] = $this->t('Past date: %display', array('%display' => $this->formatTimestamp($past_date)));
+    $summary[] = $this->t('Future date: %display', array('%display' => $this->formatTimestamp($future_date)['#markup']));
+    $summary[] = $this->t('Past date: %display', array('%display' => $this->formatTimestamp($past_date)['#markup']));
 

Not sure I really like the approach here. It's programming to the implementation, not the interface. The approach in #21 should work here, too. In this function, tough, the $future_date and $past_date are already timestamps, so you just don't need to call that method. Otherwise, it should work.

I think this would be the minimally invasive approach that fixes both formatters w/o needing to get the renderer involved and w/o poking around a render array for output.

Reapplied patch #35 to the 3.x branch

For a test we could just adapt \Drupal\Tests\field\Kernel\Timestamp\TimestampFormatterTest and call out to this function.

Hi guys I've also fixed this issue about summary for TimeAgoFormatter along with deprecated function replacement and some code refactoring changes and fixing wrong elements issue also here https://www.drupal.org/node/2867248 so check it out and review code and maybe we can close both issues then. Tnx

I've checked the patch and it's not showing format entered for example 1 year ago like you set it in config form but just 1 year in either way so I added a patch for this and pushing it now for review.

Nice catch.

Pondering

1. Whether we really should add #2867248: Timestamp Ago Formatter: Future and past format not selectable due to incorrect variable name... here, since the Timestamp settings are useless w/o that...
2. Whether we want to start a TimestampFieldTest. But, also see (1)

Few nits:

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampAgoFormatter.php
   @@ -12,6 +12,7 @@
    use Drupal\Core\Plugin\ContainerFactoryPluginInterface;
    use Symfony\Component\DependencyInjection\ContainerInterface;
    use Symfony\Component\HttpFoundation\Request;
   +use Drupal\Component\Render\FormattableMarkup;
   

   Needs to be alphabetical.

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampAgoFormatter.php
   @@ -140,8 +141,17 @@ public function settingsSummary() {
   +    $summary[] = $this->t('Future date: %display', array('%display' => $future_date_interval));
   +    $summary[] = $this->t('Past date: %display', array('%display' => $past_date_interval));
    
   

   Should use short array syntax.

3. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeTimeAgoFormatter.php
   @@ -13,6 +13,7 @@
    use Drupal\Core\Plugin\ContainerFactoryPluginInterface;
    use Symfony\Component\HttpFoundation\Request;
    use Symfony\Component\DependencyInjection\ContainerInterface;
   +use Drupal\Component\Render\FormattableMarkup;
    
   

   Needs to be alphabetical.

Leaning towards the merge, but not sure, so no need for new patch quite yet.

Needs work for #42.

Fixed #42 and moved #2867248: Timestamp Ago Formatter: Future and past format not selectable due to incorrect variable name... here. Added the test. Closing #2867248: Timestamp Ago Formatter: Future and past format not selectable due to incorrect variable name... as duplicate.

@mpdonadio, @jhedstrom shouldn't we extend DateTimeTimeAgoFormatter from TimestampAgoFormatter? Then we create a protected method TimestampAgoFormatter::getValue() that is simply transforming the field stored value into a format that can be passed to unified ::formatTimestamp() / ::formatDate(). we would avoid a lot of code duplication. Or just use a trait?

Here's a rework that extends DateTimeTimeAgoFormatter from TimestampAgoFormatter.

TODO: Shouldn't be the "empty output" configurable? Now is hardcoded as t('never') for TimestampAgoFormatter and nothing for DateTimeTimeAgoFormatter.

I needed the "time ago" formatter in our project but I found that is a bad choice. The problem is that a "time ago" interval string changes very often making it uncacheable. Then the whole page become uncacheable. What I did it was that I've created my own formatter by extending TimestampFormatter. I used the timeago jquery plugin to replace the nice formatted date/time with a "timeago string". In this way non-JS browsers will show a formatted date (the default behaviour of TimestampFormatter) and JS browsers (99%) will show the "time ago" interval. This is nice because:

1. Assures a dynamic time ago that can be refreshed by JS. I set the refresh to once per minute, but it can be made configurable.
2. Is cacheable
3. Degrades to a normal formatted date/time in non-JS

I think we should do the same in TimestampFormatter and drop/deprecate this formatter. We can add the "time ago" as a formatter option. It's not necessary to use timeago jquery plugin which seems not very flexible, we can go with the industry standard, which is Moment.js.

PS: Alternatively, we can keep the actual formatter and use a placeholder instead of rendering the the "time ago" but what is the gain?

The problem is that a "time ago" interval string changes very often making it uncacheable.

IIRC, this has been resolved for things like comment posted times, node posted times, etc. I had no idea it wasn't resolved for the datetime formatter. Let's add a follow-up issue to address that.

A pass at this was in #2500525: Time ago/hence date/time formatting breaks caching; needs appropriate max age.

@jhedstrom, @mpdonadio here's the followup where I proposed also the first patch #2921810: Allow TimestampFormatter to show as a fully cacheable time difference with JS. Unfortunately I don't have time to continue with that.

Thanks @claudiu.cristea

#45 worked for me, RTBC from my perspective (but I didn't set that assuming the core experts should review it).

Given the idea in #51 that TimestampAgoFormatter could be deprecated, then maybe #47 "extend DateTimeTimeAgoFormatter from TimestampAgoFormatter" is counter-productive? On that basis I have tentatively hidden #47 and re-exposed #45. However please put it back if I did the wrong thing.

This bug generates a warning that is quite confusing to site-developers so I wonder if the priority is more like "Normal"? Anyway it would be great to have a fix.

Possibly a blocker for #2638250: The label "Member for" on user profiles is hardcoded markup that is different from other user fields, so I've increased the priority and set to RTBC based on my review/test. Still needs review from the experts.

I need to read this applied, and stare at the test some more to think about whether it will be glitchy and prone to sporadic fails.

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampAgoFormatter.php
   @@ -66,7 +67,6 @@ class TimestampAgoFormatter extends FormatterBase implements ContainerFactoryPlu
   -
   

   Nit, unneeded change.

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampAgoFormatter.php
   @@ -153,51 +161,79 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
   -  protected function formatTimestamp($timestamp) {
   -    $granularity = $this->getSetting('granularity');
   +  protected function format(FieldItemInterface $item) {
   +    $timestamp = $this->getTimestamp($item);
   

   This is a BC problem if anyone has extended this formatter.

3. +++ b/core/tests/Drupal/FunctionalTests/Datetime/TimestampTest.php
   @@ -89,7 +91,7 @@ protected function setUp() {
   -      'mode' => 'full',
   +      'mode' => 'default',
   

   Whis is this needed?

4. +++ b/core/tests/Drupal/FunctionalTests/Datetime/TimestampTest.php
   @@ -144,6 +146,42 @@ public function testWidget() {
   +    // Check that values were saved correctly in the backend.
   +    $settings = EntityViewDisplay::load('entity_test.entity_test.default')
   +      ->getComponent('field_timestamp')['settings'];
   +    $this->assertEquals('ends in @interval', $settings['future_format']);
   +    $this->assertEquals('started @interval ago', $settings['past_format']);
   +    $this->assertEquals(3, $settings['granularity']);
   +
   

   The bug is that they didn't appear on Manage Display, so I think we want to assert the text there and not that they were saved properly.

5. +++ b/core/tests/Drupal/FunctionalTests/Datetime/TimestampTest.php
   @@ -144,6 +146,42 @@ public function testWidget() {
   +    foreach ([$past => 'started %s ago', $future => 'ends in %s'] as $timestamp => $format) {
   +      EntityTest::load($id)->set('field_timestamp', $timestamp)->save();
   

   Like this, and think it will be glitch free.

6. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampAgoFormatter.php
   @@ -140,8 +140,16 @@ public function settingsSummary() {
   -    $summary[] = $this->t('Future date: %display', ['%display' => $this->formatTimestamp($future_date)]);
   -    $summary[] = $this->t('Past date: %display', ['%display' => $this->formatTimestamp($past_date)]);
   +    $options = [
   +      'granularity' => $this->getSetting('granularity'),
   +      'return_as_object' => FALSE,
   +    ];
   +
   +    $future_date_interval = new FormattableMarkup($this->getSetting('future_format'), ['@interval' => $this->dateFormatter->formatTimeDiffUntil($future_date, $options)]);
   +    $past_date_interval = new FormattableMarkup($this->getSetting('past_format'), ['@interval' => $this->dateFormatter->formatTimeDiffSince($past_date, $options)]);
   +
   +    $summary[] = $this->t('Future date: %display', ['%display' => $future_date_interval]);
   +    $summary[] = $this->t('Past date: %display', ['%display' => $past_date_interval]);
    
   

   Kinda seems like this is replicating logic from the format() method?

Update to #55-4, I think we need to add that coverage on Manage Display in addition to the save check. Do we also need coverage in DateTimeFieldTest?

Rescoping this a bit.

Updated IS w/ credits from dup (I can't update the list).

And, this is a bug so it is 8.4.x eligible, but the patches conflict b/c #2830094: Deprecate and remove usages of datetime_date_default_time(). and possibly #2826404: Create DateTimeItemInterface and deprecate global constants in datetime.module. So, I am moving this to 8.5.x per our workflow. One we wrap up that version, we can do a quick backport.

We were getting Warning: htmlspecialchars() expects parameter 1 to be string, array given in Drupal\Component\Utility\Html::escape() after setting the "time ago" formatter on a display mode for a content type field. Patch #45 fixed the error popping up with no apparent ill effects. RTBC

However the review comments in #55, #56 have not yet been addressed so it can't be RTBC. If anyone is keen for RTBC it would be great if they can apply the comments.

I updated the patch for Drupal 8.6.

Bumping this because it breaks the UI with a large warning about arrays to strings and is quite common. Moving it back to 8.6 in hopes it can get in a patch release.

Bit of a do-over to match the IS. Will be adding tests shortly, but want to see if this causes any regressions.

Thank you for the quick patch update. That's a nice cleanup too, I hope it doesn't change too much.

Should we add another test back in? I'm trying to get my local tests working so I can help out here too

I have some tests in progress. I am working on a baseline TimestampAgoFormatterTest, which I will clone to DatetimeAgoFormatterTest. Doing this in bootstrap an effort to make DatetimeFieldTest manageable.

This should cover all of the changes. Will clone to DatetimeAgoFormatterTest.

This should be the minimal patch that addresses the bugs, covers the changed code, and tests for regressions.

I've manually tested the patch and reviewed the iterations of the patches and it now has tests to. Thank you @mpdonadio!

Wonder why I was uncredited here

#69, I am not sure what you mean? You have patches on this issue, and are in the credit list. Pretty sure I just added the additional credits in the IS #56 when we closed the dup for people who weren't listed yet who contributed on the other issue. Your patches were before then, so you were already listed.

?

I'm not sure who made the change, but please leave the "credits" section for the maintainers to adjust, it's improper / impolite to remove people from this list if you aren't in a position to commit changes.

Adding people to credit xSDx, JaceRider,jhedstrom

I've credited everyone who reviewed this issue and tried to move it along.

As there were no test-only patches as far as I can see - ran them locally. And yep we can see that they are adding the missing coverage.

Testing Drupal\Tests\datetime\Functional\DateTimeTimeAgoFormatterTest
E                                                                   1 / 1 (100%)

Time: 9.44 seconds, Memory: 4.00MB

There was 1 error:

1) Drupal\Tests\datetime\Functional\DateTimeTimeAgoFormatterTest::testSettings
Exception: Warning: htmlspecialchars() expects parameter 1 to be string, array given
Drupal\Component\Utility\Html::escape()() (Line: 424)

And

Testing Drupal\FunctionalTests\Datetime\TimestampAgoFormatterTest
E                                                                   1 / 1 (100%)

Time: 7.75 seconds, Memory: 4.00MB

There was 1 error:

1) Drupal\FunctionalTests\Datetime\TimestampAgoFormatterTest::testSettings
Exception: Warning: htmlspecialchars() expects parameter 1 to be string, array given
Drupal\Component\Utility\Html::escape()() (Line: 424)

Committed and pushed 21ba3dfc6f to 8.7.x and 855d0cf6d8 to 8.6.x. Thanks!

Backported to 8.6.x as this is a bug fix which does not make any API changes and make changes to anything covered by a BC promise and it fixes and error people are experiencing whilst using the UI.