411612 - Bugzilla OpenSearch produces Bugzilla error page in Camino

Status: RESOLVED FIXED

(Camino Graveyard :: Toolbars & Menus, defect)

Description

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

After adding the OpenSearch engine from bugzilla.mozilla.org, searching fails (Bugzilla complains about the input).

STR:

1. Visit bmo
2. Click on the search icon in the Camino search field and add "Bugzilla@Mozilla"
3. Search for "camino"

ER: https://bugzilla.mozilla.org/buglist.cgi?quicksearch=camino
A list of bugs having "camino" in their name or in the Camino product


AR: https://bugzilla.mozilla.org/buglist.cgi

"Bugzilla@Mozilla – Parameters Required

You may not search, or create saved searches, without any search terms."

Comment 1

[Sean Murphy]

It looks like Bugzilla is describing its search URL in a way different from the current OpenSearch spec.

OpenSearch Draft 3 specification indicates that URLs and parameters are to be given in the attributes of a single XML element:

> <Url type="application/rss+xml" template="http://example.com/?q={searchTerms}&amp;pw={startPage?}" />

Bugzilla only describes its base search URL in the template, and then uses a separate "Param" element to indicate additional query parameters:

> <Url type="text/html" method="GET" template="https://bugzilla.mozilla.org/buglist.cgi">
>   <Param name="quicksearch" value="{searchTerms}"/>
> </Url>

There's no mention of a "Param" element anywhere in Draft 3 of the spec, and since this isn't a private moz extension I'm guessing it must have existed in an earlier draft (as Firefox has supported OpenSearch for a long while now).  IE does not recognize this element.

I guess this is the nature of supporting a spec still in the draft stage.  I'll look into adding the ability to parse this kind of url description.  Thanks for catching this!

Comment 2

[Samuel Sidler (old account; do not CC)]

(In reply to comment #1)
> It looks like Bugzilla is describing its search URL in a way different from the
> current OpenSearch spec.

Then we should file this in the Bugzilla product here as well and get it fixed by them.

(Not saying we shouldn't fix this bug because we should, just that we should get bugzilla fixed too... maybe after fixing this bug. ;) )

Comment 3

[Chris Lawson (gone)]

If we can figure out who implemented OpenSearch for Firefox, it might be useful to get some input from them as far as what version(s) of the draft spec they wrote support for (and therefore, by extension, what site authors can reasonably expect to work). It looks like Gavin had a lot to do with it, so I'm cc'ing him.

Also see bug 335691, which looks like it might have been the Firefox version of this bug.

Comment 4

[Chris Lawson (gone)]

(In reply to comment #1)
> It looks like Bugzilla is describing its search URL in a way different from the
> current OpenSearch spec.

See bug 378759, which claims to have fixed it. I'm not sure if this is a different form of divergence with the spec or if that bug just never landed on the 3.0 branch.

Comment 5

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #4)
> (In reply to comment #1)
> > It looks like Bugzilla is describing its search URL in a way different from the
> > current OpenSearch spec.
> 
> See bug 378759, which claims to have fixed it. I'm not sure if this is a
> different form of divergence with the spec or if that bug just never landed on
> the 3.0 branch.

Or the change never got back-ported to bmo; there are all manner of potential causes here.  (And indeed, the search description seems to be based on some old version of the spec.)

Comment 6

[Sean Murphy]

Attachment: Patch

Adds support for the older "<Param>" element.

Comment 7

[Jeff Dlouhy]

Comment on attachment 296349 [details] [diff] [review]
Patch

>+    NSString *baseSearchURL = [self searchEngineURL];
>+    if (!baseSearchURL)
>+      return;
>+    NSString *paramName = [attributeDict objectForKey:@"name"];
>+    NSString *paramValue = [attributeDict objectForKey:@"value"];
>+    NSMutableString *searchURLWithParam = [NSMutableString stringWithString:baseSearchURL];

nit: Maybe add a new line after the return;

Other than that, looks good. r=me

Comment 8

[:Gavin Sharp [email: [e-mail]]]

I implemented OpenSearch support in Firefox. I used the current spec at the time. It was draft 1, I think? The links in bug 335691 are all broken, and earlier revisions of the 1.1 spec don't seem to be available at http://www.opensearch.org/Specifications/Archive :(.

The Firefox search service also accepts a slightly simpler format (sometimes referred to as "MozSearch", http://developer.mozilla.org/en/docs/Creating_MozSearch_plugins ) because we didn't want to use OpenSearch for our shipped engines.

We supported <Param> elements since the beginning - I thought they were required for existing-plugin support, so it surprises me to hear that IE ignores them. They were certainly in the spec at the time.

I'm not sure of the current state of the latest draft OpenSearch spec, and how well implemented it is (in Firefox or otherwise). I know that no changes to Firefox have been made to address any changes in the spec since the initial work that went into Firefox 2.

Comment 9

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 296349 [details] [diff] [review]
Patch

Tagging smorgan for sr?

Comment 10

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #1)
> > > It looks like Bugzilla is describing its search URL in a way different from the
> > > current OpenSearch spec.
> > 
> > See bug 378759, which claims to have fixed it. I'm not sure if this is a
> > different form of divergence with the spec or if that bug just never landed on
> > the 3.0 branch.
> 
> Or the change never got back-ported to bmo

This was the case, and reed will be fixing bmo in bug 411844 (whenever the next upgrade is).  Still, if there may be others out there based on the old version of the spec, this patch is good for us to take.

Comment 11

[Stuart Morgan]

Comment on attachment 296349 [details] [diff] [review]
Patch

>+    NSString *baseSearchURL = [self searchEngineURL];
>+    if (!baseSearchURL)
>+      return;
>+    NSString *paramName = [attributeDict objectForKey:@"name"];
>+    NSString *paramValue = [attributeDict objectForKey:@"value"];
>+    NSMutableString *searchURLWithParam = [NSMutableString stringWithString:baseSearchURL];

There's no need to have the base URL as both an immutable string then a mutable string; just get a mutable string from the base straight off the bat.

sr=smorgan otherwise.

Comment 12

[Sean Murphy]

Attachment: Patch, for Check-In

Updated with review comments for check-in.

Comment 13

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Checked in on the trunk and MOZILLA_1_8_BRANCH in advance of b1.  Thanks for the quick turn-around!