Add DateFormatter::formatDiff() as a non-buggy alternative to DateFormatter::formatInterval() when the start and end of the interval are known

The best option in my opinion would be to remove the month/year units from this class. The rest is OK. The month/year stuff is simply wrong.

Then add a method that could be used to format a time difference the right way, with two date/time inputs.

Let's at least fix the docs so they warn people about this problem.

I also don't think we should be using this in Core to format intervals between two dates, unless we know they're under 30 day intervals. We should write a better function that does it right.

It's OK with me if you want to demote the issue, I'm not going to get into a Priority war over it. But just because some logic has been around for 14 years doesn't mean it isn't flawed, and isn't being misused.

And this method really shouldn't be used the way it's being used all over Core. The idea of making "ago" formatting based on fixed number of seconds is only valid up to 30 days, so if an interval could be more than that, it doesn't work well. I'm not too worried about it on the "How long is it since you ran cron" output, which hopefully shouldn't be all that long.

But this function is also being suggested for use in a generic formatter for date/time fields, where the input could be anything, and we don't know what the output is being used for, the output could be quite misleading and/or incorrect. See #2399211: Support all options from views fields in DateTime formatters. And there are quite a few other uses in core already.

Correct formatting is built into PHP's DateInterval class, which you can generate by taking the difference of two DateTime objects:
http://php.net/manual/en/dateinterval.format.php
See example #3 for instance, and the other documentation on the class.

Ummm... I attempted to review this patch, but ... I'm could not understand the code at all. Can you give some explanation, or better yet put in some code comments and doc headers to explain what it's doing? I have no idea.

But fundamentally: why would we not just use the built-in PHP classes/code to create and format intervals? It looks like maybe the only thing is that it doesn't format using weeks. Are they that important that we need them? I doubt it, especially if it means sacrificing accuracy.

OK. Here is what I think we should do:

a) Use the patch in #9 to warn people not to use formatInterval for cases where it is inappropriate. Don't change the code in this method... although I think that the code in formatInterval() is not properly translated??!? So we should fix that!

b) Make a new method DateFormatter::formatDiff that takes as input:
- Two timestamps, with the second one defaulting to the current time
- Time zone
- Granularity
- Language code

This method would:
- Convert both timestamps to DateTime objects using the DateTime::setTimestamp() method, setting time zone as well. http://php.net/manual/en/datetime.settimestamp.php and http://php.net/manual/en/datetime.settimezone.php
- Find the difference using DateTime::diff(). This will return a DateInterval object. http://php.net/manual/en/datetime.diff.php
- Format this using DateInterval::format() with a special format string of '%R.%y.%m.%d.%h.%i.%s'. http://php.net/manual/en/dateinterval.format.php
- Use explode() to separate this into pieces, and with the granularity, figure out which pieces to print and what the values are. For instance, the result might be that it's negative (from the %R), and with granularity 2 we need to format it as 2 years and 5 days.
- Use format_plural() appropriately to format each piece, and concatenate them together. So we might have a call to format_plural(2, '1 year', '%count years'). Each piece needs to have its own call to format_plural() with the strings ***in the call*** or it will not be translated properly.
- Finally use t() to make this "ago" or ... not sure what we use for "in the future"?

c) Make sure this new method and formatInterval() link to each other with @see

d) Look at the uses of formatInterval() in Core at the moment and change most/all of them to use this new method.

Does this make sense? If so, do you want to make a patch or should I?

The reason I suggested using DateInterval::format() is that it leaves the calculation of everything to DateInterval... However, I just realized (duh!) that DateInterval has public properties that give you $y, $m, etc. so that is not necessary. ;)

I think we should probably do everything in this patch, otherwise it will not be clear why this method is even needed.

Regarding #14(a) translation... Normally when you call t() or format_plural(), or equivalent methods, you need to pass in the literal strings and not variables.

It turns out that in this case, the Drupal-specific POTX extraction code special-cases the hard-coded strings in the formatInterval class (or format_interval function in previous versions), because whoever wrote the function didn't bother to call format_plural() correctly.

This should be fixed. I discussed it with Gabor in IRC and we agreed on that; and our new method should do the same. Basically you need to have a switch/case statement instead of putting the strings on a $units member variable.

Regarding +/- or ago/hence... maybe the new method could have an input that indicates how to handle this? There could be a few options, like:
- Use - on each element to indicate past, so your output would be like "-2 years - 3 days" vs. "2 years 3 days"
- Use - in the front once, so it would be something like "- 2 years 3 days" vs. "2 years 3 days"
- Same as those two options, but use +/- so it would be "-2 years - 3 days" vs. "+2 years +3 days"
- Use ago/hence, so it would be something like "2 years 3 days ago" vs. "2 years 3 days hence"

? just brainstorming here. We are creating a new method, so we can make it do whatever we want. What are the uses in Core, do they always use ago/hence? If so that should be the default?

These are changes to the underlying API, not to any formatter. But inasmuch as the TimestampAgoFormatter calls formatInterval, yes we would find all places where Core is calling this and replace them probably with calls to this new method.

