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.
| Comment | File | Size | Author |
|---|---|---|---|
| #25 | 256792.patch | 8.47 KB | douggreen |
| #23 | 256792.patch | 8.44 KB | douggreen |
| #10 | 256792_searchform.patch | 8.49 KB | Susurrus |
| #2 | 256792_searchform.patch | 8.47 KB | David Lesieur |
| #1 | searchform.patch.txt | 8.57 KB | robertdouglass |
Comments
Comment #1
robertdouglass commentedRerolled 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.
Comment #2
David Lesieur commentedAn occurence of the hardcoded 'type' key was remaining in node_search_validate(), causing the language and taxonomy options to use the wrong key. Fixed.
Comment #3
David Lesieur commentedI 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.
Comment #4
douggreen commentedI wasn't sure about where to put language either. I'm working on the tests...
Comment #5
David Lesieur commentedOh, 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.
Comment #6
dries commentedOnly 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.
Comment #7
douggreen commentedI 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.
Comment #8
Susurrus commentedsubscribe
Comment #9
catchShouldn'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.
Comment #10
Susurrus commentedThis 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.
Comment #11
catchOne 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.
Comment #12
Susurrus commentedhook_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.
Comment #13
catchSusurrus, of course! doh. And yeah the hook_$searchfromid_form_alter() version would be more efficient, especially with registry.
Comment #14
Susurrus commentednow 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?
Comment #15
robertdouglass commented@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.
Comment #16
catchTested 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?
Comment #17
robertdouglass commented@catch: I don't think so, and that would be a great thing to write some tests for.
Comment #18
catchOpened 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.
Comment #19
Susurrus commented@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.
Comment #20
Anonymous (not verified) commentedThe last submitted patch failed testing.
Comment #21
nedjoMore precise title.
Comment #22
webchickSubscribe. I hate search module so bad it hurts.
Looks like this patch needs a re-roll, some tests, and some docs.
Comment #23
douggreen commentedrerolled
Comment #25
douggreen commentedSimpletests rock, sorry, this patch fixes the problem and should pass all tests.
Comment #26
dmitrig01 commentedThe names should be hook_search_form and hook_search_keys or hook_node_search_form and hook_node_search_keys if this is only for the node search form.
Comment #27
jhodgdonI'm not sure why this didn't get in, but at this point it's too late for D7...
Comment #28
jhodgdonI think that we want to do this for 8.x, but the patch we have here will not work since search is now plugin-based.
Comment #29
klonosThe part of this issue discussed back in #3 (2008) where code was moved out of node.module etc was actually a duplicate of #237748: Decouple core search module implementations from node and user module (and turn search/node and search/user to views). I believe.
Anyways, I think we should postpone this on #2083717: Convert Search Results to Views (or close it as a duplicate or even make the other issue a META and this one one of its sub-issues).
Comment #30
jhodgdonI'm closing #2087195: Figure out how to let the NodeSearch's supported advanced search keys be modified by developers as a duplicate of this issue, and updating the title/category here.
From the description of that issue, and the one relevant comment:
Comment #31
pwolanin commenteddepends on #2042807: Convert search plugins to use a ConfigEntity and a PluginBag
Comment #32
pwolanin commentedThe original proposal is outdated. We to look at how the node search plugin can be configured to look for additional filters to parse from the query string
Comment #33
pwolanin commentedComment #34
jhodgdonThis is really a feature. Moving to 8.1.x for now.
Comment #36
mgiffordUnassigning stale issue. Hopefully someone else will pursue this.
Comment #51
smustgrave commentedThank you for sharing your idea for improving Drupal.
We are working to decide if this proposal meets the Criteria for evaluating proposed changes. There hasn't been any discussion here for over 8 years which suggests that this has either been implemented or there is no community support. Your thoughts on this will allow a decision to be made.
Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.
Thanks!
Comment #52
smustgrave commentedSearch isn't officially being deprecated but discussed here #3476883: [Policy, no patch] Move Search module to contrib think this is something that IF it does happen the new maintainer could decide.