Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
We need custom access handling within ECA. Therefore we should provide events and actions that enable fine-granular control of entity and field access.
Steps to reproduce
Proposed resolution
Remaining tasks
User interface changes
API changes
Data model changes
Issue fork eca-3282649
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
mxhIt's not part of the critical list of issues that need to be fixed before stable 1.0, but turns out entity and field access is a crucial thing for our process requirements. Will try to address it soon, but can also move into 1.1.x if I don't get to this beforehand.
Comment #3
jurgenhaasRemoving it from the 1.0 roadmap as proposed above. But leaving it as major.
Comment #4
mxhMakes probably more sense to put into its own sub-module, as it can address content and config entities at the same time.
Comment #5
mxhComment #7
mxhComment #8
mxhI've renamed ContentEntityEventInterface into EntityEventInterface, as this one is actually not bound to content entities.
Comment #9
jurgenhaasVery interesting implementation! In a positive sense of course. I was confused at first but then started to understand what this actually does. Like it.
While reviewing the code, I found a couple of typos and code style warnings. Committed the fixes right to the MR, hope that's ok.
Set this to RTBC, only wonder about the 48 line code block in
\Drupal\eca_access\Plugin\ECA\Event\AccessEvent::lazyLoadingWildcardwhich is a duplication of what we already have in\Drupal\eca_form\Plugin\ECA\Event\FormEvent. Is it worth to optimize that? If not, we can still do that later of course.Comment #10
mxhAll good, thanks for fixing the typos.
Except I'm wondering why you removed the sentence from the AccessResult::allowed() call. I'd like to keep the sentence in there, i.e.
$result = AccessResult::allowed("Allowed by configured ECA");as this helps for debugging access results.
While the code block is rather large and very similar to the one of the form event, both events are not related to each other. I think it's not yet worth it to take care about an optimization at this place.
Comment #11
jurgenhaasBecause the PHPCS complained about it as this method does not take any arguments. The sentence should then be a comment, otherwise this could create strange results later on if the method declaration ever changed unnoticed.
Well, it's 48 (!!!) identical lines that were found by the IDE - I wouldn't be able to spot such a thing ;-) But I'm ok with optimizing that later.
This is so "funny": the cron event fixes from earlier today haven't been part of this MR yet and hence DrupalCI struggled with timezones again. I merged the latest changes from 1.0.x-dev, so all tests should then pass again.
Comment #12
jurgenhaasOh wow, there has been another place where the timezone kicked in:
\Drupal\Tests\eca_base\Unit\CronEventTest::appliesDataused thedatefunction to calculate the weekday but that function does not have any control over timezone and hence responded with Tuesday already, where it is still Monday. I've fixed that by calculating the weekday with the DateTime class like we do elsewhere as well.Comment #14
jurgenhaasMerged this now as the only outstanding question was about that code comment. If that still needs to be addressed, let's do so anyways.
Comment #15
mxhAaah, that's why. The
AccessResultclass is a bit confusing IMO, although it's supposed to be one of the best designed classes in Drupal core 😄 thanks for fixing.