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.In the SearchQuery extender, we add an alter tag like this:
$this->addTag('search_' . $this->type);
The problem is that this doesn't happen until the execute method is called, meaning that drupal_alter() has already happened (which gets done in preExecute()). The alter tag needs to be added as soon as we know the type of the query in order for it to be available to modules implementing hook_query_search_TYPE_alter(). Additionally, the preExecute method is where the first pass should be run in order to ensure that it happens before execute(), and has the power to stop the query from running.
For some background on this issue, see #997976: Add tag to node search query and #496516: Move query_alter() into a preExecute() method.
| Comment | File | Size | Author |
|---|---|---|---|
| #11 | 1435834-search_query_alters-11.patch | 3.18 KB | brockfanning |
| #2 | 1435834-search_query_alters-3.patch | 4.4 KB | rbayliss |
| #1 | 1435834-search_query_alters.patch | 2.64 KB | rbayliss |
Comments
Comment #1
rbayliss CreditAttribution: rbayliss commentedHere is a test that illustrates the problem. This should fail.
Comment #2
rbayliss CreditAttribution: rbayliss commentedAnd here is a patch including the test, with a fix to SearchQuery to move tagging into searchExpression(), and moving the first pass and associated metadata into preExecute(). This should pass.
Comment #3
rbayliss CreditAttribution: rbayliss commentedComment #4
kscheirer#2: 1435834-search_query_alters-3.patch queued for re-testing.
Comment #5
kscheirerRetesting against latest HEAD since it has been over a year.
Comment #7
jhodgdonI think we should address the moving of the normalization elsewhere -- it is not a good idea to combine issues.
See #582938: Fix up documentation and member names in SearchQuery
Anyway, this patch will need a reroll, but we should probably get it done. It should be limited to changing the tag stuff. Thanks!
Comment #8
jhodgdonI just marked #1146564: Search extender tags never 'hook'ed as a duplicate of this issue. It also has a patch.
Comment #9
jhodgdonI have possibly taken care of this issue on #1366020: Overhaul SearchQuery; make search redirects use GET query params for keywords, so postponing until that is complete.
Comment #10
jhodgdonI believe we fixed this on #1366020: Overhaul SearchQuery; make search redirects use GET query params for keywords (the tag is now added in SearchQuery::searchExpression() instead of later), but we should probably make sure by writing a test. It also needs a backport to D7.
Comment #11
brockfanning CreditAttribution: brockfanning commentedIf I understand right the next steps needed are a D7 patch limited to the tagging part only? If so, here's a reroll of rbayliss' patch along those lines.
Comment #12
brockfanning CreditAttribution: brockfanning commentedComment #13
jhodgdonForgot to set version to 7.x.
Comment #15
jhodgdon11: 1435834-search_query_alters-11.patch queued for re-testing.
Comment #16
jhodgdonThe failed test was against 8.x because the version was wrong. Now testing on 7.x.
Comment #17
brockfanning CreditAttribution: brockfanning commentedOops, thanks for that.
Comment #18
jhodgdonI have reviewed this patch and the test...
The code patch looks fine in search.extender.inc.
However, the test is not useful, in my opinion. We have a Drupal 8 SearchQueryAlterTest and a search_query_alter module, and they actually are good tests. We should just port that test and module to Drupal 7.