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

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