Time ago/hence date/time formatting breaks caching; needs appropriate max age

The cache max age does not have to be 0, it could be as much as the minimum granularity supported by the formatter. i.e. if we said 'less than a minute ago' vs. '2 minutes ago' and never say exact seconds, then a one minute max age is fine.

Or thinking about it more, given cache tags, we could possibly even calculate the maximum age based on the minimum rendered granularity. So if we're going to say 'one hour ago' or 'one day ago' - we can have a max age of hours rather than minutes. If it's 'one year ago' the max age can be weeks or months. We only need the item to expire when what gets rendered will change, and it changes less frequently the more time passes. If necessary we can calculate the number of seconds until the next change and set it to this exactly.

I didn't look at code yet to see how complex it would be, but it may be worth implementing like that to avoid the js dependency. Drupal.org for example formats things rendered to anonymous users as time ago like the maintainers block on project pages, and we've been trying to stick to no javascript served to anonymous users.

Just in case there is a jQuery plugin for that http://timeago.yarp.com/ but may be a vanilla JS solution would be better ?

I also used Moment.js in the past on non-Drupal projects, and that's an excellent library http://momentjs.com/

But sometime, a simple function is enough https://stackoverflow.com/questions/3177836/how-to-format-time-since-xxx...

If we actually must fix this before release, it should be critical. However, I'm not quite understanding why. I get that without fixing this, we will kill the page cache for a few pages, but once the JS equivalent is available, no BC break happens, just pages get faster. So why is this a release blocker, versus something that could be done in a minor/point release?

I also don't think this is a release blocker. #2467071: [meta] Find, fix & finish cache tag support: try to find broken scenarios as the anonymous user is not critical. This is not critical.

The IS links to my comment at #2467071-21: [meta] Find, fix & finish cache tag support: try to find broken scenarios as the anonymous user — I indeed said "must be fixed" but I should've said "should be fixed". Posted #2467071-26: [meta] Find, fix & finish cache tag support: try to find broken scenarios as the anonymous user to clarify that.

Gotcha, thanks. :) Correcting the issue summary here as well.

I think setting max age based on the granularity in the field formatter setting should be a pretty easy thing to do, at least conceptually. The exact algorithm may take some thought, but the all of the parts for the plumbing area already there. I'll fix once #2399211: Support all options from views fields in DateTime formatters and #2455125: Update EntityViewsData use of generic timestamp to use Field API formatter land, and whether we know whether #2456521: Add DateFormatter::formatDiff() as a non-buggy alternative to DateFormatter::formatInterval() when the start and end of the interval are known is going in or not (as it may influence the algorithm for determining max age).

Hmm that's interesting. I've never noticed "1 hour 1 second ago" - because I assume this only holds for one minute, then it stays at minutes for the next 59...

I don't think the 1 hour 1 second ago is very useful though. If the component is set to 2, we should show 1 year one month, but not 1 year 1 day or 1 year 1 second.

However, even if we have to support that, we could also do a hybrid approach:

When the max-age is going to be less than one hour, set it to min($page_cache_max_age, 3600) and add javascript. Over one hour, no javascript.

Then anonymous users without javascript could get stale date formats for up to the max age, but everyone else gets 100% accuracy. But this still means js for anonymous users so less than ideal.

That's hard to explain in text, but untested patch with what I think we'd need to do.

Probably want to split this off, but this ought to work.

Opened #2502571: Date format granularity should only render adjacent units. Since it blocks a non-js solution to this issue I think it's OK for beta evaluation.

#2502571: Date format granularity should only render adjacent units is in, which means we can implement the decaying max-age solution here if we want to.

Marked #2600422: The "Member for" output for users must be marked uncacheable. as duplicate. Applying the 'RC target triage' tag over here.

I slightly updated the issue summary, but it could still use more tidying up.

For rc target, so we want to break this up into three steps/issues.

1. Solve the immediate problem, and set max-age==0 where it's needed. That will address the immediate problem with the cache lifetimes (albeit with a sledgehammer), and shouldn't be too hard (decent number of places to change, but pretty easy). The timeago test coverage should be able to be expanded to check this w/o much effort easier. This should be adequate for RC/release.
2. Figure out an algorithm for max age. I would argue that it doesn't need to be exact, as we aren't displaying exact values to begin with. I think half the value of the requested granularity unit should be fine. I would suggest adding DateFormatter::getDiffMaxAge($granularity) to abstract this (not sure about adding a second optional parameter for $interval to lock down the API for potential further tweaks, and I admit that I would have to review the formatDiff code again to remember exactly how we landed on the implementation).
3. Add JS behavior to these to update the field values while a page is being looked at.

I'm against JS callbacks for this, other than to update a page after it has been rendered.

If we are OK w/ this approach, I can get going on this.

As a side note, I was wondering yesterday if we need an explicit 'request_time' cache context that would end up setting the max-age to 0, rather than just setting max-age to 0. Seems to make more sense to me from a semantic standpoint even though the result is the same.

