Introduce AccessResult::andAllowedIf(), orAllowedIf(), andForbiddenIf() and orForbiddenIf() to eventually replace orIf() and andIf()

This will need a bit more love. If we simply use andIfClosure and orIfClosure, we can only avoid calling the closure if the original access result was forbidden. Any other scenario requires comparing the current to the other result, even if we may end up not using the other result. In the current proposal we'd still have to call the other closure to do some inspection.

So perhaps we need something that can signal intent. The example code in the IS could also not call the callable when the original result is N, because it knows the outcome of the closure cannot be F and therefore has no impact.

Starting to think we need:

• andAllowedIf(\Closure $closure)
• andForbiddenIf(\Closure $closure)
• orAllowedIf(\Closure $closure)
• orForbiddenIf(\Closure $closure)

Perhaps also andAllowedIfHasPermission(string $permission) and orAllowedIfHasPermission(string $permission);

Then the IS would look like:

$result = AccessResult::allowedIfHasPermission($account, 'access user profiles')->andAllowedIf(function($cacheable_metadata) use ($entity) {
  $cacheable_metadata->addCacheableDependency($entity);
  return $entity->isActive();
});

All of these would internally do the sanity checks where the other code does not have to run and only then yield to andIf() and orIf() with the result of the callable. This leaves andIf() and orIf() intact whereas the original thought was to get rid of them, so maybe we need to revisit that or make the original methods protected or private.

Pushed a proof of concept. Keep in mind that these helper functions slightly differ from how andIf() and orIf() behave internally.

• In the original logic, if the original result is uncacheable, then the other result is checked for cacheability and merged if it could turn an uncacheable result into a cacheable one.
• In the new logic, we simply do not run the extra code or add the extra cacheability if the original result will not be affected by the closure result.

The whole point was not to run dead code or add dead cacheable metadata. The trade off being that we may end up not trying to make uncacheable results cacheable, which I assume is an edge case that would occur far less than the current situation where cacheable metadata is needlessly added.

Feedback addressed.

