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.

Comments

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?

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

<?php
    
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.

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.

  1. The above check fails because $_GET['filters'] is not set.
  2. A full search is triggered in the next section of code.
  3. apachesolr_search issues a validation error message because it doesn't have any keys to search on.
  4. 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

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.

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.

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.

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.

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.

Assigned:Unassigned» kiam
Status:Active» Fixed

I changed the code, and committed it.

Thanks for the report, and the help.

Status:Fixed» Closed (fixed)

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