Details
-
Type:
Patch
-
Status: Done/Fixed
-
Priority:
Trivial
-
Resolution: Fixed/Completed
-
Affects Version/s: 4.0.7
-
Fix Version/s: Unscheduled
-
Component/s: Core CiviCRM
-
Labels:None
-
Documentation Required?:None
-
Funding Source:Needs Funding
Description
On current trunk revision 37348.
Both CRM_Contact_Form_Search::preProcess and ::postProcess create their own instances of the customSelector class which in turn executes the search query based on slightly different parameters. I believe the difference in these parameters to be (likely) a bug that was not caught due to this redundancy; the second search was the only one whose results were seen. Additionally in the special case of the full-text searcher, a third set of search queries are made because the selectors created by the parent class are not persisted to class variables and made available for future use.
This patch persists the first selector and references it in the other two locations that reference it for significant savings when search queries are expensive.
Attachments
Activity
This appears to only affect custom queries since the base selector does not perform queries in the constructor. This might be more correctly fixed on the selector level instead of the Form/Search layer. Awaiting feedback.
I took a closer look at some of the custom searches and its basiclly the way the custom search is written. In general we dont want the constructor to do any work (at the most just setup and initializaton). Most of the work should be done when the actual calls are made
The fulltext search was doing work in the constructor and hence the queries were being called twice. I restructured the code a wee bit and now things happen as expected, i.e. the queries are just called once
i think the patch is in the right direction. However it does not work the way it currently is 
I've not tracked it down (will do so soon), but here's something to reproduce:
1. Goto contact basic search
2. Choose Individual and some group
3. hit search, will return all contacts
4. hit search again, will now do the search based on what you chose in set 2
I suspect the parameters sent to the selector is different and hence the change
Ah I see. I should have inspected the rest of the search implementations more closely (so much code!) and I would have seen that the approach used in the full text search was abnormal. I had expected that to be the normal approach and fixed the problem upstream.
Since it was only a flaw in that one implementation it does indeed make more sense to fix it closer to the source.
----------------------------
Re: the bug in my patch, I noticed that last night as well. It has to do with a misunderstanding I had about how the preProcess and postProcess were worked into the form submit and page building process. Its sometimes challenging to understand how Civi moves through the code and calls all the hooks.
I'm not sure if the selector instantiation can be concentrated (as I tried to do) in the preProcess step given the submit -> redirect -> display work flow.
I think this was addressed with the search optimizations in 4.4.
Removed whitespaces