@smustgrave we had another round of feedback. I am on the fence about two changes (predicate name and a more explicit param documentation.

Once @mxr576 and I agree on the final open issue, we can apply the 3 suggestions and this can be considered accepted and we can add some cases to AccessResultTest

Okay we are in agreement now. So if a committer could review, we can add tests when the approach is approved.

I need to update the IS to reflect the latest MR, but the explanation still stands:

AccessResult::andIf() and ::orIf() each have possible combinations that make the passed in access result obsolete. This leads to whatever was used to build the second access result to run for no reason at all. In case of chained calls, it may even lead to the second check's cacheability making it into the result even though the second check never applied.

Could we achieve the same thing through a deprecation dance which changes the parameter types but not the method names?
You mean check if the passed in argument is of type Closure and then run the new code, otherwise run the old code with a deprecation warning?

Yeah I can see where the confusion is coming from. Wrote a quick update.

Updated the comments, thanks for the review

One is bike-shedding about a variable name: Closure vs Predicate. We agreed to let the committers have the final say. The other two threads I just resolved as one was explained by the issue summary and the other both @mxr576 and I agreed to keep as is, as it makes more sense that way.

So I'd say it's ready for review as all threads are either resolved or resolvable by a committer.

Okay so I added two types of examples and discovered a few things while doing so.

First of all, by not adding the new methods to the AccessResultInterface, my IDE complained that calling them is potentially polymorphic as the test class UncacheableTestAccessResult does not have the new methods. So either we add them using the 1:1 rule or we leave it as is but face issues when people have their own versions of AccessResult.

As to the examples:

1. The simple scenario is looking for orIf(AccessResult::allowedIfHasPermission()) like I did in EntityFormDisplayAccessControlHandler, EntityViewDisplayAccessControlHandler and BaseFieldOverrideAccessControlHandler. These can be directly replaced by the new code.
2. The advanced case is as I did in CKEditor5MediaController. Here, the old code would run both checks either way, but the entity access check could return any of A, N or F. So our new methods can't be called here because we don't know which one to call. By hoisting the uncertain one to the top and then adding the certain one conditionally, we were still able to achieve a performance gain because now we only add the dependency if it makes a difference.

Point .2 led to another interesting bit. We could actually still benefit from an orIfCallable() and andIfCallable() method for cases where two unknowns (A, N or F) are combined because in both andIf() and orIf()'s case, nothing will happen if the original one is F.

So a combination of 2 entity access checks can still be optimized when the first check returns F.

Thinking about this further, I am really not a fan of the concept of orIf and andIf and any possible variation thereof in general. It will always lead to too much code being ran if implemented incorrectly. In #25.2 I demonstrated how some code needs to be refactored, but that code would be far more performant if written this way:

    $filters = $editor->getFilterFormat()->filters();
    if ($filters->has('media_embed') && $filters->get('media_embed')->status) {
      return $media->access('view', $this->currentUser, TRUE)->addCacheableDependency($editor->getFilterFormat());
    }
    return AccessResult::neutral()->addCacheableDependency($editor->getFilterFormat());

So the old code would run both checks and always add the dependency. The new code in the MR always runs the entity access check (the most expensive part) but only conditionally adds the dependency.

The above code, however, always adds the cheap dependency, but only runs the expensive entity access check if need be. This is still the best possible way of doing this, but not achievable with any of the four methods introduced in this issue due to the fact that an entity access check can return any of A, N or F.

Another bit to rant about, but maybe this should become an issue of its own. From FieldUpdateActionBase:

  /**
   * {@inheritdoc}
   */
  public function access($object, ?AccountInterface $account = NULL, $return_as_object = FALSE) {
    /** @var \Drupal\Core\Access\AccessResultInterface $result */
    $result = $object->access('update', $account, TRUE);

    foreach ($this->getFieldsToUpdate() as $field => $value) {
      $result->andIf($object->{$field}->access('edit', $account, TRUE));
    }

    return $return_as_object ? $result : $result->isAllowed();
  }

If the very first check returns Forbidden, this will still run ALL the other checks even if it can never affect the outcome. Better code would not run the loop if the initial result is F and break the loop it the compound result became F during the loop. Core and contrib are littered with these types of poorly performing access results and I feel it's partly because we gave them the andIf() and orIf() methods, making it so developers don't seem to know the impact of what's going on.

It's like the entity query accessCheck() method story where we ended up forcing it being set to make developers aware of something. We should consider doing something similar here.

Tests are failing on jsonapi (again...) because its ResourceTestBase wrongfully assumes that an access denied will always have the same reason:

  /**
   * Return the expected error message.
   *
   * @param string $method
   *   The HTTP method (GET, POST, PATCH, DELETE).
   *
   * @return string
   *   The error string.
   */
  protected function getExpectedUnauthorizedAccessMessage($method) {
    $permission = $this->entity->getEntityType()->getAdminPermission();
    if ($permission !== FALSE) {
      return "The '{$permission}' permission is required.";
    }

    return NULL;
  }

With our changes we can now return an access denied for entity view displays without having to run the manual permission check. But jsonapi tests are too rigid for that. Not sure how to fix this as it probably requires a significant rewrite of jsonapi tests.

Opened up #3494703: ResourceTestBase::getExpectedUnauthorizedAccessMessage() should be removed or replaced with something smarter.

I really don't have the energy right now to fix these test failures, maybe someone else can take over. Getting blocked by jsonapi tests is becoming a recurring theme and it's really demoralizing.

Would be nice if you had a readme with examples in there. It's quite a lot of code to read through. Either way, the goal here is to add to AccessResult so that we can slowly move from one approach to another. Call it a band-aid.

The real question is indeed whether we need to completely rethink AccessResult as a whole, but this issue is not for that.

I think we should merge changes in this issue that already improves the situation and think about how to slowly kill and replace AccessResult in Drupal 14/15.

Exactly. Make it "less worse" now, make it "great" (i.e. dead?) later.