The cache for user last access and login timestamps doesn't invalidate

This should fix the issue, can you try?

But keep in mind that the "last access" time is refreshing NO often than 180 sec (3 mins). You can lower the value by adding this in settings.php:

// This updates 'last access' time every 5 secs.
$settings['session_write_interval'] = 5;

For testing purposes you can do this.

While it's a caching issue I would like to pass this issue to @Wim Leers. Also a question for Wim: Shouldn't we trigger the cache invalidation [Cache::invalidateTags()] inside the entity internal cache invalidation? I mean when $this->resetCache(array($account->id())) is called, subsequently, the internal cache mechanism can also call Cache::invalidateTags(), instead of:

  public function postSave(EntityStorageInterface $storage, $update = TRUE) {
    $this->invalidateTagsOnSave($update);
  }

Fixed title, summary, version. and yes, needs tests.

Definitely, cacheability it's not an easy topic :)

So I guess that #4 is wrong because will invalidate the user cache too often. That's why I think this case fits better to auto-placeholdering. Here's a experimental patch but take into consideration following notes:

• The new 'user_timestamp' is still cached. It doesn't work as expected.
• The patch is missing an upgrade path for user entity definition and a post_update to change the formatters for user 'access' and 'login' base fields in all entity view displays and all views.
• There's a workaround in \Drupal\Core\Render\Renderer. Without that it doesn't work. I don't understand how to debug that. Is that a bug in Renderer?

A side-note: I think formatters and widgets should receive (maybe from their $this->fieldDefinition?) a flag that tells formatter if to build normally or lazy the output. For example, the definition for user 'access' base field could be:

  public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
    ...
    $fields['access'] = BaseFieldDefinition::create('user_timestamp')
      ->setLabel(t('Last access'))
      ...
      ->setRenderLazy(TRUE);
    ...
    return $fields;
  }

Then the same formatter can handle both cases depending on this bool without needing for a new formatter.

Fixed more points, added upgrade path (with test).

The IS should reflect #10.

Fixed IS. Fixed tests.

This seems like it's a duplicate of #2500525: Time ago/hence date/time formatting breaks caching; needs appropriate max age.

Sounds like that indeed, with the formatter being fixed, we would not need those workarounds in the first place.

Thanks for confirming. @claudiu.cristea, could you please review the patch on the other issue? You having worked on this patch makes you one of the best people possible to review the other patch :)

@Wim Leers, @dawehner, I don't think they are the same issue. In this case the 'access' and 'login' values of user are cached forever (because their update doesn't pass trough entity save). The other patch seems to be the opposite. Here's not only about "time ago" (aka interval calculated on current time). I would review the other but I want first that we decide if these are the same issue.

Even if this isn't a duplicate, I don't think the approach of adding a new formatter is correct (the same issue will come up lots pf places timestamps). It seems like a too specific solution to a generic problem. On the other patch, we are going down the road of calculating the cache-max age based on what the timeago will present back to the user. Another option I contemplated was whether we actually need a "request_time" cache context, which I think it the correct solution there (and yes, it would kill performance but would guarantee correct data being rendered).

In this particular case, why not just allow log in/user creation to indeed invalidate the user entity's cache tag? That'd be the more consistent/simpler solution, wouldn't it?

It seems like these are simply wrong:

  /**
   * {@inheritdoc}
   */
  public function updateLastLoginTimestamp(UserInterface $account) {
    $this->database->update('users_field_data')
      ->fields(array('login' => $account->getLastLoginTime()))
      ->condition('uid', $account->id())
      ->execute();
    // Ensure that the entity cache is cleared.
    $this->resetCache(array($account->id()));
  }

  /**
   * {@inheritdoc}
   */
  public function updateLastAccessTimestamp(AccountInterface $account, $timestamp) {
    $this->database->update('users_field_data')
      ->fields(array(
        'access' => $timestamp,
      ))
      ->condition('uid', $account->id())
      ->execute();
    // Ensure that the entity cache is cleared.
    $this->resetCache(array($account->id()));
  }

Logging in definitely doesn't happen very often. Access does. But:

  public function onKernelTerminate(PostResponseEvent $event) {
    if ($this->account->isAuthenticated() && REQUEST_TIME - $this->account->getLastAccessedTime() > Settings::get('session_write_interval', 180)) {
      // Do that no more than once per 180 seconds.
      /** @var \Drupal\user\UserStorageInterface $storage */
      $storage = $this->entityManager->getStorage('user');
      $storage->updateLastAccessTimestamp($this->account, REQUEST_TIME);
    }
  }

It'll happen at most every 180 seconds.

Not sure at all, just trying to make sure we properly discuss that option, because it'd be by far the simplest.

#19 sounds like the proper thing to do, and is one of the whole points of why we introduced cache contexts (so we don't need to manually muck around with invalidating caches).

I'm trying to understand what needs to be done here. At least comment #19sounds reasonable to me. But what would that mean for the patch in #13?

Also I doubt if we really need a user_timestamp formatter. If this formatter solves something, can we be sure it is only useful in the context of a user? If not, let's name it after what it does, not what it was created for initially.

I talked with @claudiu.cristea about this at the Drupal Dev Days. We concluded that the first patch from #4 is exactly (or pretty close to) the solution that is proposed in #19... Or did we miss something, @WimLeers?

The patch from #4 does not apply anymore so it would need a re-roll.

Here's a re-roll of the patch in #4. It works fine as far as I can see (I mainly tested with the "Member for" timestamp on user/[uid]).

That said I'm still not sure if it's the best solution. It seems wrong to invalidate a cache tag when the data itself didn't change, but only the representation of that data is stale. Thoughts?

I was specifically referring to logging in and creating users being infrequent actions. For those use cases, invalidating cache tags seems acceptable. So IMO that's a good solution for the "login timestamp" mentioned in the issue title.

For the "last access timestamp", I'd use https://www.drupal.org/developing/api/8/cache/max-age — just specify a max-age that matches: $build['#cache']['max-age'] = Settings::get('session_write_interval', 180);. This will ensure that the last access timestamp is at most 3 minutes old… which is fine, since it's also updated at most every 3 minutes. Worst case, this means it will be 5 minutes and 59 seconds old.

This view is not designed to give a real-time dashboard. It's meant to administer users, and get a sense of which users have never logged in, have not logged in since months, etc. So somewhat stale data seems fine to me.

Of course, the ideal would be to never have stale data for any of this. But AFAIK Views retrieves the last access timestamp in an actual DB query. Which means that it's not possible to cache all fields in each row except that one, and have just that one be queried when displaying the view. That may be the ideal.

IMO this issue needs feedback/guidance from a Views maintainer. I've given my opinion as a de facto cache system maintainer.

I think we should open a separate issue to look at moving these fields out of base fields into a separate service. If we were adding them now I don't think we'd have them in the base table at all.

edit: here it is #3299948: Move access and login fields out of the user entity.