QueryAccessEvent currently is used as such:

    // Allow other modules to modify the conditions before they are used.
    $event = new QueryAccessEvent($conditions, $operation, $account);
    $this->eventDispatcher->dispatch('entity.query_access.' . $this->entityType->id(), $event);

I'd like it to support generic listeners too, much like the entire entity system does for everything else:

  • hook_entity_insert vs hook_ENTITY_TYPE_insert
  • hook_entity_update vs hook_ENTITY_TYPE_update
  • etc.

Some modules may need to alter queries depending on how they are configured and can therefore not know beforehand which entity types they want to listen to. It would be cool if we could fire a generic event first and make sure we can read the entity type ID from QueryAccessEvent.

    // Allow other modules to modify the conditions before they are used.
    $event = new QueryAccessEvent($conditions, $operation, $account, this->entityType->id());
    $this->eventDispatcher->dispatch('entity.query_access, $event);
    $this->eventDispatcher->dispatch('entity.query_access.' . $this->entityType->id(), $event);
CommentFileSizeAuthor
#12 entity-3134363-12.patch6.09 KBkristiaanvandeneynde
#12 interdiff-9-12.txt1.35 KBkristiaanvandeneynde
#9 entity-3134363-9.patch6.36 KBkristiaanvandeneynde
#7 entity-3134363-7.patch6.32 KBkristiaanvandeneynde
#5 entity-3134363-5.patch6.13 KBkristiaanvandeneynde
#2 entity-3134363-2.patch6 KBkristiaanvandeneynde
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kristiaanvandeneynde created an issue. See original summary.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Factorial GmbH for ANNAI commented
Status: Active » Needs review
FileSize
entity-3134363-2.patch6 KB

Made it so the entity type ID can be NULL for now for BC reasons, but it does already call a trigger_error.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Factorial GmbH for ANNAI commented 
+++ b/src/QueryAccess/QueryAccessEvent.php
@@ -44,11 +53,19 @@ class QueryAccessEvent extends Event {
+      @trigger_error('The $entity_type_id argument must be passed to QueryAccessEvent::__construct(), it is required before Entity API 8.1.1. See https://www.drupal.org/node/3134363.', E_USER_DEPRECATED);

Obviously you can change this part as you see fit.

Status: Needs review » Needs work

The last submitted patch, 2: entity-3134363-2.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Factorial GmbH for ANNAI commented
Status: Needs work » Needs review
FileSize
entity-3134363-5.patch6.13 KB

Let's make sure the new test method only handles the thing it focuses on and does not interfere with other tests.

Status: Needs review » Needs work

The last submitted patch, 5: entity-3134363-5.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Factorial GmbH for ANNAI commented
Status: Needs work » Needs review
FileSize
entity-3134363-7.patch6.32 KB

Now if we could refrain from breaking the test we just wrote...

Status: Needs review » Needs work

The last submitted patch, 7: entity-3134363-7.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Factorial GmbH for ANNAI commented
Status: Needs work » Needs review
FileSize
entity-3134363-9.patch6.36 KB

Fixed the tests leaking into one another.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs review » Needs work 
+++ b/src/QueryAccess/QueryAccessEvent.php
@@ -44,11 +53,19 @@ class QueryAccessEvent extends Event {
     $this->account = $account;
+    if (!isset($entity_type_id)) {
+      @trigger_error('The $entity_type_id argument must be passed to QueryAccessEvent::__construct(), it is required before Entity API 8.1.1. See https://www.drupal.org/node/3134363.', E_USER_DEPRECATED);
+    }
+    else {
+      $this->entityTypeId = $entity_type_id;
+    }
   }

8.1.1 isn't really a version and depending on how you interpret it, it would introduce a change in either a minor or a patch version.

I'd actually be OK with just requiring it. See #2960643-27: Cannot load entity by uuid after rename for a similar case. The new method is a string return and code will expect it to be there, so it would break there.

Same as in the core issue, I'd argue the constructor here isn't an API and code outside of entity module isn't meant to reuse it.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Factorial GmbH for ANNAI commented

Okay fine by me. I use the constructor directly in Group, but that's really an edge case where the base access handler both does too little in one area and way too much in another for the special use case that is Group. 99% of all modules will want to use the base query access handler.

I'll adjust the patch right away.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Factorial GmbH for ANNAI commented
Status: Needs work » Needs review
FileSize
interdiff-9-12.txt1.35 KB
entity-3134363-12.patch6.09 KB
Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

Oops, crosspost:

I'm also OK with keeping it, but then we need to figure out something useful for the deprecation message. At least we now know that next major version is going to be 2.0.0 ( a year ago, we didn't know if it would be 8.x-2.x or 9.x-1.x), so I'm also OK if we put that in there. Also, I think the new guidelines would be "entity:2.0.0" then, similar to how core now uses "drupal:9.0.0".

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Factorial GmbH for ANNAI commented

I think very few modules use this system already and those that do (including Commerce) use the stock query access handler as a base. So Group is the only module I know of that builds the event itself.

If you want to err on the safe side, go with patch from #9 and adjust the "Entity API 8.1.1" to say "entity:2.0.0". If you think the risk is indeed minimal, go with #12.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs review » Fixed

Ok, #9 with the changed message it is. Unlike the core issue, the new argument is for the new generic case, so someone doing something with the existing event (class) might still work. And it doesn't hurt. Updated the version.

Status: Fixed » Closed (fixed)

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