Problem/Motivation

The SearchQuery extender only supports queries that it expects, due to the way it does queries in a two-step process.

In particular, the only supported way to add orderBy to queries using this extender is to use addScore().

Proposed resolution

Document this fact in the SearchQuery class docs or the addScore() method docs on it.

Remaining tasks

Make a patch.

User interface changes

API changes

Original issue...

The SearchQuery countQuery method removes fields and expressions, but not orderings, which causes the query to fail when there is a need to manually specify calculated_total ordering by hand (e.g. when specify additional subsequent orderings to establish order of items with tied scores). This can be resolved by resetting the order by array on the cloned inner query as per fields and expressions:

$orderbys =& $inner->getOrderBy();
$orderbys = array();
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon’s picture

Category: Task » Bug report
Status: Active » Postponed (maintainer needs more info)

Huh, I'm confused here. Can you explain what you mean by "manually specify calculated_total ordering by hand"?

Also do you know if this is also a problem in Drupal 8?

pstewart’s picture

Status: Postponed (maintainer needs more info) » Active

So I'm using a modified search query in a hook_search_execute implementation, for which I need to specify ordering for when scores are tied. This means that I need to add in the calculated_score orderBy myself so I can add in my other orderBy clauses after it, rather than let execute() do it for me:

...
$query->searchExpression($keys, $search_types);
...
$query->orderBy('calculated_score', 'DESC')
  ->orderBy('i.type', 'DESC')
  ->orderBy('i.sid', 'DESC');
...

If no ordering is specified, then everything is hunky-dory: the query by default has no ordering at the countQuery() stage, and only gets ordered during execute() on line 491:

    // If an order has not yet been set for this query, add a default order
    // that sorts by the calculated sum of scores.
    if (count($this->getOrderBy()) == 0) {
      $this->orderBy('calculated_score', 'DESC');
    }

However, if you specify an ordering as per my first snippet, then the count query fails because it doesn't strip the ordering back out as it does with fields and expressions. Adding the two lines of code from the initial report after the expressions on line 526 of search.extender.inc fixes this.

I've not tried it explicitly in D8, but the countQuery() method is very similar and has the same problem.

jhodgdon’s picture

Version: 7.x-dev » 8.0.x-dev
Status: Active » Postponed (maintainer needs more info)
Issue tags: +Needs steps to reproduce

Thanks for the explanation. We'll need to address this in Drupal 8 then first, then backport to Drupal 7.

So, I'm still kind of confused about this. It would help if you could post clear steps to reproduce this bug, like "Install this module in Drupal 7 (post the module), go to admin/config/search/settings and enable it as a search module, go to search/foo, and search for 'bar'". And if you could post the fix you needed in Drupal 7 to make your module work, as a patch for Drupal 7, that would also be helpful.

The reason I'm asking for this is that in order to get this fixed in Core, we would need to make both an automated test and the code fix, and demonstrate that the test fails without the code patch, and passes with the code patch. At this point, all I have to go on is a bit vague to me, and I don't know enough to make an automated test. (If you wanted to make an automated test in a patch, that would even be better, but I know that's a lot to ask. Posting your module code and the fix you're proposing would at least be a great help.)

So... the other question I have is more fundamental: I'm kind of wondering whether if you're doing something to SearchQuery that it isn't expecting, it wouldn't make more sense to make your own Search Query class that extends the default class and does the right thing in the CountQuery method? I'm not sure we should really require the Search classes to handle unexpected methods of doing things. I mean, we can't really anticipate all the things someone might do to a query... But I'm happy to be wrong about this... just not sure whether we need this to get fixed in Core, or whether it would be more sensible for you to extend the query class and make it work for you. ???

pstewart’s picture

Hi jhodgdon,

