Access result cache contexts rule

[mglaman]

Feature request

With variation cache there are bugs being found due to multiple return logic steps and missing cacheable metadata in each return.

So a rule which finds methods which return access result interface. Check if multiple returns. That's the beginning of the code smell. Need to see if any cacheable metadata is lost.

Todo: flush out examples

[kristiaan-factorial]

Right, so probably the most common example is a tiered AccessResult return logic. What you'll often see is something like this:

if (!$account->hasPermission('do something special')) {
  return AccessResult::forbidden()->cachePerPermissions();
}

if ($account->id() == 1337) {
  return AccessResult::allowed()->cachePerUser();
}

return AccessResult::neutral();

The problem with the above is that if we get to the neutral outcome for a given user who does have the required permissions, but is not user ID 1337, it will be cached for everyone, including user 1337 or people who do not have the permission.

Likewise, if we cache for user 1337, we will always return said result for user 1337 even if their permissions change.

What instead needs to happen is that we carry over the cacheable metadata of every check, all the way down to the return statements that may follow:

$cacheable_metadata = (new CacheableMetadata())->addCacheContexts(['user.permissions]);
if (!$account->hasPermission('do something special')) {
  return AccessResult::forbidden()->addCacheableDependency($cacheable_metadata);
}

$cacheable_metadata->addCacheContexts(['user]);
if ($account->id() == 1337) {
  return AccessResult::allowed()->addCacheableDependency($cacheable_metadata);
}

return AccessResult::neutral()->addCacheableDependency($cacheable_metadata);

This happens with all sorts of cache contexts, though, not just the user based ones. So the goal would be to check if a function returns AccessResultInterface (even if not typehinted as such) and then start analyzing when it has multiple return statements and if every statement in a logic tree carries the cacheable metadata of the previous steps.

This might be a tough nut to crack, but if cracked it would make core and contrib already a lot safer.