Allow TimestampFormatter to show as a fully cacheable time difference with JS

moment.js is too heavy. Since the timezone doesn't matter, I suggested making a custom one and yeah, to sync with PHP

https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Datetime%...

Can we pause this for a little bit to talk about the solution? Don't want to get too far down the road and then realize that we have problems.

I like the JS part, especially since it looks like it matches the current PHP behavior, and the time element part (see also #2793143: The datetime and daterange formatters (except plain) should use the HTML time element).

Few initial concerns.

Even if we deprecate the timeago formatter, I think we have a problem w/ non-JS uses. Besides non-JS browsers, formatters get used in other places (Views outputting something other than HTML, REST, etc). I don't think we can deprecate w/ plans to remove.

There would also be the issue of what to do with #2686409: Time Ago summary does not render on Manage Display for Timestamp and Datetime fields and the Datetime timeago formatter, which would have the same problem.

I'm wondering if instead of deprecating the timeago formatter and moving this functionality to the default formatter, we make this an option on the timeago formatter.

- Default behavior would be the current, with the cache quirk
- Optional JS-enhanced, cache friendly, version would be taken from this patch.

That would mean no deprecations, no non-JS problems, no update path, just an optional enhanced experience.

?

@claudiu.cristea you can't deprecate and then still have usages. That's not deprecating. The constructor deprecation would work but, for example, views.view.user_admin_people is still using the plugin so of course the deprecation message is firing.

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampAgoFormatter.php
@@ -16,9 +16,17 @@
+ * @deprecated in Drupal 8.5.0, will be removed before Drupal 9.0.0. Use the
+ *   \Drupal\Core\Field\Plugin\Field\FieldFormatter\TimestampFormatter formatter
+ *   instead and configure it with "Display as 'time ago'" option. For details,
+ *   see https://www.drupal.org/node/2926275.
+ *
+ * @todo Add the deprecation error triggering and proper mark the plugin as
+ *   deprecated once https://www.drupal.org/project/drupal/issues/2922451 is in.

Unless you remove all usages in core then this is not deprecated - it is just hopeful. Perhaps to keep the issue smaller we should deal with the deprecation in a separate issue. Since that issue will need to add the @trigger_error, remove all the usages and add a legacy test.

You mean core usages? If yes, those should be replaced in this patch.

Well then if you are really replacing all the usages then you should be able to add the @trigger_error() to the constructor but the patch in #30 is not. For example there are a few views which are using it:

grep -R "type: timestamp_ago" ./core
./core/modules/aggregator/config/install/core.entity_view_display.aggregator_feed.aggregator_feed.default.yml:    type: timestamp_ago
./core/modules/comment/config/optional/views.view.comments_recent.yml:          type: timestamp_ago
./core/modules/node/config/optional/views.view.content_recent.yml:          type: timestamp_ago
./core/modules/user/config/optional/views.view.user_admin_people.yml:          type: timestamp_ago
./core/modules/user/config/optional/views.view.user_admin_people.yml:          type: timestamp_ago

And maybe other things too. This is the whole point of the @trigger_error() - to prove that the deprecation is real in core and also tell contrib / custom that in order to be ready for the future they need to change something too.

Impressive work!

Just a very high-level review. Definitely needs JS tests.

1. +++ b/core/config/schema/core.data_types.schema.yml
   @@ -743,6 +743,16 @@ field.value.float:
   +field.value.timestamp:
   

   I think this can be done in a separate issue?

2. +++ b/core/core.libraries.yml
   @@ -937,3 +937,12 @@ drupal.dialog.off_canvas:
   +    misc/timeago.js: {}
   

   This seems like it could be defer: true.

3. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampFormatter.php
   @@ -94,6 +96,16 @@ public static function defaultSettings() {
   +        'future_format' => '@interval hence',
   

   😯 Woah, never seen this before!

4. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampFormatter.php
   @@ -101,42 +113,92 @@ public static function defaultSettings() {
   -    $elements = parent::settingsForm($form, $form_state);
   +    $form = parent::settingsForm($form, $form_state);
   ...
   -
   ...
   -    $elements['date_format'] = [
   +    $form['date_format'] = [
   

   Can you avoid renames and whitespace changes, to make this patch easier to review?

5. +++ b/core/modules/system/templates/time.html.twig
   @@ -19,4 +19,6 @@
   +{% spaceless %}
    <time{{ attributes }}>{{ text }}</time>
   +{% endspaceless %}
   

   Can also be done in a separate issue.

@claudiu.cristea I don't mind helping with the test issues to avoid adding spaceless, ping me on IRC and I'll jump on that.

Exciting to see this coming along. Anything outstanding except #36.5 / #44?

Addressing my comment in #44. If possible we try to avoid making markup changes to stable/classy.

Spaceless can be a potenially slow(because of the preg_replace) compile change, so we've avoided it:
vendor/twig/twig/lib/Twig/Node/Spaceless.php:29

            ->write("ob_start();\n")
            ->subcompile($this->getNode('body'))
            ->write("echo trim(preg_replace('/>\s+</', '><', ob_get_clean()));\n")

We have a note here: https://www.drupal.org/node/1823416#whitespace in the caveat that suggest changing the test first. So that is what I did, by adding trim() around the tests that already had strip_tags() added.

And last, incase there is pushback, the alternative to spaceless is just add a twig comment after the tag. Like PHP, Twig will remove linebreaks after control statements and also comments.
Like this will work: <time{{ attributes }}>{{ text }}</time>{# This comment removes the linebreak after. #}

Yeah that is tricky. Perhaps the whole business of refreshing is best left to a follow-up.

The needed refresh interval depends on the date format, but we don't have easy way to understand that. A solution might be to try adding various time increments to the current time, 1 second, 1 minute, 1 hour, 1 day, 1 week, 1 month, 1 year. Then try to format these new times and see if they are different from the formatted current time. The first one to be different tells us what granularity we need to operate at.

The bigger pain is that the needed granularity changes. It might only be 1 second for the first 60 seconds, only 1 minute for the first hour etc. These possibilities can be handled in a similar way, but it's a bit complex and might need some javascript unit tests to ensure it's working.

+++ b/core/misc/timeago.es6.js
@@ -0,0 +1,141 @@
+  Drupal.behaviors.timestampAsTimeAgo = {
...
+          var showTimeAgo = function () {

We could export it as a global helper `Drupal.showTimeAgo` and `detach` to cancel the updates (e.g. when the element is removed from DOM).

I also preferred a recursive setTimeOut.

IMO, 5 ~ 10 mins updates is more acceptable.

+++ b/core/misc/timeago.es6.js
@@ -0,0 +1,141 @@
+          var timeagoSettings = JSON.parse(atob($(this).attr('data-drupal-timeago')));

why base64?

** Needs to fix the ES6 code style.

Thanks!

I had same idea to use setTimeout not setInterval.

Now instead of

            if (timeagoSettings.refresh > 0) {
              element.timer = setTimeout(Drupal.showTimeAgo, timeagoSettings.refresh * 1000, element);
            }

if we had something along the lines of

            if (timeagoSettings.refresh > 0) {
              refresh = nextTimeAgoRefresh(timeago.value, timeagoSettings.refresh, options);
              element.timer = setTimeout(Drupal.showTimeAgo, refresh, element);
            }

          var nextTimeAgoRefresh = function (current, interval, options) {
             // a function that returns the number of milliseconds until we should next refresh the timeAgo
             now = new Date();
             if (current.second) { return interval};
             if (current.minute) {
               remaining = (60 - now.getSeconds()); 
               return  min(remaining, interval) * 1000;
             };
             // etc for hours, days, weeks, years
          };

Could we set it up in such a way that unit test nextTimeAgoRefresh without having to worry about clock mocking?

Have no idea how this can be tested

It works I think:
(I have written in JS but should use PHPUnit instead. Or simply attach a JS test file)

// mock function only, have to use the right PHPUnit function in our REAL code.
function assertHTML() {
  return updated
 }
 
 var timerA;
 
 function countDown() {
   timerA = setTimeout(() => {
     console.log('working')
     countDown(); // #1
   }, 1000);
 }
 countDown();
 
 var updated;
 var timerB = timerA;
 console.log(`Initial: ${timerB === timerA}`);
 intialHTML = assertHTML() // from PHPUNIT function.
 
 console.log(`Immedate assert (FALSE): ${timerB !== timerA}`); // In PHPUNIT, we need not this step to validate the data. Only JS has blocking single-thread)

 // PHPUNIT: waitForElement / wait (for JS conditional)
 var setIntervalA = setInterval(function name(params) {
   const condition = timerB !== timerA;
   if(condition) {
    console.log(`Update 1: ${condition}`);
    timerB = timerA; // waiting for another changes
    updated = true; // at least one update after intial
    clearInterval(setIntervalA) // PHPUNIT will stop automatically
   }
 }, 1000)
 

  // PHPUNIT: waitForElement / wait (for JS conditional)
  var setIntervalB = setInterval(function name(params) {
    updatedHTML = assertHTML()    
    const condition = timerB !== timerA && updated && updatedHTML !== intialHTML;
    if(condition) {
      console.log(`Update 2: ${condition}`);
     clearInterval(setIntervalB)
    }
  }, 1000)

If you take #1 away (broken code), "Update 2" will be failed.

I may miss some point here. Why 5 sec but not 3 or 4?

If we expecting that counting up from 1, 2, 3, 4, 5. Bingo! Then, I think the test failure rate will be high.

#60 is trying to catch 2 continue changes. Now, I think we able to simplify it by set 2 assertJsCondition to assign to the value. If both values are greater than previous, `Drupal.showTimeAgo` is working

#2775653: JavascriptTests with webDriver is committed, unpostponing.

This patch seems to work fine! I think we need some more test coverage though. Here are some ideas.

+++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Field/TimestampFormatterWithTimeagoTest.php
@@ -0,0 +1,89 @@
+  public function testTimestampFormatterWithTimeago() {
...
+    $this->assertJsCondition("jQuery('time:contains(\"5 seconds\")').length > 0");

Since the interval is 1 sec asserting the exact value doesn't seem to be a stable option. Maybe we could improve the asserts a bit:

• Assert the element has the right classes and attributes
• Assert the '.js-timeago' element contains the word seconds.
• Create some more content with the created dates in the past and assert for days/weeks/years etc.
• Assert for other granularities (why not just all of them) and check the '.js-timeago' element contains the words weeks, months, years etc.
• Find a way to assert the refresh (this might be the hardest one)
• Assert the future / past formats

We are using the (locked) core html_datetime pattern for creating a RFC 3339 compliant datetime, which breaks the JS in Safari on iOS/OSX because Safari expects a valid RFC3339 datetime.

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampFormatter.php
@@ -166,26 +266,97 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
+        '#attributes' => [
+          // The representation of the date/time as RFC 3339 "date-time".
+          // @see https://www.w3.org/TR/2011/WD-html-markup-20110405/time.html
+          'datetime' => $this->dateFormatter->format($item->value, 'html_datetime', '', $timezone),

Core uses the pattern 'Y-m-d\TH:i:sO' which is not valid RFC3339. It results in (for example) "2018-08-13T16:17:33+0200".
The RFC3339 standard describes the format with a required colon between the offset timezone hours:

"2018-08-13T16:17:33+02:00"

The PHP DateTime class also states that the pattern should be "Y-m-d\TH:i:sP" (using the "P" format character instead of "O", see reference).

At first I thought this might be a core issue but I think core is assuming ISO8601 for html_datetime. So instead I would suggest using a custom format 'Y-m-d\TH:i:sP' which seems to work with all browsers.

Here is a new patch addressing the Safari issue explained #67.
Additional tests as mentioned in #66 by Sean still have to be written.

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampFormatter.php
   @@ -101,42 +114,101 @@ public static function defaultSettings() {
   +
   +    $form['timeago']['refresh'] = [
   +      '#type' => 'select',
   +      '#title' => $this->t('Refresh interval (seconds)'),
   +      '#description' => $this->t("The interval to refresh the displayed 'time ago'."),
   +      '#default_value' => $timeago['refresh'],
   +      '#options' => $this->getRefreshIntervals(),
   +      '#states' => $states,
   +    ];
   

   I do have one conceptually question. I get how nice it is to refresh this, but I would have expected from reading the first bit of the issue summary, that this is something which could be moved into its own dedicated issue

2. +++ b/core/misc/timeago.es6.js
   @@ -0,0 +1,278 @@
   +          Drupal.showTimeAgo = timeElement => {
   +            const timestamp = new Date(
   +              $(timeElement).attr('datetime'),
   

   You create this function dynamically once at least for every ```.each``` function. This could be moved to a more top level function right? This way it would need to be reassigned all the time.

   On top of that the code would be a bit easier to understand

3. +++ b/core/misc/timeago.es6.js
   @@ -0,0 +1,278 @@
   +   * There are cases when the configured refresh timeout performs the refresh
   +   * even when it is not needed. This function optimizes the configured refresh
   +   * timeout to higher values, if the structure of the timeago interval doesn't
   +   * require refreshing too often.
   

   It would be super nice to give some examples in the summary of this function. I think this would massively make it easier to understand what is going on

4. +++ b/core/misc/timeago.es6.js
   @@ -0,0 +1,278 @@
   +  Drupal.dateFormatter.formatDiff = (from, to, options) => {
   ...
   +    options = $.extend({ granularity: 2, strict: true }, options);
   

   I would have expected that core JS could use default values on options already

5. +++ b/core/misc/timeago.es6.js
   @@ -0,0 +1,278 @@
   +    $.each(Drupal.dateFormatter.intervals, (interval, duration) => {
   

   ❓Is there a reason we don't use ```Array.prototype.forEach```?

Awesome work @claudiu.cristea. Most of these are nits or UX wordings, all are tentative suggestions.

1. +++ b/core/config/schema/core.entity.schema.yml
   @@ -331,6 +331,35 @@ field.formatter.settings.timestamp:
   +      label: 'Options to show as timeago'
   

   Possibly 'Time difference'. The 'Options' seems unnecessary in the label and not consistent with tooltip.

2. +++ b/core/config/schema/core.entity.schema.yml
   @@ -331,6 +331,35 @@ field.formatter.settings.timestamp:
   +          label: 'Granularity'
   

   Maybe 'Granularity of format' or 'Time units'?

3. +++ b/core/core.libraries.yml
   @@ -942,3 +942,13 @@ drupal.dialog.off_canvas:
   +drupal.timeago:
   +  version: VERSION
   +  js:
   +    misc/timeago.js:
   

   Should the addition of this behavior and library be mentioned in the change record?

4. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampFormatter.php
   @@ -101,42 +114,101 @@ public static function defaultSettings() {
   -    $elements['date_format'] = [
   +    $form['date_format'] = [
   

   What is date_format used for if using timeago? Should we be hiding it?

5. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampFormatter.php
   @@ -101,42 +114,101 @@ public static function defaultSettings() {
   +    $tooltip = $this->getSetting('tooltip');
   +    $form['tooltip']['#tree'] = TRUE;
   +    $form['tooltip']['date_format'] = [
   

   It seems strange having this above the timeago in the form; I think it should be below?

6. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampFormatter.php
   @@ -101,42 +114,101 @@ public static function defaultSettings() {
   +      '#title' => $this->t("Display as 'time ago'"),
   +      '#description' => $this->t("If checked, the timestamp will be displayed as a 'time ago' string, in javascript enabled browsers. With javascript disabled, the main behavior is in place."),
   

   This is problematic as help text because 'time ago' is not a phrase a normal person understands, and ago is specifically past. I'm also not sure we need to explain the no-JS fallback, and 'If checked' is a bit clumsy.

   I suggest:
   'Display as a time difference' and
   "Show the difference in time between the timestamp and the current time, e.g. '6 months ago'."

   Edit: I see now why we're mentioning JS/non-JS, it helps explain what the other time format settings are doing. I'm not sure what to propose, but overall the settings form is a little puzzling to the novice as it stands.

7. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampFormatter.php
   @@ -101,42 +114,101 @@ public static function defaultSettings() {
   +      '#description' => $this->t('Use <em>@interval</em> where you want the formatted interval text to appear.'),
   

   We should make our language consistent with the labels above. Maybe 'where you want the formatted time difference to appear'?

8. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampFormatter.php
   @@ -101,42 +114,101 @@ public static function defaultSettings() {
   +      '#type' => 'number',
   

   Would this be better as a select element, as it only has 7 possible values?

9. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampFormatter.php
   @@ -101,42 +114,101 @@ public static function defaultSettings() {
   +      '#title' => $this->t('Granularity'),
   

   Maybe 'Time units' would help the layman more

10. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampFormatter.php
    @@ -101,42 +114,101 @@ public static function defaultSettings() {
    +      '#description' => $this->t('How many time interval units should be shown in the formatted output.'),
    

    This could use more explanation. Maybe 'How many time units will be used in formatting the time difference. For example, if '1' is selected then the displayed time difference will only contain a single time unit such as '2 years' or '5 minutes' never '2 years 3 months' or '5 minutes 8 seconds'.

11. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampFormatter.php
    @@ -101,42 +114,101 @@ public static function defaultSettings() {
    +    $form['timeago']['refresh'] = [
    
    @@ -166,26 +266,97 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
    +        $elements[$delta]['#attributes']['data-drupal-timeago'] = Json::encode($settings);
    
    +++ b/core/misc/timeago.es6.js
    @@ -0,0 +1,278 @@
    +   *
    
    +++ b/core/misc/timeago.js
    @@ -0,0 +1,143 @@
    +          var timeagoSettings = JSON.parse($(timeElement).attr('data-drupal-timeago'));
    

    Now that we have intelligent refreshing that dynamically adapts to the time interval, I'm not clear of the point of this setting.

    In core we usually like to keep things simple and extensible rather than cater for every possibility.

    A 60 second minimum refresh interval would seem to be safe for almost any browser even with a huge amount of fields on screen, and adequate for 99% of use cases.

    It would be trivial for someone to extend this formatter and specify a faster refresh using the data-drupal-timeago attribute.

12. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampFormatter.php
    @@ -101,42 +114,101 @@ public static function defaultSettings() {
    +      '#title' => $this->t('Refresh interval (seconds)'),
    

    It's confusing labelling this as seconds and then having '1 minute' as the default option.

13. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampFormatter.php
    @@ -101,42 +114,101 @@ public static function defaultSettings() {
    +      '#description' => $this->t("The interval to refresh the displayed 'time ago'."),
    

    Perhaps 'How often (in seconds) to refresh the displayed time difference.

14. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampFormatter.php
    @@ -147,13 +219,41 @@ public function settingsSummary() {
    +    $summary[] = $this->t('Tooltip date format: @date_format', ['@date_format' => $tooltip['date_format']]);
    ...
    +      $summary[] = $this->t("Displayed as 'time ago'");
    ...
    +      $summary[] = $this->t('Past date: %display', ['%display' => $display]);
    ...
    +        $summary[] = $this->t('Refresh in @interval', ['@interval' => $refresh_intervals[$timeago['refresh']]]);
    

    We're adding a lot of options here. Do we add everything to the summary or does that get too big at some point?

    Also better 'Displayed as a time difference'.

    Better 'Refresh every @interval'. And this doesn't handle the 'No refresh' option very well.

15. +++ b/core/misc/timeago.es6.js
    @@ -0,0 +1,278 @@
    +   * There are cases when the configured refresh timeout performs the refresh
    +   * even when it is not needed. This function optimizes the configured refresh
    ...
    +   *   The configured refresh timeout interval in seconds.
    ...
    +      // interval duration is bigger than the configured refresh, use the
    

    It shouldn't say configured as the value might not come from configuration in all use cases.

16. +++ b/core/misc/timeago.es6.js
    @@ -0,0 +1,278 @@
    +    if (lastInterval !== 'second') {
    

    Does this respect really high refresh timeouts like 1000 seconds? Looks like it may bypass them and update times like "3 minutes" every minute.

17. As I understand it, the CR should be focused on what's changed. This might be clearer if it emphasised more what has been added to the default formatter, not the comparison with the timestamp_ago formatter. I'm happy to edit for grammar once the content is settled.

Changing the machine & variable names to 'diff' not 'ago' seems like a good decision/

You didn't reply to #83.11 where I suggest removing the refresh setting from the formatter UI & config.

Some nits:

1. +++ b/core/misc/time-diff.es6.js
   @@ -1,83 +1,83 @@
   +   * Replaces a timestamp, formatted as a date/time, with a time difference.
   

   Is this comment quite accurate? The code doesn't seem to know or care how the time is currently formatted.

   Should it be something more like 'Fills all time elements that have the data-drupal-timeago attribute with a refreshing time difference'?

2. +++ b/core/misc/time-diff.es6.js
   @@ -95,57 +95,66 @@
   +   *   The configured refresh interval in seconds.
   ...
   +      // lowest unit duration is bigger than the configured refresh, use the
   

   'refresh interval' not 'configured refresh'

3. +++ b/core/misc/time-diff.es6.js
   @@ -95,57 +95,66 @@
       * require refreshing too often.
   

   Should be 'refreshing more often'.

#86 thanks for the reply - I think it's good to have it spelled out like this so others can decide.

I've worked on the CR, please check to see you think it's better and still accurate.

I created #2993811: Use the time-diff library in the datetime module's formatters as the needed follow-up. Looks like we need @mpdonadio to weigh in now.

#82.5: I wasn't able to use Array.prototype.forEach(). Probably because we're iterating over an object, not over an array? I have no idea.

In that case you can use Object.keys() | Object.values() ... dependencies on the usecase.

Sincerely apologize for being away for so long. Life got in the way...

Reread the issue a few times and the latest patch.

I think essentially what we are saying is (restating the IS in my own words more for my own benefit)

- TimestampAgoFormatter, with the max-age stuff, works well for ago formats places where things can be old, but still want to update, like the "On Drupal.org ..." on our profiles.
- TimestampAgoFormatter, with the max-age stuff, works poorly for ago formats that change rather frequently, like the top of Twitter.

In other words, the max-age is good in theory, but in practice has big UX/DX impacts. So

- Let's output timestamps as proper HTML5 time elements regardless, with the correct attribute
- When outputting an ago format, add a JS ticker to update the display value of the HTML5 time element
- Defer TTL to the consumer
- Defer what to do with TimestampAgoFormatter and the datetime formatters to a followup

And we are also implying this is the better choice than adding a setting to TimestampAgoFormatter to output as a non-cached HTML5 time element w/ JS updater?

I think I am OK with this. Not passing the buck, but I think getting @jhedstrom to weigh in too, is a good idea. There is nothing technically wrong with this patch at a high level, if @jhedstrom thinks this is OK, too, we are just down to checking it for mistakes.

NM.

I spent quite a bit of time reviewing the patch, and manually testing. I can't find anything to improve upon at this time, and it definitely works as expected. Allowing the time-ago format on fully-cacheable markup will be great, and even better when we get this in in the follow-up for the datetime formatters. This even has tests for the upgrade path, which is great!

I'm not sure why the testbot is claiming the patch doesn't apply. I've applied to to the latest 8.7.x locally with no problem.

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampFormatter.php
   @@ -166,26 +265,97 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
   +    if ($tooltip['date_format'] === 'custom' && $tooltip['custom_date_format'] === 'r') {
   +      $tooltip_langcode = 'en';
   +    }
   

   This appears to be untested.

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampFormatter.php
   @@ -166,26 +265,97 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
   +      1 => $this->t('1 second'),
   +      10 => $this->t('10 seconds'),
   +      15 => $this->t('15 seconds'),
   +      30 => $this->t('30 seconds'),
   +      60 => $this->t('1 minute'),
   +      300 => $this->t('5 minutes'),
   +      600 => $this->t('10 minutes'),
   
   +++ b/core/misc/time-diff.es6.js
   @@ -0,0 +1,287 @@
   +              Drupal.formatPlural(units, '1 minute', '@count minutes'),
   ...
   +              Drupal.formatPlural(units, '1 second', '@count seconds'),
   

   If you used formatPlural() here and not t() then we'd be introducing less new translatable strings. Also this seems to be a lot of options. 10,15,30 - maybe we only need one of those? Or was there a reason for the chosen numbers - if so it should be documented.

3. +++ b/core/misc/time-diff.es6.js
   @@ -0,0 +1,287 @@
   +    year: 365 * 24 * 60 * 60,
   +    month: 30 * 24 * 60 * 60,
   +    week: 7 * 24 * 60 * 60,
   +    day: 24 * 60 * 60,
   +    hour: 60 * 60,
   +    minute: 60,
   +    second: 1,
   

   Let's do the math here. There's no need for computers to do this everytime we load the JS it's not going to change.

4. +++ b/core/modules/system/system.post_update.php
   @@ -169,3 +170,26 @@ function system_post_update_extra_fields(&$sandbox = NULL) {
   +function system_post_update_timestamp_formatter() {
   +  /** @var \Drupal\Core\Entity\Display\EntityViewDisplayInterface $entity_view_display */
   +  foreach (EntityViewDisplay::loadMultiple() as $entity_view_display) {
   

   This needs to use the \Drupal\Core\Config\Entity\ConfigEntityUpdater - see image_post_update_scale_and_crop_effect_add_anchor() for an example.

5. +++ b/core/modules/system/system.post_update.php
   @@ -169,3 +170,26 @@ function system_post_update_extra_fields(&$sandbox = NULL) {
   +        if (get_class($formatter_plugin) === TimestampFormatter::class || is_subclass_of($formatter_plugin, TimestampFormatter::class)) {
   

   Can use instanceof here. I.e if ($formatter_plugin instanceof TimestampFormatter::class) {

6. +++ b/core/modules/system/system.post_update.php
   @@ -169,3 +170,26 @@ function system_post_update_extra_fields(&$sandbox = NULL) {
   +          $component['settings'] = $formatter_plugin->getSettings();
   +          $entity_view_display->setComponent($name, $component);
   

   Do we actually need to do this? Won't the entity view display self heal when you save it and add the default in?

Rerolled patch #86 for Drupal 8.6.4

Unrelated test failure. Back to RTBC.
#3035318: `DateFormatter()` assumes 30 days per month, while February only has 28 days. Causes fails in tests.

2 of the last 16 tests (12,5%) ended with the same random failure.
https://www.drupal.org/pift-ci-job/1242425

25 Feb 2019: https://www.drupal.org/pift-ci-job/1211085
1 Mar 2019: https://www.drupal.org/pift-ci-job/1215415

Drupal\FunctionalJavascriptTests\Core\Field\TimestampFormatterWithTimeDiffViewsTest::testTimestampFormatterWithTimeDiff
Javascript condition met:
jQuery('.entity-11 time').text() > '9 seconds ago'
Failed asserting that false is true.

/var/www/html/core/tests/Drupal/KernelTests/AssertLegacyTrait.php:35
/var/www/html/core/tests/Drupal/FunctionalJavascriptTests/WebDriverTestBase.php:145
/var/www/html/core/tests/Drupal/FunctionalJavascriptTests/Core/Field/TimestampFormatterWithTimeDiffViewsTest.php:66

FAILURES!
Tests: 1, Assertions: 15, Failures: 1.

This test testTimestampFormatterWithTimeDiff() generates random test failures so I change to needs work.

--- /dev/null
+++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Field/TimestampFormatterWithTimeDiffViewsTest.php
@@ -0,0 +1,141 @@
+
+  /**
+   * Tests the timestamp formatter used with time difference setting in views.
+   */
+  public function testTimestampFormatterWithTimeDiff() {
+    $data = $this->getRowData();
+
+    // Create the entities.
+    foreach ($data as $delta => $row) {
+      EntityTest::create([
+        'type' => 'test',
+        // Using this also as field class.
+        'name' => "entity-$delta",
+        'created' => $row['timestamp'],
+      ])->save();
+    }
+    $this->drupalGet('formatter_timestamp_as_time_diff');
+
+    $page = $this->getSession()->getPage();
+    foreach ($data as $delta => $row) {
+      $time_diff = $page->find('css', ".entity-$delta")->getText();
+      $regex_pattern = "#{$row['pattern']}#";
+      // Test that the correct time difference is displayed. Note that we are
+      // able to check an exact match for rows that have a creation date more
+      // distant, but we use regexp to check the entities that are only few
+      // seconds away because of the latency introduced by the test run.
+      $this->assertRegExp($regex_pattern, $time_diff);
+    }
+
+    // Wait at least 1 second + 1 milisecond to make sure the 'right now'
+    // time difference was refreshed.
+    $this->assertJsCondition("jQuery('.entity-$delta time').text() > '$time_diff'", 1001);
+  }

I re-tested #108 to confirm the previous comment.

The test failed again https://www.drupal.org/pift-ci-job/1245779

Drupal\FunctionalJavascriptTests\Core\Field\TimestampFormatterWithTimeDiffViewsTest::testTimestampFormatterWithTimeDiff
Javascript condition met:
jQuery('.entity-11 time').text() > '9 seconds ago'
Failed asserting that false is true.

/var/www/html/core/tests/Drupal/KernelTests/AssertLegacyTrait.php:35
/var/www/html/core/tests/Drupal/FunctionalJavascriptTests/WebDriverTestBase.php:145
/var/www/html/core/tests/Drupal/FunctionalJavascriptTests/Core/Field/TimestampFormatterWithTimeDiffViewsTest.php:66

FAILURES!
Tests: 1, Assertions: 15, Failures: 1.

I think the failures are all related to the switch to daylight savings that happens around this time of year for various regions. Moving back to RTBC.

1. The default timezone for testing is Australia/Sydney (See core/includes/install.core.inc). All previous random test failures were repeated on different days so I think they are not related to DST.

25 Feb 2019: https://www.drupal.org/pift-ci-job/1211085
1 Mar 2019: https://www.drupal.org/pift-ci-job/1215415
30 Mar 2019 https://www.drupal.org/pift-ci-job/1245779

2. Here is a recent random failure probably related to the switch to daylight savings in Australia.

#3046570: \Drupal\Tests\datetime\Kernel\Views\FilterDateTest::testDateOffsets can fail during DST changes

3. Patch #108:

--- /dev/null
+++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Field/TimestampFormatterWithTimeDiffViewsTest.php
@@ -0,0 +1,141 @@
+
+  /**
+   * Tests the timestamp formatter used with time difference setting in views.
+   */
+  public function testTimestampFormatterWithTimeDiff() {

     (...)

+
+    // Wait at least 1 second + 1 milisecond to make sure the 'right now'
+    // time difference was refreshed.
+    $this->assertJsCondition("jQuery('.entity-$delta time').text() > '$time_diff'", 1001);
+  }

IMO this looks like time-sensitive random test failure.

New in Symfony 2.8: Clock mocking and time sensitive tests:

This code is so simple that it seems impossible to fail. However, depending on the load of the server, the $duration could be for example 10.00000023 and the test would fail for no apparent reason.

For clock mocking see also #2986688: DrupalDateTime does not respect \Drupal::time() when interpreting strings as dates and https://www.drupal.org/project/datetime_testing

1. +++ b/core/misc/time-diff.es6.js
   @@ -0,0 +1,287 @@
   +        $.each(Drupal.dateFormatter.intervals, (interval, duration) => {
   +          if (interval === lastUnit) {
   +            refresh = refresh < duration ? duration : refresh;
   +            return false;
   +          }
   +        });
   

   nit: we could return after this and avoid the else

2. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Field/TimestampFormatterWithTimeDiffViewsTest.php
   @@ -0,0 +1,135 @@
   +  public function testTimestampFormatterWithTimeDiff() {
   

   How confident are we this won't introduce random fails?

3. +++ b/core/tests/Drupal/Tests/Core/Datetime/DrupalDateTimeTest.php
   @@ -11,6 +15,8 @@
   +  use PhpunitCompatibilityTrait;
   +
   

   By my reckoning UnitTestCase already includes this - so not needed?

NW for last point

Addressed comments #125 in updated patch & added an interdiff as well.

#127 does not remove the "else" that was the purpose of #125.1

Made changes as per #128 & #125.1 & added an interdiff as well.

Thanks @claudiu.cristea for review, Comments addressed in #131 & added an interdiff as well.

Adding 'else condition' again in code which was removed by me in #129 mistakenly.

Patch from #133 reuploaded.

Rerolled the patch - there was a simple conflict in core/tests/Drupal/Tests/Core/Datetime/DateTest.php due to the getMock deprecation. I also fixed an es6 to js complication thing.

I have not reviewed the patch properly yet.

Closed #3061755: Use time tag in TimestampFormatter as a duplicate issue.

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampFormatter.php
   @@ -101,42 +114,100 @@ public static function defaultSettings() {
   +      '#description' => $this->t('Use <em>@interval</em> where you want the formatted interval text to appear.'),
   

   I can't really understand what the description is suggesting here. Tagging for screenshots too.

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampFormatter.php
   @@ -101,42 +114,100 @@ public static function defaultSettings() {
   +      '#description' => $this->t('Use <em>@interval</em> where you want the formatted interval text to appear.'),
   

   Same issue.

3. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampFormatter.php
   @@ -101,42 +114,100 @@ public static function defaultSettings() {
   +      '#description' => $this->t('How often to refresh the displayed time difference.'),
   

   Should this explicitly say it's for JavaScript refresh? Also do we really need this to be configurable per formatter - could it not be a sitewide thing controlled by settings?

4. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampFormatter.php
   @@ -164,28 +263,87 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
   +
   +    if ($time_diff['enabled']) {
   +      $elements['#attached']['library'][] = 'core/drupal.time-diff';
        }
    
   

   Could we move this above the foreach, it looks a bit disjointed from everything else down here.

5. +++ b/core/modules/system/system.post_update.php
   @@ -206,3 +207,22 @@ function system_post_update_add_expand_all_items_key_in_system_menu_block(&$sand
   +function system_post_update_timestamp_formatter(&$sandbox = NULL) {
   

   Like some other config entity updates, this should be done in a config save listener so that it also catches default configuration provided by modules and similar, and then the update can just load and save all entity view displays.

Hi there. I'm testing lastest patch (#142). I've seen a difference when patching in :

use Drupal\Core\StringTranslation\TranslatableMarkup;
@@ -67,6 +69,9 @@ protected function setUp() {
     $this->entityTypeManager->expects($this->any())->method('getStorage')->with('date_format')->willReturn($entity_storage);

$this->languageManager = $this-><strong>createMock</strong>('Drupal\Core\Language\LanguageManagerInterface');

My file shows getMock instead of createMock. I applied manual modifications to this file and all seems to be OK.

I really don't know can I apply this modifications to article comments views in my site and I'm using twig templates variables. Any tips?

BTW the label shows in the system and I using it for taxonomy terms and frontpage.

Thanks to all to colaborating with this.

The patch is not applicable to 8.8.x.