A recent patch #505214: Make search module into a cleaner and more flexible framework added the ability to disable particular module searches (such as node). However, the search module still assumes that node module is the default and will redirect you there - and then the advanced search form will still show.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ezra-g’s picture

Assigned: Unassigned » ezra-g

I'm taking a look at this during the code sprint.

ezra-g’s picture

Assigned: ezra-g » Unassigned
Status: Active » Needs review
FileSize
4.2 KB

Part of the problem here is that search.module assumes that it should use node.module's search when the type of search is not specified, regardless of whether node.module is configured a as an active search module.
Instead, administrators should be able to specify which module from the active ones should be used as the default.
This patch adds a "Default search module option" to the search_admin_settings form and redirects to this module's search form when someone browses to /search or search/[junk], where junk is a module that isn't configured as active for search or doesn't exist.

Thanks, @pwolanin for discussing this at the code sprint.

Status: Needs review » Needs work

The last submitted patch failed testing.

sun.core’s picture

+1, this is one of the obstacles I ran into in #375397: Make Node module optional

sun.core’s picture

Priority: Critical » Normal

Not critical. Please read and understand Priority levels of Issues, thanks.

jhodgdon’s picture

Status: Needs work » Needs review

#2: 567100.patch queued for re-testing.

jhodgdon’s picture

This looks like an excellent idea to get in, so I've requested a retest of the patch.

Status: Needs review » Needs work

The last submitted patch, 567100.patch, failed testing.

jhodgdon’s picture

Anyone interested in updating the patch in #2 so it will apply? This looks like something we should fix for D7.

jhodgdon’s picture

Status: Needs work » Needs review
FileSize
3.48 KB

Here's a new patch that fixes the idea that node must always be the default search type. It still uses node if node is active, and otherwise it just defaults to the first one it finds in the list. It also puts up an error message if you try to search and there are no active modules.

jhodgdon’s picture

Just a note that letting the UI decide which module is the default is a separate issue:
#248154: Allow UI to choose which search module is default tab

jhodgdon’s picture

A patch I just added to #245103: Search page tabs not highlighting also fixes this issue (they were rather intertwined).

jhodgdon’s picture

The latest patch of that issue does not fix this issue. chx thought they should be left separate. But this one shouldn't be addressed until that other issue is patched.

jhodgdon’s picture

Status: Needs review » Needs work

Marking as needs work until the other patch is committed from #245103: Search page tabs not highlighting

jhodgdon’s picture

Title: Advanced search form may show even when node search is disabled » Search module is assuming node is default search even when it's disabled

Changing title to reflect the actual issue that needs to be fixed.

jhodgdon’s picture

Assigned: Unassigned » jhodgdon

The relevant patch from that other issue has landed (follow-up patch is not intertwined with this issue). So, it's time to revisit this.

jhodgdon’s picture

Status: Needs work » Needs review
FileSize
11.07 KB

Here's a new patch. Kind of similar to the old patch. With tests and everything.

jhodgdon’s picture

#17: 567100-17.patch queued for re-testing.

pwolanin’s picture

Is this code right? The path component in $search_info['path'] which I think is passed into search_view() may be different than the module name. It just happens that for node and user they are the same.

jhodgdon’s picture

Status: Needs review » Needs work

Eeek. I think you may be right. Deserves a test, too (with a test module that defines a search with module 'foo', path 'bar', title 'baz').

jhodgdon’s picture

Gracious. Good catch, pwolanin -- the code in search_form() before the patch is also assuming that the path is the module name.

I'm definitely adding some tests for this!

jhodgdon’s picture

Status: Needs work » Needs review
FileSize
18.22 KB

Here's a brand new patch. It fixes the problem identified by pwolanin - that several places in the search module code, the module was assuming the search path was equal to the name of the module. That was left over from Drupal 6, which had that assumption, and there were a couple of places in the code where that hadn't been fixed. Plus my patch above introduced some more (or at least continued them).

I also added a bunch of tests that exercise this.

pwolanin’s picture

A few comments:

Throughout this code it's still unclear to me where we are referencing module names, and where we are referencing the 'path' that comes in from the user.

For example, this code:

