Searching goes to search/location even with location search module disabled [#524708]

When I search using the drupal search I get directed to the search/location results page, even though I do not have the location search module enabled.

It seems to be due to the location_search function in location.module running even though I am not using the location_search module.

I have attached a patch that fixes this problem. It is against 6.x-3.x-dev.

Comment	File	Size	Author
#1	location_hook_search_6x-3x_2.patch	815 bytes	Agileware
#1
#1	location_hook_search_5x-3x.patch	815 bytes	Agileware
#1
	location_hook_search_6x-3x.patch	956 bytes	Agileware

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

Agileware CreditAttribution: Agileware commented 20 July 2009 at 02:50

File	Size
location_hook_search_5x-3x.patch	815 bytes

location_hook_search_6x-3x_2.patch	815 bytes

Here is a slightly better patch along with the same patch for 5.x-3.x-dev

Comment #2

bdragon CreditAttribution: bdragon commented 28 July 2009 at 20:21

Status:

Needs review

» Needs work

Calling module_exists in global scope is a very bad idea, as is selectively defining functions.

Comment #3

Agileware CreditAttribution: Agileware commented 19 November 2009 at 11:00

I agree that it isn't the best idea but the way it works currently is also bad.
In the case of this hook function and the logic that determines whether or not the function exists I don't think it is dangerous even if it isn't really good practice.

Ideally hook_search should be in the location search module, not the location module.
The only issue then being the url, which is also annoying.

Comment #4

Agileware CreditAttribution: Agileware commented 10 March 2010 at 23:43

Status:

Needs work

» Needs review

Thinking about this more, due to the conditions that this function is defined
or not and the hook function that is being defines I don't really think this poses an issue.

Also, I have seen other modules doing the same thing.
For example look at content.token.inc in the cck module.
It does exactly this with no negative effects.

Comment #5

YesCT CreditAttribution: YesCT commented 11 July 2010 at 08:39

Status:

Needs review

» Postponed (maintainer needs more info)

Please check to see if this is still a problem in the new release. If it is, reply with a new summary and set the status back to active. and check to see if the patch needs a reroll against the new dev. Thanks.

Comment #6

jpmckinney CreditAttribution: jpmckinney commented 9 August 2010 at 18:19

Status:

Postponed (maintainer needs more info)

» Needs review

// When search.module asks for name, return FALSE to tell it that we're disabled.

This is not part of the API http://api.drupal.org/api/function/hook_search/6

Although search.module may understand what to do when module_invoke($module, 'search', 'name') returns FALSE, few (if any) other modules do. For example, when the location and opensearch modules are both enabled, but location_search is disabled, opensearch believes there is a real implementation of hook_search by the location module, even though there is not. Among other things, it displays an empty row on its admin page where the name of the search should appear.

The patch in #1 is the same patch I came up with, and should be applied, as not following the API breaks things (more than I've listed here).

Needs review

» Reviewed & tested by the community

Yes, I guess I could have set it to RTBC :)

Comment #10

rooby CreditAttribution: rooby commented 29 September 2010 at 13:41

Status:

Reviewed & tested by the community

» Fixed

Committed to 5, 6 & HEAD (Note that hook_search isn't in drupal 7 but I committed it anyway so the versions are kept in line until the d7 version is properly ported)
http://drupal.org/cvs?commit=429442
http://drupal.org/cvs?commit=429440
http://drupal.org/cvs?commit=429456
[EDIT: changed the url for the HEAD commit to the correct one]