Open search triggers search on search landing page [#829644]

This has cause some very odd behavior. Here's the commit: http://drupal.org/cvs?commit=327520

When we're checking it this is a result page, we're actually asking the module to do a search. On top of this being expensive, it means that things like apachesolr_has_searched() suddenly think the user has searched. Even worse, 6.x-1.2 seems to add a link tag to all searches on every page and IE will follow that link tag to opensearch/apachesolr_search and cause this search test to happen. That means, on /every page/ at least 1 search is being mad. Definitely a bad thing.

I don't know what the real fix is. It seems like maybe those links aren't suppose to be on every page? But even so, calling an actually search to see if we're on a result page can't be the right answer. The attach patch removes that code.

Comment	File	Size	Author
#7	829644_lighter_opensearch_is_search_result_page.patch	1.44 KB	neclimdul
#7
	opensearch_search_triggered_on_non_admin_pages.patch	517 bytes	neclimdul

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

Dave Reid

he/him

English

Nebraska USA

CreditAttribution: Dave Reid commented 16 June 2010 at 22:02

This is a last-resort condition that shouldn't even hit with core or apachesolr. What are you using that is causing this condition to hit?

Comment #2

apaderno

he/him

Italian

Brescia, 🇮🇹 🇪🇺

CreditAttribution: apaderno commented 17 June 2010 at 02:41

The condition checked before the code that invoke the search hook is the following:

     if ($module == 'apachesolr_search' && !empty($_GET['filters'])) {
       return TRUE;
     }

This means that the search hook could be invoked for apachesolr_search.module too, if $_GET['filters'] is empty. I am don't know if this could really happens, and in which cases, though.

Comment #3

neclimdul

He/Him

CreditAttribution: neclimdul commented 17 June 2010 at 14:23

Sure, so here's why I marked it critical and the whole thing about IE. If you look at the source of http://www.apqc.org/ you'll see

<link href="http://www.apqc.org/opensearch/node" title="Content search for APQC" rel="search" type="application/opensearchdescription+xml" /> 
<link href="http://www.apqc.org/opensearch/user" title=" search for APQC" rel="search" type="application/opensearchdescription+xml" /> 
<link href="http://www.apqc.org/opensearch/apachesolr_search" title="Search search for APQC" rel="search" type="application/opensearchdescription+xml" />

This is on every page because of the way opensearch_init is setup(maybe another bug but separate discussion). Now, IE8 is not the most brilliant browser in the word and seems to just blindly requests these URLs when it sees them. When it hits the apachesolr_search one, a couple things happen.

The above check fails because $_GET['filters'] is not set.
A full search is triggered in the next section of code.
apachesolr_search issues a validation error message because it doesn't have any keys to search on.
The next page view the user sees a cryptic validation error about search terms despite them actually searching for anything. Next page view since this was triggered as a background request that didn't call theme('page') to send messages.

Also, $_GET['filters'] not being set is actually the common case since that is only used for facets. The general search has the key as part of the url like core search. see: http://drupal.org/search/apachesolr_search/views

Comment #4

neclimdul

He/Him

CreditAttribution: neclimdul commented 17 June 2010 at 14:26

BTW, I spent a good part of my afternoon yesterday figuring this out. Its a weird interaction that I can easily see being missed during development. I couldn't reproduce the reported bug for quite a while because I was only testing in Firefox, Chrome and IE6, not IE8. If I sounded short I'm sorry, just trying to get a lot of information conveyed quickly.

Comment #5

Anonymous (not verified) CreditAttribution: Anonymous commented 19 June 2010 at 13:28

The code is trying to handle search URLs like http://drupal.org/search/apachesolr_search/?filters=type%3Aproject_project, which are accepted by the module, but that a user could not possibly generate when using the search page.

In order to get such URL generated from the module, users should not insert any keywords, but in that case they would get an error immediately (as they didn't pass a single keyword to search for).

The question is then: should Opensearch feed consider such cases, or not? As a user search cannot generate that kind of URLs, I would think that it is an edge case that should not be considered.

Comment #6

Anonymous (not verified) CreditAttribution: Anonymous commented 28 June 2010 at 17:03

IMO, the module should avoid to try to get the search results before it really needs them. In the case of a link like http://example.com/opensearch/node/search-this-term, the module should just return an empty feed, if the search doesn't return any result.

This means that _opensearch_is_search_result_page() should not be invoked when the module is supposed to return the Opensearch description file.
As reported, a link like http://example.com/opensearch/apache_solr causes a call to _opensearch_is_search_result_page() ($_GET['filters'] is not defined, when the browser follows a given link), which then tries to get the search results; this causes the ApacheSolr Search Integration module to report an error because it has not get any keywords to look for.
The behavior of Internet Explorer is correct; when it finds a meta tag like <link href="http://www.apqc.org/opensearch/node" title="Content search for APQC" rel="search" type="application/opensearchdescription+xml" />, it tries to get the Opensearch description because it adds a new search provider (I think that is the used term) to the list of available search providers.

When then the module is serving a link like http://example.com/opensearch/apache_solr/search-term, it doesn't make sense to first verify if there are search results, and then get them with a second call to the search hook.

Comment #7

neclimdul

He/Him

CreditAttribution: neclimdul commented 28 June 2010 at 18:19

File	Size
829644_lighter_opensearch_is_search_result_page.patch	1.44 KB

That sound like what I was thinking. I'm not sure of the original conversation or issue that caused this code to be added but if its really necessary that we have this query in, maybe this would be a better approach. Its a lot stricter about making sure there's actually a possible reason to ask the search API for results. I'm not really convinced its a valid check though since an empty result page is still a search result page.

Comment #8

Anonymous (not verified) CreditAttribution: Anonymous commented 29 June 2010 at 00:12

I'm not really convinced its a valid check though since an empty result page is still a search result page.

It's what I think too.
The search module doesn't return an error 404 page, when the search doesn't get any result; it seems logic the module doesn't return such error page too.

I will change the code between 6 hours. I will remove the calls to the function _opensearch_is_search_result_page(), and probably add the code as reported in patch @#7.