Add an alter hook to EntityQuery

The Needs Review Queue Bot → tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Rerolled 3001496-15.patch to 10.1.x-dev.

interdiff_15_29.txt

Added accessCheck() to Drupal\KernelTests\Core\Entity\EntityQueryTest::testAlterHook

Will require a change record for the api change.

Change record added https://www.drupal.org/node/3363939 →

Seems to be a copy and paste of the description on this ticket. Leaving the tag.

I stumbled upon this issue through my RSS feed on the change records.
However it doesn't read like a CR.

I agree that it needs a different text and an example usage.

I'm sorry please. Can you give me an example of usage?

The change record for this issue has been published. It should still be a draft at this point, right?

Yes it should be unpublished

@elber more information on how to write a change record is here:
https://www.drupal.org/community/contributor-guide/task/write-a-change-r... →

I rewrote the change record → and used an example from the tests. I think it would be great to have a more illustrative example if anyone has any ideas.

New functions believe should be typehinted please.

Tagging for issue summary to have the standard issue template applied, especially the api section of the template.

There appear to be a few hooks being added can they all be listed in the CR please.

Ok, I updated the change record to list all hooks, and I updated the issue summary. I also made an MR of patch #32 to make it easier for myself to understand the changes.

Please review the new issue summary and change record.

Issue summary looks good.

Slightly tweaked the CR to include the full changes from the entity.api. May be overkill be figured it would save someone time from having to open the file.

Saved credit for ptmkenny for the issue summary update, cr changes,

Hiding patches as fix is in the MR

Ran the test only feature to verify the coverage

1) Drupal\KernelTests\Core\Entity\EntityQueryTest::testAlterHook
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
 Array &0 (
+    5 => '5'
     7 => '7'
     13 => '13'
     15 => '15'
 )
/builds/issue/drupal-3001496/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:121
/builds/issue/drupal-3001496/vendor/phpunit/phpunit/src/Framework/Constraint/IsIdentical.php:79
/builds/issue/drupal-3001496/core/tests/Drupal/KernelTests/Core/Entity/EntityQueryTest.php:751
/builds/issue/drupal-3001496/core/tests/Drupal/KernelTests/Core/Entity/EntityQueryTest.php:1313
/builds/issue/drupal-3001496/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
FAILURES!
Tests: 17, Assertions: 278, Failures: 1.
PHPUnit 9.6.13 by Sebastian Bergmann and contributors.
Testing Drupal\Tests\Core\Entity\Query\Sql\QueryTest

Also see that all 4 new hooks are being altered in the test module.

Believe this is good to go.

I'm triaging RTBC issues → . I read the IS, the comments, the change record. I didn't find any unanswered questions. After reading #18 I thought that this would be a won't fix but the next comment explains why this is useful.

However, I do not see any comments that discuss a code review nor are they any comments in the MR.

I then applied the diff but there are conflicts, so this needs work. I read the MR and left two comments. Also, new functions and methods should have return type hints. See https://www.drupal.org/docs/develop/standards/php/php-coding-standards#s... → .

I tried to improve the change record so it was not mostly a copy/paste of code. To do that I searched the change records for others where a hook was added and used those as a guide. It is not complete, so I am tagging for change record updates.

Just a bit of finalizing to do here.

Prashant.c → made their first commit to this issue’s fork.

left some comments.

Appears to have a merge conflict.

Also was tagged for CR updates which appear to still be needed.

I reviewed https://www.drupal.org/project/drupal/issues/1801726 → and it never actually existed and we didn't catch it in time despite the dozens of revisions I did and the dozens who reviewed it. The documentation clearly shows I initially wanted to add it but didn't.