Thanks for the response - I can certainly make a patch, and am happy to have a go at making a test case, albeit for D7 in both cases (I've not chance to try out D8 yet). It looks to me like the SearchQuery class is written with the expectation of being to be able to specify ordering, a lot of the code comments with the class seem to imply this, it just looks like no one's actually tried it before.

jhodgdon’s picture

If you can make a test case and/or a patch for Drupal 7, that would be a great help! Another contributor (possibly me) can definitely work with that and port it to Drupal 8. Thanks!

jhodgdon’s picture

I'm still confused about what is going on here. Have you been able to make a test case, or do you have a special search module that is triggering this error that you could attach here? See #3 for details of what we'd need.

If not, this issue is likely to get closed as "Cannot Reproduce", because we have no way to reproduce it.

jhodgdon’s picture

Status: Postponed (maintainer needs more info) » Closed (cannot reproduce)

We are triaging search issues... I am currently closing this as "Cannot reproduce", since there are no steps to reproduce the issue after several months.

If it is still an issue, please reopen and provide clear steps to reproduce the issue in Drupal 7 or 8, or a module that triggers it, or ... something like that. Thanks!

pstewart’s picture

Status: Closed (cannot reproduce) » Active
FileSize
1.45 KB
959 bytes

Sorry it's taken me so long to get back on this. I've created two patches (these are against D7). One patch modifies SearchMatchTestCase::_testQueries() to test the count query. This mimics the behaviour when a SearchQuery which is also a PagerQuery is executed, as the PagerQuery execute performs the count query. I've also included includes an explicit orderBy on the SearchQuery in this patched test.

The other patch modifies SearchQuery::countQuery to remove order bys from the count query. The test fails without the countQuery patch, however it will succeed if the explicit orderBy is removed, as the SearchQuery doesn't normally add the implicit calculated_score orderBy to the query until after countQuery has been performed.

The essence of the problem is that if you have a need to specify explicit ordering beyond the default on a SearchQuery which is also a PagerQuery, then it won't work unless countQuery removes order bys from the count query.

jhodgdon’s picture

Category: Bug report » Support request
Status: Active » Fixed

Ah, I see your problem. You aren't supposed to add orderBy to the query "by hand". You should use the addScore() method on SearchQuery instead. This exists on Drupal 7 and 8 versions.

I do not think this is something we should fix in the SearchQuery class. It definitely does not support all types of queries, because of its two-pass functionality.

pstewart’s picture

I've been using orderBy to tie-break results with tied scores, which I realise should be solved by using addScore more effectively.

However, isn't it still a bug that orderBy is broken? The documentation for addScore implies that it should work. There are use cases (albeit a bit tenuous, perhaps): for example, what if you wanted to reverse the search results? Surely it would be preferable to manually specify orderBy('calculated_total', 'ASC'). Or how about if you wanted to implement user-selectable sorts on a results page (e.g. sort by content type then by score)? These might not be common things to want to do, but it would be a shame to rule them out for want of a simple fix for what seems to be an oversight in the SearchQuery::countQuery implementation.

jhodgdon’s picture

I'm not convinced. Scores are supposed to be used because they're properly normalized etc. Using custom orderBy is not supposed to be used.

pstewart’s picture

Might I suggest that if orderBy is to remain unfixed then the documentation for addScore is updated to reflect this, and make explicitly clear that orderBy is not supported by paged search queries?

jhodgdon’s picture

Title: SearchQuery->countQuery() does not remove ordering, breaks when adding calculated_totals ordering by hand » Document that SearchQuery does not support orderBy in queries outside of addScore
Component: search.module » documentation
Category: Support request » Task
Issue summary: View changes
Status: Fixed » Active
Issue tags: -Needs steps to reproduce +Novice, +Needs backport to D7

OK.

FMB’s picture

Tried to correct the documentation as suggested. Also corrected some coding conventions and faulty markup.

FMB’s picture

Status: Active » Needs review
jhodgdon’s picture

Status: Needs review » Needs work

Hm, sorry maybe my issue summary wasn't clear.

We're talking about the SearchQuery class in core/modules/search/src/SearchQuery, not the generic SelectExtender class. The needed documentation should go there.

FMB’s picture

Thank you jhodgdon. I am a bit confused: should we document this on addScore() or orderBy()? (Apparently there is already a word about this in the documentation block of addScore if I understand correctly.)

jhodgdon’s picture

SearchQuery does not have its own orderBy() method.

And you're right, addScore() has a note related to this, but it currently says:

    * Also note that if you call orderBy() directly on the query, search scores
   * will not automatically be used to order search results. Your orderBy()
   * expression can reference 'calculated_score', which will be the total
   * calculated score value.

This needs to say something different : it needs to document that orderBy will not work at all.

The last submitted patch, 8: search.test.inc_.7-x.20150415.diff, failed testing.

The last submitted patch, 8: search.extender.inc_.7-x.20150415.diff, failed testing.

FMB’s picture

Status: Needs work » Needs review
FileSize
891 bytes

Here is a new patch for addScore().

jhodgdon’s picture

OK, maybe we should give a bit more information.

How about:

Note that you must use this method to add ordering to your searches, and not call orderBy() directly, when using the SearchQuery extender. This is because of the two-pass system the SearchQuery class uses to normalize scores.

FMB’s picture

Here is a new patch which brings more accurate information.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Perfect, thanks for the patch!

alexpott’s picture

Version: 8.0.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Docs are not frozen in beta. Committed 6b91f52 and pushed to 8.0.x. Thanks!

  • alexpott committed 6b91f52 on 8.0.x
    Issue #2369675 by FMB, pstewart: Document that SearchQuery does not...
FMB’s picture

Status: Patch (to be ported) » Needs review
FileSize
1 KB

Here is the patch for D7.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, looks good!

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 7.x - thanks!

  • David_Rothstein committed 793788f on 7.x
    Issue #2369675 by FMB, pstewart, jhodgdon: Document that SearchQuery...

Status: Fixed » Closed (fixed)

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