A fix like "Add max-age=0 for now" needs Perf Testing before commit IMO. Its easy to cause a critical regression with that approach.

Yes I think that's worse than the date formatting being stale.

OK, I'm going to jump in and start with plumbing in max-age into the render arrays w/ a new method, DateFormatter::getDiffMaxAge (or something similarly named), and figure out a first pass at an algorithm.

Thanks @mpdonadio! I'll provide reviews :)

Not sure if I caught everything, but it is kinda easier to just calculate the proper max-age everywhere. Still need to do the Date plugin for views, and I need to look at the views changes again.

The solution b) which is part of the patch seems to be the right first step. Adding the javascript could be properly implemented in 8.1

@mpdonadio Do you plan to work on this issue?

My time is spotty for the near term (unassigning myself), and I totally forgot I had started this. I think the patch is just about there; the fails are probably b/c a missed injection of the request stack somewhere.

#23 looks how I'd expect as well.

Let's fix the test failures ...

This is pretty epic, see screenshot:

A thought that I had while talking with @dawehner on IRC was that I think the timeago formatters need to return a render array, with the #cache entry in it. This would eliminate some duplicate code, and also allow precise cache ages (I think my logic is flawed in place on the ages). In the places where only the formatted value is used outside of a render context (I think just the RDF module), the #markup value can just be referenced.

Tests are passing. Could we update IS with the selected approach?

@moshe weitzman
Well, as you can read in #31 the current logic has quite some flaws so we better restart and try to think about the problem again.

Here is an idea for a different approach. This allows you to return an object which also has the max-age calculated. I hope just taking the smallest granularity + 1 of that unit is a good approach.

#33 looks clear to me, and fixes the bug. If we go this way, i would appreciate a warning on the form that alerts admins - Selecting very granular measures (e.g. 1 second, 1 minute) negatively impacts the cacheability.

The IS discusses a javascript solution but the comments don't say if/why that has been discarded. It would be so much more cacheable if PHP always emitted a timestamp and this formatter changed the timestamp via JS. This is the approach that Github and many others take. Furthermore, user's would no longer choose a granularity - the JS just ups the text as the page ages in the browser. This would fix many other places in the code where we use 'time ago' format. Just grep for 'time ago' to see them.

The issue summary says something about it, though:

For simplicity (we want to fix a bug which exists in 8.0.x) we went with Option 2 and experiment with Option 1 in further issues.

Skipped the examples in admin forms so far.

This is applying the new idea to a couple of places, but we still have to figure out how to deal with

This should fix already something.

Here with some more unit test coverage.

Looks great :)

