"bool $return_as_object" needs to go the way of the dodo

Relatedly, the value of the render array #access property and the return value of the render array #access_callback property can be booleans or AccessInterface objects. It's probably worth considering whether those should be enforced to be access objects as well.

Relatedly, the value of the render array #access property

This is pretty handy in form alters to just not show a particular form element on a form, so not sure we should prevent people setting it to FALSE.

> This is pretty handy in form alters to just not show a particular form element on a form, so not sure we should prevent people setting it to FALSE.

Well, AccessResult::forbidden() is not that much more work. But yes, #access is often just used as a plain not visible flag and not really something access related. But it very much is an issue, yes.

> How is this a thing? We should get rid of $return_as_object altogether and if people really care about only the outcome, they can still call $result->isAllowed() instead.

For context, the reason it is like this is BC. This was introduced late in D8 development phase and it wasn't feasible to break all existing code using those API's.

> This is the exact same type of situation as entity queries' access flag where we want people to actively consider the security implications of what they're doing.

FWIW, I don't really see it as the same thing. entity query forces you to make a decision, $entity->access()->isAllowed() doesn't force you to make a decision or do anything with that it just requires to type out an extra method call. Also, some API's that have such a condition parameters around cacheability do sometimes explicitly expose that cacheability into the global/current state, for example \Drupal\Core\Utility\Token::doReplace(). So making this required might actually have a negative effect on cacheability bugs. Pretty sure access doesn't do that though. One reason for that is the account parameter, because due to that, you never know if it's really/always for the current user or not. There are issues open about that problem.

For context, the reason it is like this is BC. This was introduced late in D8 development phase and it wasn't feasible to break all existing code using those API's.

Yeah I'm aware of the history. I could have phrased the IS as "How is this still a thing", I suppose.

FWIW, I don't really see it as the same thing. entity query forces you to make a decision, $entity->access()->isAllowed() doesn't force you to make a decision or do anything with that it just requires to type out an extra method call.

I disagree here. As long as we allow people to throw out cacheable metadata by using the boolean return value, we are letting them live in perfect ignorance of what their access check really meant.

If we start forcing everyone to use AccessResultInterface instead, it will make people aware of what they're dealing with and hopefully that will lower the barrier for people to properly add their access checks' cacheability to whatever they're building.

You could still get away with just calling ->isAllowed(), sure, but at that point you'd at least should be aware that you're dealing with a value object which can do more than just tell you whether access is granted. So at that point it would then be more fair to say the developer was aware and failed to add the right cacheable metadata. Right now the default $return_as_object is FALSE, so people don't even see that parameter and might not know they could be asking for a proper value object instead.

One reason for that is the account parameter, because due to that, you never know if it's really/always for the current user or not. There are issues open about that problem.

Yeah, I know. This is one of them: 🐛 Access result caching per user(.permissions) does not check for correct user Needs work

That shouldn't stop us from fixing all the places where currently cacheable metadata is thrown in the garbage, though. As we are moving pieces to placeholders, these issues are popping up left and right and we should try to put an end to them in bulk. Deprecating a boolean return value from access checks would go a long way already.

Perhaps it would put more pressure on us to finally fix the issue regarding non-current-user access checking. We have one in AccessPolicyProcessor using the account switcher, but that's only possible because said processor's internal cacheable metadata will never bubble up and we know exactly which user we're dealing with. This was by design as I was honestly annoyed with how tightly coupled our access checks are to the user cache contexts (and therefore current user).