Replace AccessResult::andIf() and ::orIf() with ::andIfCallable() and ::orIfCallable() to prevent code from needlessly running and adding cacheability

catch → credited mxr576 → .

+1. The syntax for orIf and andIf has always seemed overly verbose especially when you try to avoid the situation described above, this would make things much closer to actual PHP operators where code after || or && only runs when necessary.

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.

Appears to have already received feedback on MR.

Feedback addressed.

@mxr576 wonder your thoughts on the responses in the threads?

@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.

What is the reason we're introducing new methods? Especially since we want to remove andif and orIf. That means we expect all future code to be more verbose and redundant in being named after its argument type (which, unless there's multiple alternatives, is generally considered an anti-pattern).

Could we achieve the same thing through a deprecation dance which changes the parameter types but not the method names? orIf and andIf are both already statically typed to AccessResultInterface so we could first widen them to callable|AccessResultInterface, deprecate on a AccessResultInterface being thrown and then later dropping the AccessResultInterface type altogether to allow only callables. That leaves the final codebase still with the succinct orIf and andIf but with the desired callable type.

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?

That means we expect all future code to be more verbose and redundant in being named after its argument type

This is no longer the case, the methods are ::orAllowedIf() and ::andAllowedIf() in the MR, but the issue summary is out of date.

Moving to needs work for an issue summary update.

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

Thanks for the IS update!

Remaining tasks

1. Introduce new methods
2. Deprecate old methods
3. Start cleaning up core in a gazillion places
4. Make old methods internal

Well, remaining tasks should be scoped to this ticket, isn't it? From 2nd point they are potential follow up tasks to me that can be opened once these new APIs landed.

Speaking of new APIs, probably this should be added to release notes highlights.

Left a few comments on the... comments 😅 but generally speaking I like this idea a lot.

Updated the comments, thanks for the review

Seems there are some still open threads. Am I correct in that.

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.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

False positive

I'm very +1 on the general approach here, it will be both a performance and DX improvement.

It would be good to convert at least one core implementation of this so it's easier to see what a before/after looks like (will probably also help with writing the change record when we do that).

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 🐛 ResourceTestBase::getExpectedUnauthorizedAccessMessage() should be removed or replaced with something smarter Active .

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.