1. +++ b/core/lib/Drupal/Core/Datetime/DateFormatter.php
   @@ -226,6 +231,7 @@ public function formatDiff($from, $to, $options = array()) {
   +    $max_age = 1e99;
   

   Nice, I didn't know this was possible :)

2. +++ b/core/lib/Drupal/Core/Datetime/FormattedDateDiff.php
   @@ -0,0 +1,77 @@
   +   * For example when the time difference is 1 day 1 hour, it can be cached
   +   * until now + 1 hour, so its 3600 seconds.
   

   English could use some refining.

3. +++ b/core/lib/Drupal/Core/Datetime/FormattedDateDiff.php
   @@ -0,0 +1,77 @@
   +   *   A date range until the string cannot be longer cached.
   

   It's not a range, it's a duration.

4. +++ b/core/lib/Drupal/Core/Datetime/FormattedDateDiff.php
   @@ -0,0 +1,77 @@
   +  public function toRenderable() {
   

   I think this means it could be replaced with a JS-based "time ago" formatter without breaking BC?

5. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampAgoFormatter.php
   @@ -176,19 +179,30 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
   +      $build = [
   +          '#markup' => SafeMarkup::format($this->getSetting('past_format'), ['@interval' => $result->getString()]),
   +      ];
   

   4 spaces -> 2 spaces

6. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampAgoFormatter.php
   @@ -176,19 +179,30 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
   +    CacheableMetadata::createFromObject($result)->applyTo($build);
   
   +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeTimeAgoFormatter.php
   @@ -176,20 +177,32 @@ public function settingsSummary() {
   +    CacheableMetadata::createFromObject($result)
   +      ->applyTo($build);
   
   +++ b/core/modules/user/src/UserListBuilder.php
   @@ -151,8 +152,20 @@ public function buildRow(EntityInterface $entity) {
   +      CacheableMetadata::createFromObject($last_access)
   +        ->applyTo($row['access']['data']);
   

   Inconsistent formatting.

7. +++ b/core/modules/views/src/Plugin/views/field/TimeInterval.php
   @@ -87,6 +87,7 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
   +    // @fixme
   

   Is this just a "todo" or is this case much more complex?

I think this means it could be replaced with a JS-based "time ago" formatter without breaking BC?

As said on IRC, this has to be some kind of setting on the formatter in order to not break rendered REST views.

4 spaces -> 2 spaces

WOW

Is this just a "todo" or is this case much more complex?

Oh well, actually, we don't have to do anything. An interval doesn't change depending on the current request. Its just a number

It looks like #2650768: The cache for user last access and login timestamps doesn't invalidate may be a duplicate of this.

@Wim Leers, it's not a duplicate of #2650768: The cache for user last access and login timestamps doesn't invalidate but will fix half of that, more exactly the "timestamp_ago" part. The other issue has problems also when using the simple timestamp formatter, not in relation with current time.

IMO, the solution here would be (inspired from #2650768: The cache for user last access and login timestamps doesn't invalidate) change the \Drupal\Core\Field\Plugin\Field\FieldFormatter\TimestampAgoFormatter formatter to lazy build the value. Assuming that this formatter is used only on entity (base or configurable) fields we just prepare this list of arguments for the lazy placeholder callback: entity_type_id, entity_id, field_name, langcode & formatter settings. Then, in lazy builder callback, we re-load the field value (that should be cheap because is always in static cache) and re-format it.

Before reviewing, we need a plan. Does this sound like a plan?

           case 'm':
             $interval_output = $this->formatPlural($interval->m, '1 month', '@count months', array(), array('langcode' => $options['langcode']));
+            $max_age = min($max_age, 30*86400);
             break;

What about February? Shouldn't we pick 28*86400 as the max age as a safe value?

IMO, the solution here would be (inspired from #2650768: The cache for user last access and login timestamps doesn't invalidate) change the \Drupal\Core\Field\Plugin\Field\FieldFormatter\TimestampAgoFormatter formatter to lazy build the value.

Why do you think the solution provided here would not work? It allows to cache those things and not require, as your issue, to rerender everything all the time ...

What about February? Shouldn't we pick 28*86400 as the max age as a safe value?

That is a good point, well yeah, why not.

It allows to cache those things and not require, as your issue, to rerender everything all the time ...

Exactly. See #2650768-19: The cache for user last access and login timestamps doesn't invalidate.

So can we proceed with the patch in #41? It works, is used on production and also has some form of test coverage.

30 days is what we use elsewhere in core to represent the average month (eg, DateFormatter), so I am fine with that. The timeago formatters are approximate anyway.

I like this approach much better than what I had started, especially that is pushes the change into the formatter itself and not where the formatter is used, which future proof additions to core, and will also improve contrib.

1. +++ b/core/lib/Drupal/Core/Datetime/FormattedDateDiff.php
   @@ -0,0 +1,77 @@
   +  /**
   +   * A date duration until the string cannot longer be cached.
   +   *
   +   * Let's say the time difference is 1 day 1 hour. In this case, we can cache
   +   * it until now + 1 hour, so maxAge is REQUEST_TIME + 3600 seconds.
   +   *
   +   * @var int
   +   */
   +  protected $maxAge;
   

   I think this comment is misleading/confusing. CacheableDependencyInterface describes max-age as

   The maximum time in seconds that this object may be cached.

   I think the short description can be "The maximum time in seconds that this string may be cached." and the long description can be "... so maxAge is 3600 seconds."

2. +++ b/core/lib/Drupal/Core/Datetime/FormattedDateDiff.php
   @@ -0,0 +1,77 @@
   +   * @param int $max_age
   +   *   A date duration until the string cannot be longer cached.
   +   */
   

   Similarly, I think "The maximum time in seconds that this string may be cached." is better here to be consistent with other core docs.

Otherwise, this is ready IMHO.

Ok, I think the doc change is good, and I like where this patch went. All of my original code is gone, so I think setting RBTC is OK here.

This looks excellent :)

This looks great. I worked on a very early sketch of the patch, but don't think there's much of my code left.

Since this changes render arrays, adds a new class etc, I'm initially committing it to 8.1.x only (also it's 8.0.3 release day today so it wouldn't be able to go in until after that anyway).

If you strongly feel it should be backported to 8.0.x then please re-open and we can figure out how doable that is with the bc policy.

Committed/pushed to 8.1.x, thanks!

And no JS :)

Though we listed this as a Major, I think just keeping this in 8.1 is fine. Considering the timing of 8.1, I would rather focus efforts on the other datetime things we are trying to get in for then.

Well, I can just say that this is super confusing for actual content editors, so maybe we should priotize it as bit?

Given the timing of 8.1 (beta is Real Soon Now, and release is a few weeks), is it worth is to figure out how to backport this w/o a BC break?

This introduced a bug: #2732441: Time ago formatter throws notices.

Hi All,

I found this issue (on its 3-year anniversary, coincidentally) because I encountered this "bug" in Drupal 8.5.3. Changing the view with a "time ago" field from Caching: Tag based to Caching: None fixed my issue. Is that the recommended solution? I see patches in this issue, but since Drupal is *way* past the versions referenced here, I assume it would either have been incorporated in core, or abandoned if there was some problem with it.