Deprecating hook_query_alter

+++ b/core/lib/Drupal/Core/Database/Query/Select.php
@@ -477,7 +477,9 @@ public function preExecute(SelectInterface $query = NULL) {
+      $module_handler->alter($hooks, $query);
+      $module_handler->alterDeprecated('This hook is deprecated in Drupal 9.1.0 and will be removed before Drupal 10.0.0. Convert your calls to hook_query_TAG_alter. See: https://www.drupal.org/node/', 'query', $query);

We need to reverse the order here because the hook_query_alter() was fired first. To preserve BC in D9 properly. Also $hooks = ['query']; should be $hooks = []; because with this code it's being fired once by the call to ->alter() and once by the call to ->alterDeprecated().

So git archaeology about the amusing if (isset($this->alterTags)) { mess.

We added the hook_query_TAG_alter in #320591: Tag specific alter hook at this point the generic alter was outside the if. And then later we did #619580: Only invoke query_alter on tagged queries which means we only alter tagged queries.

Since hook_query_alter() is called for queries that have at least a tag, why isn't the documentation for that hook changed to make that explicit?
hook_query_alter() is never an exact equivalent to hook_query_TAG_alter(), as the latter doesn't allow to alter any tagged query basing on its metadata.

It's true there are modules that implement hook_query_alter() to just handle queries with a very specific tag, but it also true that Drupal core doesn't use a specific tag to say the query contains a specific metadata. With hook_query_TAG_alter(), the modules are required to know a tag name, even if, for example, they are just interested in those queries for which $query->getMetadata('account') returns a value.

@daffie no we definite should not mark all tests @group legacy. That would imply we've not removed all usages of hook_query_alter() from core. That's not the case. The test module is #9 is broken - that's why things are failing everywhere.

We'll also need to replace taxonomy_test_query_alter() with the hook_query_TAG_alter() equivalent.

We also need to:

• update docs in \Drupal\Core\Database\Query\PagerSelectExtender::execute()
• update class docs of \Drupal\search\SearchQuery()
• update docs in \Drupal\Core\Database\Query\SelectExtender::execute
• Think about the help text in \Drupal\views\Plugin\views\query\Sql::buildOptionsForm()
• Think docs that are like "This is the hook_query_alter() for queries tagged with"

+++ b/core/tests/Drupal/KernelTests/Core/Database/AlterTest.php
@@ -5,8 +5,12 @@
 /**
  * Tests the hook_query_alter capabilities of the Select builder.
  *
+ * The group legacy is added, only because the deprecated hook_query_alter is
+ * called when a tag has been added to the query.
+ *
+ * @group legacy
  * @group Database
- * @see database_test_query_alter()
+ * @see database_test_query_TAG_alter()
  */
 class AlterTest extends DatabaseTestBase {

This test does not need to be marked @legacy. It's not relying on a hook_query_alter(). The comment is added here is incorrect. Merely calling $module_handler->alterDeprecated() does not trigger a deprecation. There has to be at least one implementation for the deprecation to be triggered.

Also I could not find any bug report where somebody complains that hook_query_alter does not work as promised.

There are developers who know that, though. (See tests/src/Functional/DbeeQueryUsersTest.php, for example.)

// The hook_query_alter function only works on dynamic tagged queries.
$query->addTag('test');
if (!$dbee) {
  $query->addTag('dbee_disabled');
}
$query->orderBy('uid');
$result = $query->execute();

+++ b/core/lib/Drupal/Core/Database/Query/Select.php
@@ -473,11 +473,13 @@ public function preExecute(SelectInterface $query = NULL) {
+      $module_handler->alterDeprecated('This hook is deprecated in Drupal 9.1.0 and will be removed before Drupal 10.0.0. Convert your calls to hook_query_TAG_alter. See: https://www.drupal.org/node/3127431', 'query', $query);
+      $module_handler->alter($hooks, $query);

I've been looking at this again and reading what was committed on #765860: drupal_alter() fails to order modules correctly in some cases I think unfortunately this is in the realms of the super super complex. I think in order to do this in a 100% bc compatible manner with HEAD we're going to have to copy large parts of \Drupal\Core\Extension\ModuleHandler::alter() to here :( Or somehow how add support for only deprecating a single hook out of an array of hooks in \Drupal\Core\Extension\ModuleHandler::alterDeprecated() which doesn't look good.

AH there is another way. We can call the old alter() as before but check implementations here also given the amount of time a select can occur I think we should limit the deprecation to being triggered once per execution - especially as this is low level and extremely performance sensitive code.

I've also tweaked the format of the deprecation message to match the standard - unfortunately the alterDeprecated() has not been updated yet.

I've also fixed the issue summary to encompass the learnings and concisely state the problem and the resolution.

I'm not sure the hook only being called on tagged queries is sufficient reason to remove it, for example cps_query_alter() in Drupal 7 uses it to support a dynamic amount of entity types. The query tags are predictable, but there would have been no way for CPS to specify each entity type's query tag individually in a different hook implementation, including an unknown number of contrib and custom ones.

https://git.drupalcode.org/project/cps/-/blob/7.x-1.x/includes/query.inc

CPS itself won't see a Drupal 8 port because workspaces module in core is covering the same use case (and without relying on hook_query_alter()), but the reason for implementing hook_query_alter() is the same if you had a similar situation.

@daffie correct me if I'm wrong but the motivation behind this issue was to try to bring ->select() performance closer to ->query() performance right? So we can deprecate query()? Given that we've discovered the alters only get fired for tagged queries - is this really worth it? Given the use-case described in #17?

no way for CPS to specify each entity type's query tag individually in a different hook implementation, including an unknown number of contrib and custom ones.

Doesn't entity types share a query tag entity_query that can be used for all entity types?

@johndevman That module is changing all the queries having a $entity_type . '_load_multiple' tag, not all the entity queries.

That tag is still used in Drupal 9: The SqlContentEntityStorage.php file contains this line.

$query->addTag($this->entityTypeId . '_load_multiple');

@daffie so the reason there is no Drupal 9 need for the module is because there was a multi-year project to refactor core entity storage, update every entity type to it, etc. etc. as well as iirc adding a new hook explicitly to prevent needing to query rewrite. But it's the same basic thing as occasionally needing to implement hook_form_alter(), even if a lot narrower.

If the multiple load query had also added a tag like entity_load_multiple alongside ENTITY_TYPE_load_multiple, CPS could have used that (similar to hook_BASE_FORM_ID_form_alter()).

Note that domain_entity (part of the domain module) which does have a supported Drupal 8 version and probably will have a D9 version too also uses hook_query_alter in exactly the same way as CPS does in Drupal 7.

I read all the comments in this issue, but I still have to find an explanation of why this hook should be removed.
It doesn't seem it's because it increases performance, as that is already done by invoking the hook only when the query has a tag, At this point, avoiding one or more if() in code doesn't make the code more performant.

hook_query_TAG_alter() cannot replace hook_query_alter() because the latter allows to alter every tagged query for which I don't need to know exactly the used tags. I could also alter every tagged query containing a specific metadata; in this case, the tag name is irrelevant, and the hook could not be hook_query_TAG_alter(), which requires to put a tag name in the hook name.
While Drupal core could use an entity_load_multiple tag for queries loading an entity, what can Drupal core do to make clear a query uses specific metadata? Should it add a tag like account for those queries for which $query->getMetaData('account') returns a value, and use a similar tag for everything added as metadata?

I wonder that you count only contrib mofuyles for the hook, I see at least 3 supported client projects which using this hook to provide custom access instead of writing hooks for every possible tag and metadata keys.

I will check deeper suggested replacement but jook removal is no-go in 9.x, domain entity has 2-3 variations in contrib and all of them used and supported.

It means it could be used as example to prevent security issues with hook removal

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.