- Issue created by @Alezu
- Merge request !537Issue #3533724 by jurgenhaas: Condition entity type and bundle needs to check for empty type → (Open) created by Alezu
- 🇩🇪Germany jurgenhaas Gottmadingen
Please work on the 3.0.x branch, and we'll then backport from there.
Also, when this gets implemented, it should be done in a way that it works for all events that are available for access control.
- 🇩🇪Germany mxh Offenburg
This won't really help. Once forbidden, always forbidden thanks to 📌 ECA Access: Do not set AccessResult in applies() Postponed .
- 🇩🇪Germany jurgenhaas Gottmadingen
It will help as discussed in https://drupal.slack.com/archives/C0287U62CSG/p1751989605459709
The idea is that the event can be configured with a default which will be used only if the actions following it won't set any access result. That way, the "forbidden" in the event would only be set, if the following actions didn't set any other decision.
- 🇩🇪Germany mxh Offenburg
What if you have one ECA config (weight 0) that sets access forbidden for anything by default, and another ECA config (weight 1) that sets access to allowed based on some conditions - will it work?
- 🇩🇪Germany jurgenhaas Gottmadingen
Each of those events will be able to set an access result. How the access control handler deals with that when it receives one after the other, depends upon the implementation of that handler.
- 🇩🇪Germany mxh Offenburg
For ECA, one event is being instantiated per access call. That means, when having multiple ECA configurations reacting upon the same access event, they set the access result on the same object.
https://git.drupalcode.org/project/eca/-/blob/3.0.x/modules/access/src/H...
Before 📌 ECA Access: Do not set AccessResult in applies() Postponed it was possible as was described in #8 and you could also just set a default access on certain conditions. It was a mechanism we have used intentionally in hundreds of configurations and we heavily rely on it. Now it's not possible anymore.
- 🇩🇪Germany mxh Offenburg
@alezu thanks for creating a merge request. Is it ready for being reviewed?
- 🇩🇪Germany jurgenhaas Gottmadingen
I stand corrected, you're right @mxh. The event is instantiated once for a specific entity, but multiple ECA models can respond to that.
In that case, the implementation of the optional default in the configured event in the ECA model should follow the same logic as what we currently have with the
\Drupal\eca_access\Event\EntityAccess::setAccessResult
implementation: if the event defines a default (which is not mandatory), and no result is being set yet whengetAccessResult()
is being called, then the default should be applied in the same way as currently insetAccessResult
.Unless I'm missing something, that should allow for the required flexibility. As for the example in #8, the ECA event which gets triggered first, doesn't set a default, but the second one does. That means, the access denied used as default in the second event will only be applied if nothing has been set at the very end of the access check.
- 🇷🇺Russia Alezu
@mxh, I didn't test it enough and also I see that one UNIT test failed but visually it works as expected.
- 🇩🇪Germany mxh Offenburg
Besides I doubt this approach actually works out to restore what was possible before: I don't think it's a good idea that an event is not only an event (an entrypoint) but suddenly influences the outcome / result that is usually the purpose of an action. I think this would then be the first time, that an event takes over the role of a component that actually changes something. So this will add even more complexity for configurators. It was kind of straight forward that you just had to check for the "Set access result" actions in the process configurations for what is the supposed outcome in the end. Now you'd need to calculate the actual outcome in a weird way that is hard to explain, using these truth-tables.
The truth-tables of Drupal's access systems do makes sense - on a level of sub-systems. For example, the Group module will return an access result, and another access module (for example ECA) will return another access result. Combining the received results using the truth-table makes sense. But it doesn't really make sense on the level of ECA process configurations.
If you'd want to make actual use of such a truth-table on the level of ECA process configurations, you could allow it, but it should be explicit. Example: The "Set access result" would provide an option how you want to set the result - either using a truth-table for OR concatenation, for AND concatenation, or overwrite (= the way it was before). But this approach is now nearly impossible to achieve since that change in the linked issue.
For clarification, my example of #8 is a representation to achieve the following use case: "Let's deny access by default, also for the case anything goes wrong. And then optionally open up the gates based on conditions" As mentioned, this is currently not possible anymore, also not with the idea described in #12.
- 🇩🇪Germany mxh Offenburg
@alezu ok, once you've finished the merge request and is supposed to be reviewed, please feel free to set the status of this issue to Needs review. Thanks!