I am sorry :(

I fixed the change record

I fixed the comments, but a rebase is still needed.

Thanks @prashant.

My last fixes were trivial, so I'm RTBC per #48.

Doh

Looking at how SQL queries are altered - it only triggers the alter hook if the query is tagged. See \Drupal\Core\Database\Query\Select::preExecute(). I'm wondering if we should do the same. Ie. change the code to

  protected function alter(): Query {
    if (isset($this->alterTags)) {
      $hooks = ['entity_query', 'entity_query_' . $this->getEntityTypeId()];
      foreach ($this->alterTags as $tag => $value) {
        $hooks[] = 'entity_query_' . $tag;
        $hooks[] = 'entity_query_' . $this->getEntityTypeId() . '_' . $tag;
      }
      \Drupal::moduleHandler()->alter($hooks, $this);
    }
    return $this;
  }

Hmmm are all entity queries tagged... ah interesting, is that some tags that you'd expect to be there like ENTITY_TYPE_ID_access are only added in \Drupal\Core\Entity\Query\Sql\Query::prepare() which is called after this. Unfortunately in the current design it does not seem possible for a hook implementation to know if \Drupal\Core\Entity\Query\QueryBase::$accessCheck is TRUE or not.

Also I'm wondering about the impact on the existing entity_query and entity_query_ENTITY_TYPE_ID tags. They feel somewhat superfluous and duplicates.

Another thought is in what order should the hooks be called. Obviously hook_entity_query_alter() first... but then I think it should be hook_entity_query_TAG_alter() then hook_entity_query_ENTITY_TYPE_alter() and then hook_entity_query_ENTITY_TYPE_TAG_alter()

Looking at how SQL queries are altered - it only triggers the alter hook if the query is tagged. See \Drupal\Core\Database\Query\Select::preExecute(). I'm wondering if we should do the same.

I'm not sure why we would do that. Mind you, I'm not sure why SQL queries do that. Any ideas why it would be good to do this?

Being able to alter absolutely any entity query would seem like a useful feature for modules like DER or workspace that want to globally enhance entity queries.

some tags that you'd expect to be there like ENTITY_TYPE_ID_access are only added in \Drupal\Core\Entity\Query\Sql\Query::prepare() which is called after this.

I think that might be good. In some sense these access tags seem like part of a deeper level of the API. If you want to mess with an entity query's access should do do it through accessCheck() which is it's API for the purpose; or you accept the greater responsibility of altering the Sql query.

Unfortunately in the current design it does not seem possible for a hook implementation to know if \Drupal\Core\Entity\Query\QueryBase::$accessCheck is TRUE or not.

Agreed. This is one symptom of the need identified in #2502363: Add getters to EntityQuery → . Once this issue is fixed, it will get more valuable to fix that one.

Also I'm wondering about the impact on the existing entity_query and entity_query_ENTITY_TYPE_ID tags. They feel somewhat superfluous and duplicates.

Maybe - although they allow a deeper level of sql modification than entity queries currently do. Shall I open a follow-up to explore deprecating them?

Another thought is in what order should the hooks be called. Obviously hook_entity_query_alter() first... but then I think it should be hook_entity_query_TAG_alter() then hook_entity_query_ENTITY_TYPE_alter() and then hook_entity_query_ENTITY_TYPE_TAG_alter()

Entity/Query/Sql/Query::prepare does this:

    $this->sqlQuery->addTag('entity_query');
    $this->sqlQuery->addTag('entity_query_' . $this->entityTypeId);

    // Add further tags added.
    if (isset($this->alterTags)) {
      foreach ($this->alterTags as $tag => $value) {
        $this->sqlQuery->addTag($tag);
      }
    }

Which means that sql queries alter hooks fire in the order ENTITY_TYPE then TAG (as is the current MR) not the TAG then ENTITY_TYPE you propose. I suggest we keep to the same pattern as sql queries unless we have a good reason to be different.

Re the last part of #60 it's not quite the same as sql's alter tags though... because you are adding the tag on to both entity_query and entity_query_ENTITY_TYPE_ID

Also if a tag and entity type ID clash your site will have problems and that means this needs work.

I think it needs to be something like

      $hooks = ['entity_query', 'entity_query_' . $this->getEntityTypeId()];
      foreach ($this->alterTags as $tag => $value) {
        $hooks[] = 'entity_query_tag_' . $tag;
        $hooks[] = 'entity_query_tag_' . $this->getEntityTypeId() . '_' . $tag;
      }

to avoid clashes.

I don't have a strong sense either way about the order.

Also if a tag and entity type ID clash your site will have problems and that means this needs work.

Good point. I'm a bit unsure about this one though:

$hooks[] = 'entity_query_tag_' . $this->getEntityTypeId() . '_' . $tag;

Strictly _tag_ is not needed here to avoid collision but I can see that it might be more consistent to have it.

I wondered if this is better:
$hooks[] = 'entity_query_' . $this->getEntityTypeId() . '_tag_' . $tag;

I'd like is to to decide the hook name structure before I change the code as it will need changing in quite a few places.

The problem with your suggestion is that it still allows for clashes consider a site the following entity type names:
two
two_tag_alpha

If there's a query with the tag alpha there will be a clash. Yes this is very contrived but someone somewhere will have a set up like this. This is why there are no hooks with SOMETHING_hardcode_SOMETHING. It's really hard to come up with something that will not clash. Also this is why we should move away from hooks and to something like hux → .

The spellchecking CI seems to be broken ... but I think this is right.

MR is showing as unmergable. The spell-check has been fixed a week or two ago, branch needs to be rebased with latest 11.x

Thanks @smustgrave, that sorted it.

Believe feedback has been addressed

Sorry, but this only affects SQL-based entity types. I think entity queries for remote entities (like the ones in the Apigee Edge module → ) should also be supported (and the entity.api.php changes also suggest this).

We can't change an implementation that's not in core? But I do agree that this needs to be clarified/extended, and implementation that is in core are config entity queries.

Unfortunately entity query classes do not inherit their execute() method from the abstract class QueryBase, so there's no way to guarantee all entity queries have this.

Added support for config entity queries and test coverage for that, documented in CR and entity.api.php the limitations on which entity queries have these.

Thanks @berdir for thinking of config entities.

For the 2 open threads from @amateescu

Can you rebase the MR appears to have a unit failure.

I have rebased the MR, needs review. Thanks!

Thanks @manish-31. Tests are now green. This is still NR for the changes made in response to #70, but otherwise RTBC per #68.

Applied some nitpicky missing :void returns.

Appears all feedback has been addressed though.

Added suggestion to MR to resolve possible hook clashes suing double underscores. Note we use this technique elsewhere in core. See \Drupal\Core\Plugin\FilteredPluginManagerTrait::getFilteredDefinitions and theme hooks.

Implemented double underscores.

Although

$hooks[] = "plugin_filter_{$type}";
$hooks[] = "plugin_filter_{$type}__{$consumer}";

does uses only 1 double underscore, I agree with @alexpott that we should do

$hooks[] = 'entity_query_tag__' . $tag;
$hooks[] = 'entity_query_tag__' . $this->getEntityTypeId() . '__' . $tag;

The reason I can see is that 'tag_something' is a possible entity type id.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Theme suggestion update seems good.

Have always seen the double underscore but didn't consider it was the standard, sorry!

Updated change record and issue summary for the new hook patterns.

Adding issue credit for reviews, comments that helped shape the MR and writing the change record.

The release note could do with finessing into something that can be added to something like https://www.drupal.org/project/drupal/releases/10.2.0 → - the current release note is not fit for that.

Committed c536598 and pushed to 11.x. Thanks!
Committed 19ae323 and pushed to 10.3.x. Thanks!

Improved release snippet in IS

This introduced a mock method that is deprecated in PHPUnit 10, can a follow up be opened to fix it.

	'message' => '#^Call to deprecated method returnValue\\(\\) of class PHPUnit\\\\Framework\\\\TestCase\\:
Use \\<code\\>\\$double\\-\\>willReturn\\(\\)\\</code\\> instead of \\<code\\>\\$double\\-\\>will\\(\\$this\\-\\>returnValue\\(\\)\\)\\</code\\>$#',
	'count' => 1,
	'path' => __DIR__ . '/tests/Drupal/Tests/Core/Entity/Query/Sql/QueryTest.php',

📌 Fix remaining return*() methods of class PHPUnit\Framework\TestCase deprecated in PHPUnit 10 Fixed

@mondrake sure open a follow-up but I don't think we should re-open this issue. PHPUnit 10 is less important than our our code. This should remain fixed.

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