refactor advanced search form and search keywords
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | search.module |
| Category: | feature request |
| Priority: | normal |
| Assigned: | douggreen |
| Status: | patch (code needs review) |
LIVE FROM THE MINNESOTA SEARCH SPRINT, this patch refactors the advanced search form and search keys so that modules can add additional "things" to search on. In the current code only node types, taxonomy terms, and languages are added to the advanced search form. Then, everywhere the search keys are processed, these three special use cases are checked for.
This patch makes the system more generic.
I've added two new hooks, hook_searchform and hook_searchkeys:
hook_searchform returns the form that gets added to the advanced search fieldset. For example,
function taxonomy_searchform() {
if ($taxonomy = taxonomy_form_all(1)) {
$form['category'] = array(
'#type' => 'select',
'#title' => t('Only in the category(s)'),
'#prefix' => '<div class="criterion">',
'#size' => 10,
'#suffix' => '</div>',
'#options' => $taxonomy,
'#multiple' => TRUE,
);
return $form;
}
}Notice that the form returned does not included $form['advanced'] because it is relative to the advanced fieldset.
hook_searchkeys returns the keys used in this form along with the SQL to generate. hook_searchkeys may may also return additional join clause. For example,
function taxonomy_searchkeys() {
return array(
'category' => array(
'where' => "tn.cid = '%s'",
'join' => ' INNER JOIN {term_node} tn ON n.vid = tn.vid'
),
);
}I'm open to different hook names.
This patch accomplishes similar refactoring to the search_rankings patch #145242, although the two patches refactor different things for different purposes.
| Attachment | Size |
|---|---|
| searchform.patch | 8.39 KB |
#1
Rerolled to avoid E_ALL errors and to remove the leading space in the 'join' bit of searchkeys. TODO includes decide if the hook names are appropriate and to write tests.
#2
An occurence of the hardcoded 'type' key was remaining in node_search_validate(), causing the language and taxonomy options to use the wrong key. Fixed.
#3
I love this patch. It makes the advanced search form customizable without adding much complexity. It also makes the core cleaner by moving module-specific search terms where they really belong, and out of the node module.
My only concern is whether the language form element really belongs to the locale module. Should it go into the translate.module instead? It is translate that handles content translation, while locale is more about the user interface language.
#4
I wasn't sure about where to put language either. I'm working on the tests...
#5
Oh, just checked this more closely... The stuff that enables the language setting for content types is in locale.module. That surprises me a little, but that means the patch would be okay.
#6
Only skimmed this patch briefly and it is not up to standards yet:
* We don't use the word 'Node' in output.
* We write 'Node is sticky' not 'Node is Sticky'.
* Code comments are inconsistent.
* It would be useful to document the score functions (i.e. the math).
Gotta run now, more detailed review forthcoming. Other than the above details, it looks good.
#7
I think that Dries's comment above was meant for #145252, which is where we added "Node is Sticky." and where the math functions accidentally were uncommented.
#8
subscribe
#9
Shouldn't it be hook_search_form() and hook_search_keys() instead of searchform/searchkeys ?
Overall this looks like a lovely cleanup patch otherwise. No test regressions.
#10
This relates to #233476: Add search by node creation date and the author and would be nice to get into core. Here's a reroll with the changes in #9.
#11
One thing that'd be nice, would be the possibility to remove stuff from the advanced_search form too - say I don't want a full list of taxonomy terms on there etc., it's not clear to me whether that's possible with this patch. hook_search_form_alter() ? Could be a followup though if it's not already covered.
#12
hook_form_alter() should still work. I'm not sure the order in which hooks are called by module, but any module's hook_form_alter() could remove items as long as it is called after node.module's hook_form_alter().
I had a thought about the search form, though. Wouldn't it be more efficient to use the specific alter hook for the search form instead of the general hook_form_alter()? That's one less function that's called every time a form is altered.
#13
Susurrus, of course! doh. And yeah the hook_$searchfromid_form_alter() version would be more efficient, especially with registry.
#14
now I'm wondering if this hook should be run from search.module. This would also make all alter hooks of the form be able to remove any form item, not just those modules called after node.module.
This sound more like the right approach?
#15
@Susurrus: not sure about putting the hook in search module. The node search implementation is distinct from the search module, which is supposed to be just a framework. This distinction needs to be preserved. Nothing specific to any individual search should be put in search.module. But maybe I don't understand what you're proposing?
Also, I think we should focus on getting the current patch in as it is a clear improvement, and then proceed with further refactoring.
#16
Tested the patch and it works fine.
I noticed there's no tests for ensuring the advanced search form itself works properly - is there an issue elsewhere for that?
#17
@catch: I don't think so, and that would be a great thing to write some tests for.
#18
Opened an issue here for advanced search form tests #280794: TestingParty08: Tests needed for advanced search form. Since it ought to work the same on the surface with or without this patch, it's probably good in a separate issue.
#19
@robertDouglass: I'm not proposing putting any additional fields into search.module. What I'm proposing is just moving the hooks into search.module since they're search hooks. I find it weird that node.module is calling hooks like hook_search_keys(), as I would think that would be called from search.module.
What I think should happen is that the advanced search form be supported by search.module itself instead of tacked on through node.module.