Add an alter hook to EntityQuery

+1 to the idea. Can you post a patch?

Adding a hook is very simple. But on its own its of modest value, because entity queries are black boxes, you can put things into them but you can't see what's inside them; they have setters but not getters.

For example, while normal queries implement ConditionInterface and have a conditions() method that returns the conditions array, entity query does not.

Perhaps entity query could implement ConditionInterface, or perhaps it could have a bunch of getters like getConditions(), getSort(), getRange(), etc.

My bad, there's an issue for that:
#2502363: Add getters to EntityQuery

Something like this.

Fix lint.

Add tagged hooks too.

Missing a use statement, also omitted 2 hook invocations from field_test.module

We now need to mock the module handler in entity query unit tests.

Test results show 17 coding standards need fixing

Fixed coding standards

@jonathanshaw thanks for reviving the issue and the patch.

Probably not the kind of review you were looking for 😞 but I think there is a case for not adding this alter.

1. In #2611638: hook_entity_query_alter() is dead, remove it from entity.api.php when the doc was removed it was commented that nobody had noticed in 3 years.
2. This issue went 2 years without a patch after it was posted.
3. The issue only has 10 followers(not all would necessarily be in favor of adding the alter)
4. There is contrib module that does this https://www.drupal.org/project/entity_query_alter. The module has no usage and there has only 1(non-bot) issue in the last 2 years. The corresponding github repo also has no issues, stars or forks. Never the less it does prove you can do the alter with a change to core. I think of another way with decorating the query service.
5. chx who added this original hook in d7 says it is no longer needed in d8 for the purpose it was added in d7.
6. Any time we had an alter it is going to make it more complex for people to figure out all the things that can change the query.
7. We are adding extra maintenance burden for the alter code, the tests and the docs for feature that seems very edge case given the facts above.

I have been trying to think of ways we could make Drupal less complicated. I think not adding alter hooks when there is not a clear use case or even a case that it would be widely used is one way we can not add more complexity.

@bojanz thanks for the extra info. I didn't realize you were solving this Entity API and it has so much usage.

Yes I would agree that it does show it is more needed than my #18. I disregard #18 unless somebody else thinks it makes a difference. I think now #19 shows the importance

Successfully tested patch in #17. Makes it far more easier & elegant to alter entity queries, I welcome this :)

#3250973: Compatibility With Entity Query Field Exists Method could probably make use of this.

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.

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. The entity.api.php changes and the change record also suggest that all entity queries would become alterable, not only the SQL-based ones.

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',

#3433052: Fix remaining return*() methods of class PHPUnit\Framework\TestCase deprecated in PHPUnit 10

@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.

Aggregate entity queries were somehow missed in this issue, opened a followup for them: #3489929: Aggregate entity queries are missing the alter hooks