+function search_view($type = NULL) {
+  $type = search_get_active_type($type);

Should instead be like:

+function search_view($path = NULL) {
+  $type = search_get_active_type($path);

Also, I see a possible problem with this:

+  $default = 'search_code_for_default';

This should use a string which can never be a valid setting - e.g. it should include spaces or some other characters.

jhodgdon’s picture

Good points. I think the new docblocks on these functions make clear what is a path and what is a module name, but I agree the argument names could make this even clearer. And you are right on the setting.

pwolanin’s picture

I'm also a bit unfond of the pattern here where /search is the default search and we redirect to it, since that seem to suggests that as I enable/disable modules the actuall target of that form submission will change.

I think we want to be doing the opposite - the bare /search path will redirect to the default search path? That's the Drupal 6 behavior.

pwolanin’s picture

I'm working on a new patch building on the above which handles this all a little differently.

jhodgdon’s picture

Good luck with that. The reason we are redirecting to path /search/ is due to the strange way we're using hook_menu(). See
#245103: Search page tabs not highlighting

jhodgdon’s picture

Also, if you want to change that behavior, the other issue would be a better place than this one to do it.

jhodgdon’s picture

pwolanin: I just want to save you a lot of time going down a fruitless path... see #245103 - the search tabs don't work without screwy hook_menu(), and then the redirect to path /search was necessary to not have two sets of tabs showing... read the comments on the issue or in the code to see why... I don't think you'll find another way to do it all that doesn't involve a redirect to /search for the default search path.

And again, if you do find a way, that other issue is the place to discuss it and/or change it, not here. I was asked on that issue not to bring this one into the patch, and let's do the same here.

Also, I think this is not as big of a problem or misdirection as you're thinking. Probably setting up which search modules are enabled should only happen once when the site is set up, so it shouldn't really be changing. So /search will go to whatever the default is... why is that a problem?

pwolanin’s picture

@jhodgdon - trying to get the tests to pass on the config form.

I'm not seeing the behavior you describe of 2 sets of tabs.

pwolanin’s picture

Here's my new attempt. It adds radios to the search settings form to select the default - I think that's better than having a function whose return value seemingly can only be controlled by tweaking module weights.

pwolanin’s picture

Thinking about search_data() - I don't think there is any reason we need to check for non-empty params - seems like babysitting the code.

Also improved some code comments, etc.

jhodgdon’s picture

RE #30 on two sets of tabs - you would need to go back to the code as it was in intermediate stages of #245103.

I'll test this patch and read it through more carefully a bit later this morning... I like the approach, but I'm skeptical about some of it working correctly at first glance (double tabs, etc.). But only trying it out will tell...

One thing I saw the first time through:

+  $form['active']['search_default_module'] = array(
+    '#title' => t('Default search'),
+    '#type' => 'radios',
+    '#default_value' => 'node',
+    '#options' => _search_get_module_names(),
+    '#description' => t('Determine which search module is the default.')
+  );

I don't think I like like the labeling on this form element. I would say "Determine" -> "Choose", since the administrator is making the choice, and maybe change the #title to "Default search module".

pwolanin’s picture

It all works well locally - but more testing certainly would be good.

jhodgdon’s picture

FileSize
22.14 KB

It does seem to be working correctly. I did some cleanup on the doc/comments, but the functionality is the same as in the patch in #32.

jhodgdon’s picture

pwolanin: I'm for RTBC on the patch on #35. What say you?

pwolanin’s picture

Status: Needs review » Reviewed & tested by the community

Robert Douglass looked it over too, so I think this it is.

jhodgdon’s picture

#35: 567100-cleanup.patch queued for re-testing.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Overall this looks good, no visual regressions from before. Two things:

+++ modules/search/search.pages.inc	14 Jun 2010 15:43:15 -0000
@@ -7,33 +7,54 @@
+    // No path or invalid path: find the default module. Note that if there
+    // are no enabled search modules, this function should never be called,
+    // since hook_menu() would not have defined any search paths.

I tested with an invalid path, and I get:

Error message
Notice: Undefined index: spoon in search_view() (line 30 of /Users/webchick/Sites/core/modules/search/search.pages.inc).

This means we're lacking test coverage for this branch.

+++ modules/search/search.test	14 Jun 2010 15:43:16 -0000
@@ -984,27 +987,35 @@
-    $edit[$body_key] = l($node->title, 'node/' . $node->nid);
+    $edit[$body_key] = l($node->title, 'node/' . $node->nid) . ' pizza sandwich';

What the heck is this? :P

40 critical left. Go review some!

pwolanin’s picture

argh- patch doesn't apply

also, when putting in an invalid search path, webchick saw:

Notice: Undefined index: spoon in search_view() (line 30 of /Users/webchick/Sites/core/modules/search/search.pages.inc).

but that could be due to the hunk that failed to apply.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
20.43 KB

I think some dope cleaned up up trailing white space in the doxygen. Hopefully this applies cleanly.

pwolanin’s picture

FileSize
21.74 KB

oops - missed the new test module

pwolanin’s picture

@webchick - we do test searches against the word "pizza" - but probably jhodgdon can explain better.

jhodgdon’s picture

Probably the tests need comments. My eyes are bugging out this morning... will take a look later.

jhodgdon’s picture

FileSize
22.51 KB

Here's a patch with an added comment on the pizza line, as well as an additional test to make sure that we test that if you go to an invalid URL, you're redirected to the right URL.

As a note, with the latest patch attached, I cannot reproduce webchick's problem reported in #39.

pwolanin’s picture

Status: Needs review » Reviewed & tested by the community

back to rtbc since the patch applies again, and jhodgdon was even energetic enough to add more test code.

jhodgdon’s picture

#45: 567100-45.patch queued for re-testing.

jhodgdon’s picture

What do we need to do to (please) get this committed?

pwolanin’s picture

#45: 567100-45.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 567100-45.patch, failed testing.

jhodgdon’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
22.43 KB

Straight reroll. It looks like a drupal_add_css() line was removed from search_form(), causing patch not to apply. Not sure why/when, but whatever...

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 567100-51.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
22.09 KB

looks like the conflict was moving the css add to hook_init()

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

This is a clean reroll of the RTBC patch above.

webchick’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +API change

Ok, checked over this again tonight and looks good. It looks like a pretty massive and scary patch, but a good chunk is tests. Yay.

Committed to HEAD.

There are quite a few functions changing signatures here, so tagging as "API change". Please summarize what adjustments, if any, contrib modules will need to make as a result of this patch.

pwolanin’s picture

Thanks!

This is the only place modules might need a minor change

-function search_data($keys = NULL, $type = 'node') {

+function search_data($keys, $module) {

It was previously a no-op if the keys were NULL, or if the $type was not specified.

Since we can't assume 'node' is valid, there is no sane default to supply here, so the caller always needs to supply a value.

jhodgdon’s picture

Of course, any module doing search_data will have already been supplying a module name to search their module's data... but OK, we can add a note to the upgrade page I guess? I am not sure an email notification of this api change is necessary. Actually or that documenting this change in the module upgrade guide is either.

search_view() has a change as well -- used to default to 'node', now defaults to the default search module (which is generally node unless node is disabled). Again, I don't know that this needs to be announced/documented as a change.

Another API change was an additional argument to search_get_info(), but since the behavior if you don't supply this argument is the same as it was before, and also this function is probably only used by search.module itself, I don't think we need to document that either.

Anything else that changed in the file that looks like an API change is really just a documentation change to clarify what was already happening.

rfay’s picture

I will announce that search_view() and search_data() changed signatures.

mfb’s picture

Status: Fixed » Needs review
FileSize
870 bytes

Here's a one-line cleanup to fix the default value (didn't seem worth filing a new issue :)

jhodgdon’s picture

Status: Needs review » Fixed

Are you seeing this not working? Why did you reopen this issue?

I think this is not necessary. I believe a while back there was a change to core so that on systems settings forms, the #default_value was used as the default for variable_get() and we didn't have to do variable_get() any more?

Marking it back to fixed until we get more information as to why you decided this was broken...

pwolanin’s picture

Status: Fixed » Reviewed & tested by the community

it's not working right - the settings form shows node when another module is selected as default.

This patch fixes it.

mfb’s picture

Status: Reviewed & tested by the community » Fixed

I can open a new issue if preferred (but it's a one-line fix and related to the title of this issue..) That change was rolled back so Drupal 7 settings forms still require variable_gets() unfortunately.

mfb’s picture

Status: Fixed » Reviewed & tested by the community
Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

mfb’s picture

Status: Fixed » Reviewed & tested by the community
jhodgdon’s picture

Good catch! It's definitely not committed.

marvil07’s picture

I can confirm that the last patch is still _not_ committed.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok, now it's committed to HEAD. ;) Thanks!

Status: Fixed » Closed (fixed)
Issue tags: -API change

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