ECA Access: Do not set AccessResult in applies()

Here's a patch.

Marking as postponed until I have a chance to create an MR against 2.0.x

We hit an issue where we had multiple events that use the "Determining Entity Access" plugin, and hit this issue. After reviewing, I think the solution proposed by @ergonlogic is incorrect -- I do think the result should get initialized with AccessResult::neutral(). However, if there is more than one of these events in your site, the last one wins because of this reset.

In our case two access checks applied to a particular action:

node:application, update
node:*, view,update

The first was setting an AccessResult::allowed, the second AccessResult::neutral. After processing the ECA event, AccessResult::neutral was the final result, and access ended up being denied.

I think the correct solution is to make use of AccessResult::orIf(). In the event plugin, setAccessResult() method, I think it should see if it already has an access result set, and if it does, replace it with $this->accessResult->orIf($result).

This allows all the access rules to get checked, and reasons are available for all of the models.

The logic for the orIf() call is to return AccessResult::forbidden if either the existing or new accessresult is forbidden; return AccessResult::allowed if either the existing or new result is allowed, and return AccessResult::neutral if both are neutral.

The previous solution is still subject to the "last check wins" if multiple checks return something other than neutral.

Proposed fix in MR.

In ECA 2.0, the initialisation with neutral has already been built in as part of 📌 Enable event plugins to determine appliance Fixed . It's now contained in \Drupal\eca_access\Plugin\ECA\Event\AccessEvent::appliesForWildcard. The problem with that is in fact, if multiple ECA events are subscribing to the same Drupal event, then each of them initializes the access result again.

But the proposed MR resolves that issue correctly. Thanks @freelock for that suggestion. I've just left a couple of comments that are simple to apply.

I've resolved the 2 open threads, please review once again so that we can then back port and fix this.

Lokks great!

Merged and back ported to 2.1.x

Automatically closed - issue fixed for 2 weeks with no activity.