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 |
opensearch_search_triggered_on_non_admin_pages.patch | 517 bytes | neclimdul | |
Comments
Comment #1
Dave ReidThis 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
apadernoThe condition checked before the code that invoke the search hook is the following:
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
neclimdulSure, 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
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.
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
neclimdulBTW, 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 commentedThe 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 commentedIMO, 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
neclimdulThat 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 commentedIt'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.Comment #9
Anonymous (not verified) CreditAttribution: Anonymous commentedI changed the code, and committed it.
Thanks for the report, and the help.