So right now, formatInterval is doing:

  foreach ($this->units as $key => $value) {
    $key = explode('|', $key);
    if ($interval >= $value) {
      $output .= ($output ? ' ' : '') . $this->formatPlural(floor($interval / $value), $key [0], $key [1], array(), array('langcode' => $langcode));

What it needs to be doing is something like this for $units:

protected $units = array(
  'years' => 31536000,
  'months' => 2592000,
...
);

and then in that $output line, something like:

  switch ($key) {
     case 'years':
        $this_one = format_plural(floor($interval / $value), '1 year', '@count years', $langcode);
...

And here is the code that hard-wires these strings in the "potx" project:
http://cgit.drupalcode.org/potx/tree/potx.inc#n1495
If t() and format_plural() are called properly, with literal strings instead of variables, kludges like this are NOT necessary.

I like the idea of the arguments, and am fine with getting rid of the ability to repeat the - sign. We can always add it later if we need it.

Oh and maybe the args could be consolidated, so the prefix/suffix are an array of two values, for what to do if it's positive and what to do if it's negative? What about if the interval is zero too? ?!?

So it could be something like callThis($date1, $date2, array('hence', 'ago')) of course that needs t() in there... but you get the idea.

Attributes: good idea... although perhaps calling it $options would be better, since $attributes tends to be used in code specifically for HTML element attributes? Also putting them all in one array makes it easy to use array addition to set defaults. Also it makes it a LOT easier for someone to set one value and ignore the rest.

Most recent patch is looking good -- coming together! A few thoughts:

1. +++ b/core/lib/Drupal/Core/Datetime/DateFormatter.php
   @@ -193,6 +199,79 @@ public function formatInterval($interval, $granularity = 2, $langcode = NULL) {
   +   *   prepended with a minus sign if timestamp1 is larger than timestamp2.
   ...
   +    $date_time1->setTimezone(new \DateTimeZone($timezone));
   +
   +    $date_time2 = new \DateTime();
   +    $date_time2->setTimestamp($timestamp2);
   +    $date_time2->setTimezone(new \DateTimeZone($timezone));
   +
   

   I would avoid the word "larger" here, talking about times, and instead use either "before" or "after".

2. +++ b/core/lib/Drupal/Core/Datetime/DateFormatter.php
   @@ -193,6 +199,79 @@ public function formatInterval($interval, $granularity = 2, $langcode = NULL) {
   +    $date_time1->setTimezone(new \DateTimeZone($timezone));
   +
   +    $date_time2 = new \DateTime();
   +    $date_time2->setTimestamp($timestamp2);
   +    $date_time2->setTimezone(new \DateTimeZone($timezone));
   +
   

   Slight optimization: could just create the TimeZone object once?

3. +++ b/core/lib/Drupal/Core/Datetime/DateFormatter.php
   @@ -193,6 +199,79 @@ public function formatInterval($interval, $granularity = 2, $langcode = NULL) {
   +    foreach ($interval as $key => $value) {
   +      if ($value > 0) {
   +        // Switch over the keys to call formatPlural() explicitly with literal
   

   Hm. This makes me a bit uncomfortable. $interval is a date-time-interval class object, not an array. Are we sure what order this comes in as, and that there are not any values in the foreach besides the ones you think should be there? Is it documented somewhere? maybe it would be clearer and safer to make our own array of ('y', 'm', 'd', ...) and iterate on that, and get the values with $interval->$key?

4. +++ b/core/lib/Drupal/Core/Datetime/DateFormatter.php
   @@ -193,6 +199,79 @@ public function formatInterval($interval, $granularity = 2, $langcode = NULL) {
   +            $output .= ($output ? ' ' : '') . $this->formatPlural($interval->format('%' . $key), '1 year', '@count years', array(), array('langcode' => $langcode));
   

   I think we can just call $interval->$key rather than asking $interval to format itself? or use $value?

5. +++ b/core/modules/views/src/Plugin/views/field/Date.php
   @@ -151,13 +151,13 @@ public function render(ResultRow $values) {
   -          return $this->dateFormatter->formatInterval($time_diff, is_numeric($custom_format) ? $custom_format : 2);
   +          return $this->dateFormatter->formatDiff($value, REQUEST_TIME, 'UTC', is_numeric($custom_format) ? $custom_format : 2);
            case 'time ago':
   

   I'd be surprised if 'UTC' was the correct time zone to use for this (or the other ones in this area of the patch)?

6. +++ b/core/tests/Drupal/Tests/Core/Datetime/DateTest.php
   @@ -128,4 +128,111 @@ public function testFormatIntervalZeroSecond() {
   +  public function providerTestFormatDiff() {
   

   At some point, some tests with time zones seem like a good idea.

Whoops. That first code segment in my previous comment should have been the docs for the new method, where it says something about one timestamp being "larger" than the other. Sorry about the confusion!

Thanks for investigating the time zones! That seems sensible to me -- no time zones then.

I took a look at the latest patch and have some questions/suggestions, but it looks pretty good!

1. +++ b/core/lib/Drupal/Core/Datetime/DateFormatter.php
   @@ -193,6 +199,95 @@ public function formatInterval($interval, $granularity = 2, $langcode = NULL) {
   +   *   (optional) An associative array with additional options. Following keys
   

   Should say "The following keys..."

2. +++ b/core/lib/Drupal/Core/Datetime/DateFormatter.php
   @@ -193,6 +199,95 @@ public function formatInterval($interval, $granularity = 2, $langcode = NULL) {
   +   *   can be used:
   +   *   - granularity: An integer value that signals how many different units to
   +   *   display in the string. Defaults to 2.
   +   *   - langcode: A string representing a language code. It is used to
   +   *   translate to a language other than what is used to display the page.
   +   *   Defaults to NULL, which results in the current user language being used.
   +   *
   

   Formatting here: The text on the second line below the - list bullets should be indented 2 spaces.

3. +++ b/core/lib/Drupal/Core/Datetime/DateFormatter.php
   @@ -193,6 +199,95 @@ public function formatInterval($interval, $granularity = 2, $langcode = NULL) {
   +   * @return string
   +   *   A translated string representation of the interval. The string will be
   +   *   prepended with a minus sign if timestamp1 time is after timestamp2.
   +   *
   

   Um... It doesn't seem like prepending with a - is definitely a good idea? Weren't we going to at least have some options here? I doubt that this is the best default.

4. +++ b/core/lib/Drupal/Core/Datetime/DateFormatter.php
   @@ -193,6 +199,95 @@ public function formatInterval($interval, $granularity = 2, $langcode = NULL) {
   +    // @todo handle 0
   

   What does this mean?

5. +++ b/core/lib/Drupal/Core/Datetime/DateFormatter.php
   @@ -193,6 +199,95 @@ public function formatInterval($interval, $granularity = 2, $langcode = NULL) {
   +            $output .= ($output ? ' ' : '') . $this->formatPlural($interval->y, '1 year', '@count years', array(), array('langcode' => $options['langcode']));
   

   Would this be easier if we did something like

   $thisoutput = $this->formatPlural...
   

   in the switch statement, and then just one

   $output .= ($output ? ' ' : '') . $thisoutput;
   

6. +++ b/core/modules/views/src/Plugin/views/field/Date.php
   @@ -151,24 +151,35 @@ public function render(ResultRow $values) {
              return ($time_diff < 0 ? '-' : '') . $this->dateFormatter->formatInterval(abs($time_diff), is_numeric($custom_format) ? $custom_format : 2);
   +
            case 'inverse time span':
              return ($time_diff > 0 ? '-' : '') . $this->dateFormatter->formatInterval(abs($time_diff), is_numeric($custom_format) ? $custom_format : 2);
   +
            case 'time span':
              return $this->t(($time_diff < 0 ? '%time hence' : '%time ago'), array('%time' => $this->dateFormatter->formatInterval(abs($time_diff), is_numeric($custom_format) ? $custom_format : 2)));
   +
   

   Why are we still using formatInterval for these cases?

RE #50 item 3: Presumably prefix/suffix are config somewhere, but we should document that these should be passed in translated.

So... I also took a look at this patch and I'm a bit confused/bothered by parts of it...

1. +++ b/core/lib/Drupal/Core/Datetime/DateFormatter.php
   @@ -193,6 +199,109 @@ public function formatInterval($interval, $granularity = 2, $langcode = NULL) {
   +   *   (optional) A UNIX timestamp, defining the to date and time. If NULL is
   +   *   passed, the current request time is assumed. Defaults to NULL.
   +   * @param array $options
   

   Wording is slightly awkward and inconcise here... can we change this to:

   ... to date and time. NULL (default) indicates the time of the current request.

2. +++ b/core/lib/Drupal/Core/Datetime/DateFormatter.php
   @@ -193,6 +199,109 @@ public function formatInterval($interval, $granularity = 2, $langcode = NULL) {
   +   *   - granularity: An integer value that signals how many different units to
   +   *   display in the string. Defaults to 2.
   +   *   - langcode: A string representing a language code. It is used to
   

   Second line here needs 2 spaces of indentation. Same in the next bullet point.

3. +++ b/core/lib/Drupal/Core/Datetime/DateFormatter.php
   @@ -193,6 +199,109 @@ public function formatInterval($interval, $granularity = 2, $langcode = NULL) {
   +   *   - prefix: A string that is prepended to the output.
   +   *   - suffix: A string that is appended to the output.
   +   *
   

   So we need to say "translated string"

4. +++ b/core/lib/Drupal/Core/Datetime/DateFormatter.php
   @@ -193,6 +199,109 @@ public function formatInterval($interval, $granularity = 2, $langcode = NULL) {
   +   *   - prefix: A string that is prepended to the output.
   +   *   - suffix: A string that is appended to the output.
   +   *
   

   These should also say the default is empty?

5. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampAgoFormatter.php
   @@ -87,7 +87,7 @@ public function viewElements(FieldItemListInterface $items) {
   -        $updated = $this->t('@time ago', array('@time' => $this->dateFormatter->formatInterval(REQUEST_TIME - $item->value)));
   +        $updated = $this->dateFormatter->formatDiff($_SERVER['REQUEST_TIME'] - $item->value, $_SERVER['REQUEST_TIME'], array('suffix' => 'ago'));
          }
   

   Um, really? I don't think this is right. Shouldn't this just have $item->value as the first arg and the second as NULL? I think the value here is a timestamp, right?

6. +++ b/core/modules/aggregator/src/Controller/AggregatorController.php
   @@ -127,8 +127,8 @@ public function adminOverview() {
   -      $row[] = ($last_checked ? $this->t('@time ago', array('@time' => $this->dateFormatter->formatInterval(REQUEST_TIME - $last_checked))) : $this->t('never'));
   -      $row[] = ($last_checked && $refresh_rate ? $this->t('%time left', array('%time' => $this->dateFormatter->formatInterval($last_checked + $refresh_rate - REQUEST_TIME))) : $this->t('never'));
   +      $row[] = ($last_checked ? $this->dateFormatter->formatDiff($_SERVER['REQUEST_TIME'] - $last_checked, $_SERVER['REQUEST_TIME'], array('suffix' => 'ago')) : $this->t('never'));
   +      $row[] = ($last_checked && $refresh_rate ? $this->dateFormatter->formatDiff($last_checked + $refresh_rate - $_SERVER['REQUEST_TIME'], $_SERVER['REQUEST_TIME'], array('suffix' => 'left')) : $this->t('never'));
          $links['edit'] = [
   

   Same here.

7. +++ b/core/modules/contact/src/MessageForm.php
   @@ -200,7 +200,7 @@ public function validate(array $form, FormStateInterface $form_state) {
            $form_state->setErrorByName('', $this->t('You cannot send more than %limit messages in @interval. Try again later.', array(
              '%limit' => $limit,
   -          '@interval' => $this->dateFormatter->formatInterval($interval),
   +          '@interval' => $this->dateFormatter->formatDiff($_SERVER['REQUEST_TIME'] + $interval),
            )));
   

   Here, it seems like it should just be formatDiff($interval) shouldn't it?

8. +++ b/core/modules/contact/src/MessageForm.php
   @@ -200,7 +200,7 @@ public function validate(array $form, FormStateInterface $form_state) {
   -          '@interval' => $this->dateFormatter->formatInterval($interval),
   +          '@interval' => $this->dateFormatter->formatDiff($_SERVER['REQUEST_TIME'] + $interval),
   
   +++ b/core/modules/system/src/Form/CronForm.php
   @@ -101,8 +101,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   -    $status = '<p>' . t('Last run: %cron-last ago.', array('%cron-last' => $this->dateFormatter->formatInterval(REQUEST_TIME - $this->state->get('system.cron_last')))) . '</p>';
   +    $status = '<p>' . $this->dateFormatter->formatDiff($this->state->get('system.cron_last'), $_SERVER['REQUEST_TIME'], array('prefix' => 'Last run:', 'suffix' => 'ago')) . '</p>';
   

   Might just pass in NULL here for second arg?

9. +++ b/core/modules/user/src/UserListBuilder.php
   @@ -151,10 +151,8 @@ public function buildRow(EntityInterface $entity) {
   +    $row['member_for'] = $this->dateFormatter->formatDiff($entity->getCreatedTime());
   +    $row['access'] = $entity->access ? $this->dateFormatter->formatDiff($entity->getLastAccessedTime(), REQUEST_TIME, array('suffix' => 'ago')) : t('never');
        return $row + parent::buildRow($entity);
   

   Same here. If we have this NULL capability, let's use it?

10. +++ b/core/modules/views/src/Plugin/views/cache/Time.php
    @@ -141,7 +141,7 @@ public function validateOptionsForm(&$form, FormStateInterface $form_state) {
    -    return $this->dateFormatter->formatInterval($results_lifespan, 1) . '/' . $this->dateFormatter->formatInterval($output_lifespan, 1);
    +    return $this->dateFormatter->formatDiff($results_lifespan, 1) . '/' . $this->dateFormatter->formatDiff($output_lifespan, 1);
    

    What is this 1 as second arg here? Seems wrong?

The rest of the changes have similar questions/issues. Am I misunderstanding something?

This is getting closer!

I think there are still a couple of problems:

1. +++ b/core/lib/Drupal/Core/Datetime/DateFormatter.php
   @@ -193,6 +199,112 @@ public function formatInterval($interval, $granularity = 2, $langcode = NULL) {
   +   * @return string
   +   *   A translated string representation of the interval.
   +   *
   

   I had to return up here to read the code when I was reviewing the rest of this patch, to figure out the polarity of "from" and "to" in the arguments. Maybe we should add something to this @return docs that explains the polarity, which I think is that if $2 is after $1, the numbers will be positive?

   And maybe the arguments should be called $from and $to rather than $timestamp1 and $timestamp2?

2. +++ b/core/lib/Drupal/Core/Datetime/DateFormatter.php
   @@ -193,6 +199,112 @@ public function formatInterval($interval, $granularity = 2, $langcode = NULL) {
   +          default:
   +            break;
   +        }
   

   Do we really need this? ;)

3. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampAgoFormatter.php
   @@ -87,7 +87,7 @@ public function viewElements(FieldItemListInterface $items) {
   -        $updated = $this->t('@time ago', array('@time' => $this->dateFormatter->formatInterval(REQUEST_TIME - $item->value)));
   +        $updated = $this->dateFormatter->formatDiff($item->value, NULL, array('suffix' => 'ago'));
          }
   

   Hm. I'm wondering about this now... There could potentially be languages where "@time ago" got translated as "foo @time bar" or even "foo @time", where "foo" and "bar" represent some foreign words/phrases. So maybe passing in a prefix/suffix in these cases is not the right thing to do? I think we should be calling formatDiff and then using it in $this->t() as in the old code instead.

   And similar for other areas of this patch.

4. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampAgoFormatter.php
   @@ -87,7 +87,7 @@ public function viewElements(FieldItemListInterface $items) {
   +        $updated = $this->dateFormatter->formatDiff($item->value, NULL, array('suffix' => 'ago'));
   

   I think the suffix needs to be t('ago'), unless we're doing t('@time ago') as noted above? Similar for other prefix/suffix except when it is -.

5. +++ b/core/modules/contact/src/MessageForm.php
   @@ -200,7 +200,7 @@ public function validate(array $form, FormStateInterface $form_state) {
            $form_state->setErrorByName('', $this->t('You cannot send more than %limit messages in @interval. Try again later.', array(
              '%limit' => $limit,
   -          '@interval' => $this->dateFormatter->formatInterval($interval),
   +          '@interval' => $this->dateFormatter->formatDiff($_SERVER['REQUEST_TIME'] + $interval),
            )));
   

   Maybe the correct way to call this would be formatDiff(0, $interval)?

6. +++ b/core/modules/contact/src/Tests/ContactSitewideTest.php
   @@ -231,7 +231,7 @@ function testSiteWideContact() {
   -    $this->assertRaw(t('You cannot send more than %number messages in @interval. Try again later.', array('%number' => $this->config('contact.settings')->get('flood.limit'), '@interval' => \Drupal::service('date.formatter')->formatInterval(600))));
   +    $this->assertRaw(t('You cannot send more than %number messages in @interval. Try again later.', array('%number' => $this->config('contact.settings')->get('flood.limit'), '@interval' => \Drupal::service('date.formatter')->formatDiff(REQUEST_TIME - 600))));
    
   

   Here actually this is a test. We should just pass in "5 minutes" as a literal string in the test, which is the expected result of the formatting?

7. +++ b/core/modules/views/src/Plugin/views/field/Date.php
   @@ -151,19 +151,38 @@ public function render(ResultRow $values) {
   +          $options = array(
   +            'granularity' => isset($this->options['granularity']) ? $this->options['granularity'] : 2
   +          );
   +          return $this->t('%time ago', array('%time' => $this->dateFormatter->formatDiff($value, NULL, $options)));
   +
   

   This section of code seems to be doing the right thing with t() and the prefix/suffix, in contrast to most of the rest.

8. +++ b/core/modules/views/src/Plugin/views/field/Date.php
   @@ -151,19 +151,38 @@ public function render(ResultRow $values) {
            case 'raw time hence':
   -          return $this->dateFormatter->formatInterval(-$time_diff, is_numeric($custom_format) ? $custom_format : 2);
   +          return $this->dateFormatter->formatDiff($value, NULL, array('granularity' => is_numeric($custom_format) ? $custom_format : 2));
   +
   

   Hm, does this work right? Seems like if time ago passes in $value, NULL, then time hence should pass in NULL, $value [assuming that NULL in param 1 means "get the current request time" -- the method doesn't do that currently but could/should?]

9. +++ b/core/modules/views/src/Plugin/views/field/Date.php
   @@ -151,19 +151,38 @@ public function render(ResultRow $values) {
   +          $options = array(
   +            'granularity' => is_numeric($custom_format) ? $custom_format : 2,
   +            'prefix' => ($time_diff < 0 ? '-' : ''),
   +          );
   +          return $this->dateFormatter->formatDiff(abs($time_diff), NULL, $options);
            case 'inverse time span':
   

   This doesn't look right still, see previous review.

See also #2399211-71: Support all options from views fields in DateTime formatters

Took a look at the latest patch and I think it's looking pretty good! A few comments - and I think we're getting down to the edge cases now...

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampAgoFormatter.php
   @@ -87,7 +87,7 @@ public function viewElements(FieldItemListInterface $items) {
   -        $updated = $this->t('@time ago', array('@time' => $this->dateFormatter->formatInterval(REQUEST_TIME - $item->value)));
   +        $updated = $this->t('@time ago', array('@time' => $this->dateFormatter->formatDiff($item->value)));
          }
   

   So... It's a bit interesting here, what happens for times that are in the future.

   formatInterval() would I think end up saying "0 sec" if $item->value was in the future.

   formatDiff() would end up saying something like "1 year ago" if the time was in the future.

   Hm....

2. +++ b/core/modules/aggregator/src/Controller/AggregatorController.php
   @@ -127,8 +127,8 @@ public function adminOverview() {
   -      $row[] = ($last_checked ? $this->t('@time ago', array('@time' => $this->dateFormatter->formatInterval(REQUEST_TIME - $last_checked))) : $this->t('never'));
   -      $row[] = ($last_checked && $refresh_rate ? $this->t('%time left', array('%time' => $this->dateFormatter->formatInterval($last_checked + $refresh_rate - REQUEST_TIME))) : $this->t('never'));
   +      $row[] = ($last_checked ? $this->t('@last_checked ago', array('@last_checked' => $this->dateFormatter->formatDiff($last_checked))) : $this->t('never'));
   +      $row[] = ($last_checked && $refresh_rate ? $this->t('@time_left left', array('@time_left' => $this->dateFormatter->formatDiff($last_checked + $refresh_rate))) : $this->t('never'));
          $links['edit'] = [
   

   Similar here. I think $last_checked is fine, but $last_checked + $refresh_rate could actually be in the future pretty easily, if it's time to check but cron hasn't run recently. So I think in that last line, it should check to see if $last_checked + $refresh_rate > current time, and if so it would output the "0" option.

   Given both of these ... maybe we need to have an option to our new formatDiff function that says "Treat $from > $to as 0", or something like that, so that we can capture the same behavior as formatInterval() and handle this case?

   Not sure...

3. +++ b/core/modules/views/src/Plugin/views/field/Date.php
   @@ -151,19 +151,35 @@ public function render(ResultRow $values) {
   +          return $this->t(($time_diff < 0 ? '%time hence' : '%time ago'), array('%time' => $this->dateFormatter->formatInterval($time_diff, NULL, $options)));
   

   This needs to be two separate t() calls, for proper picking up of the strings in t() for translation. So it should be:
   $time_diff < 0 ? t('%time hence', array()) : t('%time ago', array());

   Same a few lines above.

Good idea on the weeks, David! Wasn't aware of the other issue, sorry.

And one other thought I had. There is at least one place in this patch where we're passing in formatDiff($interval, 0) to represent "this number of seconds from now".

Due to the months issue (the reason for this in the first place), and even though I am pretty sure I was the one who suggested calling it that way in the first place, I do not think it is correct to do this. It will be fine if $interval is small, but if it goes over a day, due to time zones and months and stuff, calling
formatDiff($interval, 0)
is not really going to be the same as calling
formatDIff(NOW + $interval)

So... need to look at those too.

Yeah, so that's interesting [cross-posted with #67].

We should be using formatDiff() if the start/end times are known, and I guess we should be using formatInterval() if it's really a fixed interval that we're trying to represent, like "we want to run cron every 6000 seconds, represent that as a time span"?

[edit, added the rest]

As opposed to "represent the last time cron was run or the next time it will run as a time span", which is a formatDiff() for sure, since it's relative to the current time.

This is very close! A few nitpicks and a few substantive comments... and can I just say nice work and very thorough testing?

1. +++ b/core/lib/Drupal/Core/Datetime/DateFormatter.php
   @@ -193,6 +199,119 @@ public function formatInterval($interval, $granularity = 2, $langcode = NULL) {
   +   *   - strict: A boolean value indicating whether or not the from timestamp
   

   Nitpick: boolean => Boolean

2. +++ b/core/lib/Drupal/Core/Datetime/DateFormatter.php
   @@ -193,6 +199,119 @@ public function formatInterval($interval, $granularity = 2, $langcode = NULL) {
   +   *     can be after the to timestamp. If TRUE and from is after to, the
   +   *     result string will be "0 seconds". If FALSE and from is after to, the
   +   *     result string will be the formatted time difference. Defaults to FALSE.
   +   *
   

   I would say $from and $to rather than just "from" and "to" here?

3. +++ b/core/lib/Drupal/Core/Datetime/DateFormatter.php
   @@ -193,6 +199,119 @@ public function formatInterval($interval, $granularity = 2, $langcode = NULL) {
   +          case 'd':
   +            // \DateInterval doesn't support weeks, so we need to calculate them
   +            // ourselves.
   +            $interval_output = '';
   +            if ($interval->d > 6) {
   +              $interval_output .= $this->formatPlural(floor($interval->d / 7), '1 week', '@count weeks', array(), array('langcode' => $options['langcode']));
   +              $granularity--;
   +            }
   +            if ($granularity > 0 && $interval->d % 7 !== 0) {
   +              $interval_output .= ($interval_output ? ' ' : '') . $this->formatPlural($interval->d % 7, '1 day', '@count days', array(), array('langcode' => $options['langcode']));
   +            }
   +            break;
   

   I find code with % to be a bit difficult to read... This could be written a bit differently:

   $days = $interval->d;
   if ($days >= 7) {
     $weeks = floor($days / 7);
     // format the weeks
     $days -= $weeks * 7;
     $granularity--;
   }
   if ($granularity > 0 && $days > 0) {
   ...
   }
   

   I'm not strongly suggesting this though. Matter of preference. ;)

4. +++ b/core/modules/views/src/Plugin/views/cache/Time.php
   @@ -141,7 +141,7 @@ public function validateOptionsForm(&$form, FormStateInterface $form_state) {
   -    return $this->dateFormatter->formatInterval($results_lifespan, 1) . '/' . $this->dateFormatter->formatInterval($output_lifespan, 1);
   +    return $this->dateFormatter->formatDiff($results_lifespan) . '/' . $this->dateFormatter->formatDiff($output_lifespan);
      }
   

   Ah. The second arg to formatInterval is the granularity, so we'd better pass that into formatDiff to get the same output here? I finally checked what those "1" values were. ;)

5. +++ b/core/modules/views/src/Plugin/views/field/Date.php
   @@ -151,19 +151,41 @@ public function render(ResultRow $values) {
            case 'raw time ago':
   -          return $this->dateFormatter->formatInterval($time_diff, is_numeric($custom_format) ? $custom_format : 2);
   +          return $this->dateFormatter->formatDiff($value, NULL, array('granularity' => is_numeric($custom_format) ? $custom_format : 2));
   +
   

   This may be a case where we should use the 'strict' option?

6. 
   +++ b/core/modules/views/src/Plugin/views/field/Date.php
   @@ -151,19 +151,41 @@ public function render(ResultRow $values) {
            case 'time ago':
   -          return $this->dateFormatter->formatInterval(-$time_diff, is_numeric($custom_format) ? $custom_format : 2);
   +          return $this->dateFormatter->formatDiff($value, NULL, array('granularity' => is_numeric($custom_format) ? $custom_format : 2));
   +
   

   This may be another case where we should use the 'strict' option?

7. 
   +++ b/core/modules/views/src/Plugin/views/field/Date.php
   @@ -151,19 +151,41 @@ public function render(ResultRow $values) {
            case 'raw time hence':
   -          return $this->t('%time hence', array('%time' => $this->dateFormatter->formatInterval(-$time_diff, is_numeric($custom_format) ? $custom_format : 2)));
   +          return $this->t('%time hence', array('%time' => $this->dateFormatter->formatDiff($value, NULL, array('granularity' => is_numeric($custom_format) ? $custom_format : 2))));
   +
   

   here too?

8. +++ b/core/modules/views/src/Plugin/views/field/Date.php
   @@ -151,19 +151,41 @@ public function render(ResultRow $values) {
            case 'raw time span':
   -          return ($time_diff < 0 ? '-' : '') . $this->dateFormatter->formatInterval(abs($time_diff), is_numeric($custom_format) ? $custom_format : 2);
   +          $options = array(
   +            'granularity' => is_numeric($custom_format) ? $custom_format : 2,
   +          );
   +          return ($time_diff < 0 ? '-' : '') . $this->dateFormatter->formatDiff($time_diff, NULL, $options);
            case 'inverse time span':
   

   Hmmmm... I don't think we want to be pssing $time_diff into formatDiff? Shouldn't we be passing in $value like in the other cases? And this probably also needs 'strict'?

9. +++ b/core/modules/views/src/Plugin/views/field/Date.php
   @@ -151,19 +151,41 @@ public function render(ResultRow $values) {
            case 'inverse time span':
   -          return ($time_diff > 0 ? '-' : '') . $this->dateFormatter->formatInterval(abs($time_diff), is_numeric($custom_format) ? $custom_format : 2);
   +          $options = array(
   +            'granularity' => is_numeric($custom_format) ? $custom_format : 2,
   +          );
   +          return ($time_diff < 0 ? '-' : '') . $this->dateFormatter->formatDiff($time_diff, NULL, $options);
            case 'time span':
   

   Same here?

10. +++ b/core/modules/views/src/Plugin/views/field/Date.php
    @@ -151,19 +151,41 @@ public function render(ResultRow $values) {
    +          if ($time_diff < 0) {
    +            $time_span = $this->t('%time hence', array('%time' => $this->dateFormatter->formatInterval($time_diff, NULL, $options)));
    +          }
    +          else {
    +            $time_span = $this->t('%time ago', array('%time' => $this->dateFormatter->formatInterval($time_diff, NULL, $options)));
    +          }
    +          return $time_span;
             case 'custom':
    

    And here?

I don't think we need to worry too much about the magic number of "7 days makes a week". ;)

This is again looking pretty good but I still think there are some problems with the conversions of existing uses of formatInterval():

1. +++ b/core/modules/user/src/UserListBuilder.php
   --- a/core/modules/views/src/Plugin/views/cache/Time.php
   +++ b/core/modules/views/src/Plugin/views/cache/Time.php
   
   +++ b/core/modules/views/src/Plugin/views/cache/Time.php
   +++ b/core/modules/views/src/Plugin/views/cache/Time.php
   @@ -141,7 +141,7 @@ public function validateOptionsForm(&$form, FormStateInterface $form_state) {
   
   @@ -141,7 +141,7 @@ public function validateOptionsForm(&$form, FormStateInterface $form_state) {
      public function summaryTitle() {
        $results_lifespan = $this->getLifespan('results');
        $output_lifespan = $this->getLifespan('output');
   -    return $this->dateFormatter->formatInterval($results_lifespan, 1) . '/' . $this->dateFormatter->formatInterval($output_lifespan, 1);
   +    return $this->dateFormatter->formatDiff($results_lifespan) . '/' . $this->dateFormatter->formatDiff($output_lifespan, NULL, array('granularity' => 1));
      }
   

   Actually... This is not really a time difference. It's really a time interval. I don't think that converting it to formatDiff() is the right thing to do here at all. We should leave it as formatInterval(). It has a limited number of options as it's coming from an options form anyway.

2. +++ b/core/modules/views/src/Plugin/views/cache/Time.php
   @@ -141,7 +141,7 @@ public function validateOptionsForm(&$form, FormStateInterface $form_state) {
   -    return $this->dateFormatter->formatInterval($results_lifespan, 1) . '/' . $this->dateFormatter->formatInterval($output_lifespan, 1);
   +    return $this->dateFormatter->formatDiff($results_lifespan) . '/' . $this->dateFormatter->formatDiff($output_lifespan, NULL, array('granularity' => 1));
   

   The first formatDiff() still needs a granularity option. It only got added to the second one.

3. +++ b/core/modules/views/src/Plugin/views/field/Date.php
   @@ -151,19 +151,45 @@ public function render(ResultRow $values) {
            case 'raw time hence':
   -          return $this->dateFormatter->formatInterval(-$time_diff, is_numeric($custom_format) ? $custom_format : 2);
   +          return $this->dateFormatter->formatDiff($value, NULL, array('strict' => TRUE, 'granularity' => is_numeric($custom_format) ? $custom_format : 2));
   +
   

   Is this the right polarity? It's supposed to be time hence, meaning that $value is supposed to be in the future. So it seems like we need to call formatDiff(REQUEST, $value) with strict=TRUE?

4. +++ b/core/modules/views/src/Plugin/views/field/Date.php
   @@ -151,19 +151,45 @@ public function render(ResultRow $values) {
            case 'time hence':
   -          return $this->t('%time hence', array('%time' => $this->dateFormatter->formatInterval(-$time_diff, is_numeric($custom_format) ? $custom_format : 2)));
   +          return $this->t('%time hence', array('%time' => $this->dateFormatter->formatDiff($value, NULL, array('strict' => TRUE, 'granularity' => is_numeric($custom_format) ? $custom_format : 2))));
   +
   

   Same here as in raw time hence?

5. +++ b/core/modules/views/src/Plugin/views/field/Date.php
   @@ -151,19 +151,45 @@ public function render(ResultRow $values) {
            case 'raw time span':
   -          return ($time_diff < 0 ? '-' : '') . $this->dateFormatter->formatInterval(abs($time_diff), is_numeric($custom_format) ? $custom_format : 2);
   +          $options = array(
   +            'granularity' => is_numeric($custom_format) ? $custom_format : 2,
   +            'strict' => TRUE,
   +          );
   +          return ($time_diff < 0 ? '-' : '') . $this->dateFormatter->formatDiff($value, NULL, $options);
            case 'inverse time span':
   

   here I think we do not want strict, sorry if I said we did before? Because we are checking for the polarity of the time diff and then we want the span as output from formatDiff(), right?

6. +++ b/core/modules/views/src/Plugin/views/field/Date.php
   @@ -151,19 +151,45 @@ public function render(ResultRow $values) {
   +          if ($time_diff < 0) {
   +            $time_span = $this->t('%time hence', array('%time' => $this->dateFormatter->formatInterval($value, NULL, $options)));
   +          }
   +          else {
   

   Ooops, this is calling formatInterval with the formatDiff args, and then also for hence with strict it needs to be (request, $value) not ($value, NULL) right?

   Maybe we need to have the $from arg also accept NULL and be set to REQUEST_TIME?

I looked through this latest patch... I think it's all correct now. Phew! Thanks for all the hard work!

I am updating the issue summary to address the points in #85. I think this should still be RTBC.

The patch does, in fact, add documentation to formatInterval:

+   *   A translated string representation of the interval. Note that for
+   *   intervals over 30 days, the output is approximate: a "month" is always
+   *   exactly 30 days, and a "year" is always 365 days. It is not possible to
+   *   make a more exact representation, given that there is only one input in
+   *   seconds.
+   *
+   * @see \Drupal\Core\Datetime\DateFormatter::formatDiff()
    */

Really, what more do you want??????

As it stated in the issue summary already, we still need the old function because there are still a few places in Core where it's being used to format an interval that is not the difference between two specific dates. I've added a few more notes to the summary to hopefully clarify this... The bottom line is that the new function is for when there is a difference between two specific dates. The old function is for when there is only a number of seconds and it is not the difference between two specific dates, and if you use the old function, you should simply not rely on it being anything sensible if your interval is longer than 30 days, because it can't be. We've documented this in the patch already...

I'm at my wit's end in figuring out how to fix this so it will be satisfactory. I am once more setting this back to RTBC... sorry I am really not trying to be annoying and we've really tried to document everything correctly here... The code patch is good. The docs are good. The tests are good. We need the new function, and just about every spot in Core is changed to use it, except for a couple of places that do not have two dates to compare, which cannot use the new function.

Hopefully this will do it. If not... please really what more do we need to do???? I just don't get the objection here.

OK, good idea, setting back to needs work. Let's move that whole paragraph about when not to use this up into the main body of the docs instead of in the @return, and make sure it explicitly says "use this other thing instead".

OK. Yes. I thought I had checked this before, but you are right, there are still some uses of formatInterval that should be fixed that are not covered by this patch. Either they have crept in since the last time we checked, or we missed them the last time.

So, double Needs Work.

I could make the next patch but I think I would rather keep myself open for reviewing.
@pjonckiere, are you available? Or alternatively, @David_Rothstein, would you want to review if I make the changes?

OK I'll take it on.

Wow, I guess there were quite a few places that still needed fixing. Here's a new patch. I believe I have updated all uses of formatInterval() to formatDiff that should be updated now. The remaining ones are being used to create options lists, or are genuinely intervals where the start/end date does not appear to be known, or at least it is not obvious how to get it (such as in the Batch API, but hopefully that would not be running for months anyway!).

Test failure is due to a typo in the change I made in UserAdminListingTest at line 94. Here's a new patch. Only that one line in that one file changed.

Also downgrading to a normal bug because of objections raised above, and updating the issue summary a bit again.

Wow, thanks for the detailed and careful review! Much appreciated.

Here is a new patch... hopefully addressing all of your review points. A few notes -- everywhere else I agreed with what you said and hopefully made good changes:

item 1: I decided to remove the commented-out code, since our coding standards forbid this. Instead turned it into a proper @todo and filed an issue to track it: #2488844: Write tests for raw time span and time span formatters in Views

item 2: Fixed both functions' $langcode docs to be more precise.

item 3: I think I agree with you that "strict" should be the default, given that we've had formatInterval with that default for ages and ages. I also don't think it matters in 99% of the cases, because the usual use of this is "pass in a date in the past and format it as ... ago", for things like last time cron ran, node update time, last comment time, etc. But I agree that is how this new function should be. So, that's been updated. Most of the uses of the function didn't change though, since we were not too careful about strict anyway, and most of them were expected to be times in the past anyway.

item 4: Really, I think all the tests should change to test for a definite output string rather than testing output by comparing to another call to the function that is being tested, which seems a bit circular. I suggested it in comment #62 item 6... would prefer to leave it as it is. If you feel strongly about it, I'm willing to change it.

item 5: This is providing a sample formatted piece of output for the "since" date token type. I think that the previous code was weird/incorrect. The changed code will give us output of "10 min", which seems reasonable; the previous one would have done something like "30 years 10 days etc.", because it was passing in REQUEST_TIME in as an interval, which was just plain wrong. Obviously there was no test...

item 9: Excellent point. Went back and fixed all the time strings to be as similar as possible, using "@time" as the placeholder as much as possible for consistency.

item 10: The reason we did this was an earlier review comment that we want to avoid having REQUEST_TIME all over our code. So at least this way it is confined to this one function, for the most part. I think it should stay.

item 11: This is the same as item 3 and I agree. The change has been made in the new patch.

Thanks! I have carefully verified that the patch in #110 is a good reroll of the RTBC patch in #105, with the changes from #2428795: Translatable entity 'changed' timestamps are not working at all mentioned in #109 merged in correctly.

I'm not really excited about this new direction but OK. Please someone update the summary.

Ignore me, I'm feeling better about this direction today. I think I was just reacting like "Gee whiz, we were almost done let's just get it IN already" or something yesterday. Long day.

So, I took a look at the current patch. I disagree - if we're going this way, we need to have two wrapper functions, because of the 'strict' default for formatDiff. So you need to be able to pass in a date/time in the past or future and still use 'strict'. You can't do that with just one wrapper function... right?

Also, you need to look at the docs for the wrapper function. They are still referring to $from and $to and other stuff from formatDiff(), so they need to be fixed, so that it's clear whether they're expecting a time in the past or in the future, and so that they refer to the args of this function.

I took a careful look at the latest patch. Looking good! Thanks for all the work (again)!!!

I had a few minor suggestions on the API doc blocks, and one code question:

1. +++ b/core/lib/Drupal/Core/Datetime/DateFormatter.php
   @@ -116,8 +127,8 @@ public function __construct(EntityManagerInterface $entity_manager, LanguageMana
   +   *   (optional) Language code to translate to. NULL (default) means to use
   +   *   the user interface language for the page.
       *
   

   This is very clear. But...

2. +++ b/core/lib/Drupal/Core/Datetime/DateFormatter.php
   @@ -193,6 +212,179 @@ public function formatInterval($interval, $granularity = 2, $langcode = NULL) {
   +   *   - langcode: A string representing a language code. It is used to
   +   *     translate to a language other than what is used to display the page.
   +   *     Defaults to NULL, which results in the current user language being
   +   *     used.
   +   *   - strict: A Boolean value indicating whether or not the timestamp can be
   

   This is not so clear. There is not just one "language to display the page", there is sometimes a selected UI language and possibly a different Content language, and "user language" in the second sentence is definitely not right (implies a languge choice of the user). So we need to be clear here.

   It should probably either match the previous examples, or be changed to say:

   langcode: The language code for the language used to format the date. Defaults to NULL, which results in the user interface language for the page being used.

   This same wording change needs to be done in the other doc blocks too... they all should match.

3. +++ b/core/lib/Drupal/Core/Datetime/DateFormatter.php
   @@ -193,6 +212,179 @@ public function formatInterval($interval, $granularity = 2, $langcode = NULL) {
   +   * @see \Drupal\Core\Datetime\DateFormatter::formatDiff()
   +   */
   +  public function formatTimeDiffSince($timestamp, $options = array()) {
   +    $request_time = $this->requestStack->getCurrentRequest()->server->get('REQUEST_TIME');
   

   We should probably have @see links between all three of the formatDiff functions?

4. +++ b/core/modules/locale/locale.pages.inc
   @@ -119,6 +119,6 @@ function template_preprocess_locale_translation_update_info(array &$variables) {
   -  $variables['time'] = \Drupal::service('date.formatter')->formatInterval(REQUEST_TIME - $last);
   +  $variables['time'] = \Drupal::service('date.formatter')->formatTimeDiffSince($last);
      $variables['link'] = \Drupal::l(t('Check manually'), new Url('locale.check_translation', array(), array('query' => \Drupal::destination()->getAsArray())));
   

   I love how much more readable this is. ;)))))

5. +++ b/core/modules/views/src/Plugin/views/field/Date.php
   @@ -151,24 +151,33 @@ public function render(ResultRow $values) {
            case 'time hence':
   -          return $this->t('%time hence', array('%time' => $this->dateFormatter->formatInterval(-$time_diff, is_numeric($custom_format) ? $custom_format : 2)));
   +          return $this->t('%time hence', array('%time' => $this->dateFormatter->formatTimeDiffUntil($value, array('granularity' => is_numeric($custom_format) ? $custom_format : 2, 'strict' => FALsE))));
   +
   

   Remind me why we put strict=FALSE here? Seems like that's not right?

Note: #2488844: Write tests for raw time span and time span formatters in Views just got to RTBC and will conflict with this, because it (a) adds a bunch more formatInterval stuff and (b) is the issue about removing the @todo that was added to this patch in comment #105, so it changes those same lines.

So this may be a "race to commit" problem. Oh well, they're both good patches, this is not a terrible problem to have. ;)

Anyway adding this comment then I'll review...

[Sorry, edit: fixed issue link here]

Great -- I think the code and docs in the patch all look good now. Thanks!

Just to be sure nothing new was needed, I applied this patch and grepped through the code for remaining uses of formatInterval. I checked them all over:

a) Various form builders (and their tests) - used to create option lists for intervals for cron and things like that, example:

core/lib/Drupal/Core/Block/BlockBase.php:    $period = array_map(array(\Drupal::
service('date.formatter'), 'formatInterval'), array_combine($period, $period));

These are actually intervals, so we cannot convert them to use formatDiff or the wrappers.

b) core/modules/views/src/Plugin/views/field/TimeInterval.php -- this is a Views field handler that apparently takes a time interval in seconds and uses formatInterval to format it as an interval. As it has no start/end dates, needs to still use formatInterval. Who knows if this is actually being used anywhere anyway? But it's correct as it is.

c) Batch UI - formats the time elapsed for the request. This is OK to stay as a formatInterval and would require a rewrite of the Batch UI to change it anyway (unlikely to be longer than a few minutes in any case, so formatInterval is accurate there).

That's it, so that looks good too.

The change record looks good to me too. I don't know if we actually need change records for new API functions, but it cannot really hurt.

Let's ship this!

Thanks! I believe that Xano's comments in #141 have been addressed, so setting back to RTBC. Xano: feel free to disagree. ;)

Right, #2488844: Write tests for raw time span and time span formatters in Views got committed, so this needs some updates, as noted in comment #139. It will not be a straight reroll, because I think that patch added some more calls to formatInterval() that may need to be fixed up.

Do you want to make a quick check through the patch from the other issue and see if there are more formatInterval() calls to update, before I review this new patch?

#151 looks like the list of To Dos. I think we only needed to look at the latest commits related to this.

But after applying the new patch, looking at remaining formatInterval() calls would still be a good idea for the reviewer, to make sure they're only the ones listed in comment #140. So... I added that list in #140 to the issue summary, and updated it in general to make it even more clear. Hopefully.

Looking good! Mostly... :)

I also looked through remaining calls to formatInterval() and agree that this patch catches all of them that should be fixed.

But I'm still confused about a couple of the conversions in this patch, and/or think they should be different -- two to be exact:

1. +++ b/core/modules/aggregator/src/Controller/AggregatorController.php
   @@ -127,8 +127,8 @@ public function adminOverview() {
   -      $row[] = ($last_checked && $refresh_rate ? $this->t('%time left', array('%time' => $this->dateFormatter->formatInterval($last_checked + $refresh_rate - REQUEST_TIME))) : $this->t('never'));
   ...
   +      $row[] = ($last_checked && $refresh_rate ? $this->t('@time left', array('@time' => $this->dateFormatter->formatTimeDiffSince($last_checked + $refresh_rate))) : $this->t('never'));
          $links['edit'] = [
   

   This one for Aggregator checking "time left" seems wrong. I think it should be Until and not Since, because it's formatting "how long until the next expected check". Right?

2. +++ b/core/modules/system/system.tokens.inc
   @@ -68,7 +68,7 @@ function system_token_info() {
   -    'description' => t("A date in 'time-since' format. (%date)", array('%date' => \Drupal::service('date.formatter')->formatInterval(REQUEST_TIME - 360, 2))),
   +    'description' => t("A date in 'time-since' format. (%date)", array('%date' => \Drupal::service('date.formatter')->formatTimeDiffSince(REQUEST_TIME - 360))),
      );
   

   This one is still bothering me, every time I look at it, and I wonder if we should just leave it as formatInterval() but change the input to a more reasonable number of seconds? I think that was the intent, although the code is simply wrong to do REQUEST_TIME - 360 (which is formatting the time elapsed since some date in 1972 I guess, but maybe that was the actual intent?).

   So. The idea of this line is to show an example of what a "since" token for a date will look like, like "1 hour 30 minutes" or "1 year 2 months" or whatever. The original line here will be showing something like "30 years xxx". We decided in an earlier comment that they probably intended it to be "6 minutes" and got it wrong. Who knows, since there's not a code comment here. But in any case, I'm wondering if maybe we should just pick something relatively short like "1 hour 30 minutes" and leave this as formatInterval() to format the sample, to avoid REQUEST_TIME being introduced? So we would just leave this at formatInterval(3630) and add a code comment saying that this should output a sample of 1 hour 30 minutes.

   Thoughts?

Fair enough. Let's leave that as is then.

I believe this is ready to go, and the issue summary is updated. Hopefully we can get it in this time before we need another big change. ;)

FYI, another potential conflict with #2399211: Support all options from views fields in DateTime formatters. If that issue gets committed first, we'll need to look at its uses of formatInterval() and convert them as well. If this gets committed first, we should make a new patch for that other issue to convert its formatInterval() calls there.

Another one to watch: #2502571: Date format granularity should only render adjacent units. This is changing how formatInterval() deals with cases that would currently output stuff like "1 year 1 second" to just do "1 year" until it gets to "1 year 1 month". We'll need to do the same for formatDiff() if that is accepted; if this gets in first we'll need to add formatDiff() to the patch on that issue.

Yes, we have a lot of race conditions here. ;)