Access denied returns need cache metadata

Actually, we don't need cache metadata added in this method.

Here's my reasoning.

HelpAccessControlHandler extends the default EntityAccessControlHandler.

It only overrides the protected checkAccess() method. So, access should always be checked via the public access() method on the default EntityAccessControlHandler, which will call checkAccess() if it doesn't already deny access some other way. Here's the access() method:
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21...

So, if you look at this... you can see that after the call to checkAccess(), it does this:

// Also execute the default access check except when the access result is
  // already forbidden, as in that case, it can not be anything else.
  if (!$return->isForbidden()) {
    $return = $return->orIf($this->checkAccess($entity, $operation, $account));
  }
  $result = $this->setCache($return, $cid, $operation, $langcode, $account);

So, this method takes care of doing the cache stuff.

I think so anyway... Any thoughts, especially @andypost?

Hm... well I'm not so sure now, maybe we do need to add metadata for the entity. ???

I looked at the 32 method that say they override the checkAccess method:
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21...

The most relevant ones, it seems to me, would be checking access for config entities. So I looked at the ones for date formats, aggregator feeds, etc.

Most of them don't seem to be doing any cacheable metadata stuff. One exception is
https://api.drupal.org/api/drupal/core%21modules%21system%21src%21DateFo...

 if ($entity->isLocked()) {
      return AccessResult::forbidden('The DateFormat config entity is locked.')->addCacheableDependency($entity);
    }
    else {
      return parent::checkAccess($entity, $operation, $account)->addCacheableDependency($entity);
    }

So it looks like if the access check does something on the entity, we should add metadata for the entity. Makes sense!

For lines that just check permissions, if you call AccessResult::allowedIfHasPermission(), that already takes care of adding metadata for the permission stuff, it looks like, so I think we don't need to worry about that.

So, yes, I think we should add entity metadata to the entity operations, and also for subsequent ones in the method after we've checked stuff on the entity. Working on a patch, and tests...

Here's a patch for review, with new tests added... all tests pass locally...

RE #5 -- thanks for the review! But I'm not sure about that. The lines above only returned an access result in some cases. If we got to the lines of code you highlighted, then we (a) checked something on the entity above and (b) didn't return yet. So the fact that we're returning this result depends on something on the entity not passing the previous if() statement. Right?

Hmmm... that doesn't mean the parent is correct, or correct in this case...

These lines are what is in question:

    if ($operation == 'unlock' || $operation == 'lock') {
-      return AccessResult::allowedIfHasPermission($account, 'administer help topic locking');
+      return AccessResult::allowedIfHasPermission($account, 'administer help topic locking')->addCacheableDependency($entity);
     }

You're right that the code here doesn't do any checks on the entity. However, if we get to these lines and execute them, we've already checked a few lines up whether the entity is locked or unlocked, but not returned because of what the operation is.

So if the entity lock state changes, and we don't have entity metadata saying "If the entity changes, we need to verify this access permission again", then we might incorrectly think we don't need to recalculate access, when we need to do so.

Right? Or am I not understanding what ->addCacheableDependency($entity) is supposed to guard against?

Great, thanks for all the reviews! I'll go ahead and commit this and update the Core patch.