Problem/Motivation

The SearchQuery class has a bunch of confusing stuff about first and second passes in its documentation and member names. This needs to be cleaned up.

Proposed resolution

Fix up the documentation so it is more accurate. Rename local variables and member names to reflect what they are actually for.

Remaining tasks

Patch needs to be tested and reviewed.

User interface changes

None.

API changes

The protected member variable $executedFirstPass is changed to $executedPrepare.
The public member function executeFirstPass() is changed to prepareAndNormalize().

Original report by douggreen

We need to rename the executeFirstPass() method in the SearchQuery object. I'm marking this critical, because I think the naming here is going to lead to even more "search" confusion.

I panicked when I looked at the new search.extender.inc SearchQuery::executeFirstPass() because the name lead me to think that we had rolled back to using two passes on the search query to generate results.

In 5.x, the first pass referred to "OR" selection using the temporary table; then the second pass applied the additional checks.

In 6.x, the first and second pass queries were removed, but the language was still left to describe the conditionals used.

The comment is correct. Here is the current comment excerpt from 7.x:

* Results are retrieved in two logical passes. However, the two passes are
* joined together into a single query. And in the case of most simple
* queries the second pass is not even used.
*
* The first pass selects a set of all possible matches, which has the benefit
* of also providing the exact result set for simple "AND" or "OR" searches.
*
* The second portion of the query further refines this set by verifying
* advanced text conditions (such as negative or phrase matches).

The method executeFirstPass() is misleading. AFAICT, we're not "executing" the first pass query here. We're building the first pass query and executing the normalization query. I'm not sure what to call this. Any suggestions?

CommentFileSizeAuthor
#18 interdiff-16-18.txt1.25 KBjhodgdon
#18 rename_executefirstpass-582938-18.patch10.02 KBjhodgdon
#16 interdiff-13-16.txt7.62 KBjhodgdon
#16 rename_executefirstpass-582938-16.patch9.15 KBjhodgdon
#13 rename_executefirstpass-582938-13.patch2.27 KBsuperspring
#8 rename_executefirstpass-582938-8.patch3.3 KBsuperspring
#6 rename_executefirstpass-582938-6.patch3.3 KBsuperspring
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

douggreen’s picture

douggreen CreditAttribution: douggreen commented
Title: Rename of executeFirstPass() in SearchQuery object » Rename executeFirstPass() in SearchQuery object
Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

Well, I'm neither. It was simply the only thing that came to my mind ;)

douggreen’s picture

douggreen CreditAttribution: douggreen commented

A few possibilities:

prepareAndNormalize()
prepareFirstPassAndNormalize()
buildFirstPassConditionsAndNormalize()
addFirstPassConditionsAndNormalize()

any of the above without the AndNormalize()...

sun.core’s picture

sun.core CreditAttribution: sun.core commented
Priority: Critical » Normal

Why is this critical? If this really is important, then you may have a chance to get this in until 15th... not sure though.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Version: 7.x-dev » 8.x-dev

This is an API change, booting to Drupal 8. The API should be long-ago fixed at this point, unless there's really a bug here.

superspring’s picture

superspring CreditAttribution: superspring commented
Assigned: douggreen » Unassigned
Status: Active » Needs review
FileSize
rename_executefirstpass-582938-6.patch3.3 KB

I've renamed it to prepareAndNormalize (since this describes it well) and modified the documentation appropriately.

Status: Needs review » Needs work

The last submitted patch, rename_executefirstpass-582938-6.patch, failed testing.

superspring’s picture

superspring CreditAttribution: superspring commented
Status: Needs work » Needs review
FileSize
rename_executefirstpass-582938-8.patch3.3 KB

Find and replace fail :p
Attempt two, same patch.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Needs work

I was looking through the old issues in the Search module... I think this is a good idea, but the patch needs some work:
- It needs to be rerolled.
- There are a number of documentation problems, which I'll list below.

But I think this is a good idea and I think the proposed new name for the method and property make sense.

The issues to be fixed:

a)

  /**
-   * Executes the first pass query.
+   * This prepares and normalizes the query.

Violates docs standards. Should be "Prepares and normalizes...".

b)

+   * This is done by parsing the query with prepareAndNormalize. The resulting
+   * keywords are then weighted and normalized to give them scores. After this
+   * the search can be executed or modified further.

This whole paragraph doesn't make sense to me, and it shouldn't refer to the name of the method that is being documented like that.

c)

+   * @return boolean

boolean -> bool

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

Actually though... Why do we need a separate method for this? Couldn't we just use the standard preExecute()?

Another issue #1435834: Cannot alter search queries, tag added too late suggested moving the call to executeFirstPass() into preExecute(), but I really don't see why we need a separate method when this standard method exists.

The docs for preExecute() on the interface are:
Generic preparation and validation for a SELECT query.

This seems like it should be there?

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

I think preExecute() is not possible because it has return values but I don't remember all the details, long time ago that I did the initial DBTNG conversion.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

Hm. The patch on comment #2 on the other issue seems to think it would work on preExecute. In this case, the normalize method already has logic that check whether it works, and it looks like preExecute is also a method to return FALSE if there is a problem? Seems like a good fit? But I'm not an expert on this...

superspring’s picture

superspring CreditAttribution: superspring commented
Status: Needs work » Needs review
FileSize
rename_executefirstpass-582938-13.patch2.27 KB

Rerolling and applying fixes from #9

Status: Needs review » Needs work

The last submitted patch, rename_executefirstpass-582938-13.patch, failed testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

I'll wait to review until a working patch is submitted... but as a quick note, please spell "normalize" consistently, and #9 (c) was not taken care of. Thanks!

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Title: Rename executeFirstPass() in SearchQuery object » Fix up documentation and member names in SearchQuery
Issue summary: View changes
Status: Needs work » Needs review
FileSize
rename_executefirstpass-582938-16.patch9.15 KB
interdiff-13-16.txt7.62 KB

Here's a new patch with a bunch of fixed-up documentation and one new renamed local variable. I also created an issue summary

The last submitted patch, 16: rename_executefirstpass-582938-16.patch, failed testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
FileSize
rename_executefirstpass-582938-18.patch10.02 KB
interdiff-16-18.txt1.25 KB

OK, that last patch didn't actually fix up the direct use of the renamed method in the NodeSearch plugin. This patch should work a bit better. I have no idea how any of the node searching tests managed to pass without that change. I would have expected all of them to throw exceptions and die.

I also don't know why the bot didn't set the issue status to needs work when the tests failed.

Very mysterious. Anyway, let's see how this one does. At least one of the failing tests passed for me locally, and searching is working. Should be OK.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Postponed

This issue probably should be postponed on
#1366020: Overhaul SearchQuery; make search redirects use GET query params for keywords
#1853536: Reintroduce Views integration for search.module (not supporting backlinks view)
which will most likely conflict and are higher priority.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Postponed » Fixed

This was completely taken care of in #1366020: Overhaul SearchQuery; make search redirects use GET query params for keywords

Status: Fixed » Closed (fixed)

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