Provide a line of code that defines legal query formats for other scripts to use

RESOLVED FIXED in Bugzilla 2.18

Status

()

Bugzilla
Query/Bug List
RESOLVED FIXED
14 years ago
5 years ago

People

(Reporter: cso, Assigned: cso)

Tracking

2.18
Bugzilla 2.18
Bug Flags:
approval +
blocking2.20 +
approval2.18 +
blocking2.18 +

Details

Attachments

(2 attachments, 7 obsolete attachments)

(Assignee)

Description

14 years ago
This comes from bug 274397 comment 19 - there should be a way to check whether a
query is in the required format.

I intend to look at this shortly.
(Assignee)

Comment 1

14 years ago
Nominating for Blocking 2.18 per Dave's comments in the other bug
Flags: blocking2.18?
(Assignee)

Comment 2

14 years ago
Created attachment 169438 [details] [diff] [review]
Fix v1

Patch v1

The Patch adds a function to globals.pl (IsValidQueryType) which can have extra
types added into it, and changes userprefs.cgi and query.cgi to use this for
checking for valid query types.  I'm not convinced the Userprefs stuff is
perhaps the way to go, but it works for me, and at the moment I can't come up
with anything better :(
(Assignee)

Updated

14 years ago
Attachment #169438 - Flags: review?(justdave)
Comment on attachment 169438 [details] [diff] [review]
Fix v1

Since the point of separating this was to make it easy on customizers, we
should probably put the list in a qw() so it's easy to read/modify, and put a
comment in front of it explaining what it's for and why someone might want to
add to the list (like if they add a format for the query page and want a cookie
set when people visit it)

Also, we've been trying to avoid adding stuff to globals.pl, because globals.pl
is going to go away soon anyway.  I'm not sure which .pm file would work best
for this one though...
Attachment #169438 - Flags: review?(justdave) → review-
Why are we not finding the valid query types by parsing the names of templates
in the appropriate directory? Performance?

This would be self-extending, and easier for customisers.

Gerv
(Assignee)

Comment 5

14 years ago
(In reply to comment #4)
> Why are we not finding the valid query types by parsing the names of templates
> in the appropriate directory? Performance?
> 
> This would be self-extending, and easier for customisers.

As I understand it, there are only two types of query this bit needs to refer
to, yet there are 7 search-*.html.tmpl files in the search/ folder.
(Assignee)

Comment 6

14 years ago
Created attachment 169592 [details]
Bugzilla::Query

Bugzilla::Query

This contains the IsValidQueryType subroutine.
(Assignee)

Comment 7

14 years ago
Created attachment 169593 [details] [diff] [review]
Patchv2 (Trunk)

Use the new module to check if its a valid query.
Attachment #169438 - Attachment is obsolete: true
(Assignee)

Comment 8

14 years ago
Comment on attachment 169593 [details] [diff] [review]
Patchv2 (Trunk)

Note, the patch also actually applies to 2.18, with an offset.
Attachment #169593 - Flags: review?(justdave)
(In reply to comment #4)
> Why are we not finding the valid query types by parsing the names of templates
> in the appropriate directory? Performance?

That's what we did originally, and it's the sole reason we released 2.18rc2 a
single week after 2.18rc1, because it was a fatal error.  The reports also use
query.cgi with search-* templates.  If you set a cookie when the reports page is
displayed, then you go back to the query page, you wind up with one of the
reports pages, and no way to get back because there's no tabs at the top.  See
bug 275788.
Flags: blocking2.20+
Flags: blocking2.18?
Flags: blocking2.18+
OS: Windows XP → All
Hardware: PC → All
Target Milestone: --- → Bugzilla 2.18
you know, perhaps for this reason, it would be better to provide a list of
formats to exclude instead of include...
(In reply to comment #9)
> See bug 275788.

Oops, make that bug 250881.

Comment 12

14 years ago
Comment on attachment 169592 [details]
Bugzilla::Query

>package Bugzilla::Query;

  Why doesn't this just go inside of Bugzilla::Search? A "search" and a "query"
are the same thing, I believe... although maybe not in Bugzilla...

  Still, validation of Bugzilla::Search's input seems like something that
should be in Bugzilla::Search.
(Assignee)

Comment 13

14 years ago
(In reply to comment #12)
>   Why doesn't this just go inside of Bugzilla::Search? A "search" and a "query"
> are the same thing, I believe... although maybe not in Bugzilla...

That would be because I didn't see that... 

Updated

14 years ago
Whiteboard: patch awaiting review
(Assignee)

Comment 14

14 years ago
(In reply to comment #12)
>   Still, validation of Bugzilla::Search's input seems like something that
> should be in Bugzilla::Search.

Something that occured to me just now: 

This is not validating Bugzilla::Search's input as such - neither of the files
that use this module make use of Bugzilla::Search (that I can see).

I'm quite happy to move this method in their though, if thats whats desired (and
I'll attach a patch shortly).
(Assignee)

Comment 15

14 years ago
Created attachment 169925 [details] [diff] [review]
Branch Patch v2

As previous patches, but with the code in Bugzilla::Search instead.
Attachment #169925 - Flags: review?
(Assignee)

Comment 16

14 years ago
Created attachment 169926 [details] [diff] [review]
Trunk Patch v3

Trunk patch with the code in Bugzilla::Search
Attachment #169926 - Flags: review?

Comment 17

14 years ago
Comment on attachment 169926 [details] [diff] [review]
Trunk Patch v3

+		 next if $comp !~ /query_format=(.*)/i;

Why the /i at the end? I've tested your patch on the trunk and hacked the URL
so that "Query_format" is used instead of "query_format".

It's not accounted for and it's not considered in any testing that I could
perform, so doing case insensitive searching here doesn't make sense.
Attachment #169926 - Flags: review? → review-

Comment 18

14 years ago
Comment on attachment 169926 [details] [diff] [review]
Trunk Patch v3

Same here.

Updated

14 years ago
Attachment #169925 - Flags: review? → review-
(Assignee)

Comment 19

14 years ago
Created attachment 169941 [details] [diff] [review]
Trunk Patch v4

Removes the /i (why I put it there, I can't think)
Attachment #169926 - Attachment is obsolete: true
Attachment #169941 - Flags: review?
(Assignee)

Comment 20

14 years ago
Created attachment 169942 [details] [diff] [review]
Branch Patch v4

Branch Patch, with the /i fixed
Attachment #169925 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #169942 - Flags: review?
(Assignee)

Comment 21

14 years ago
Created attachment 169945 [details] [diff] [review]
Trunk Patch v5

After discussion with vladd, he suggested a way to make userprefs.cgi simpler -
so this implements it.
(Assignee)

Updated

14 years ago
Attachment #169941 - Attachment is obsolete: true
Attachment #169945 - Flags: review?
(Assignee)

Updated

14 years ago
Attachment #169941 - Flags: review?
(Assignee)

Comment 22

14 years ago
Created attachment 169946 [details] [diff] [review]
Branch Patch v5

As attachment 169945 [details] [diff] [review] but for the trunk
Attachment #169592 - Attachment is obsolete: true
Attachment #169593 - Attachment is obsolete: true
Attachment #169942 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #169946 - Flags: review?
(Assignee)

Updated

14 years ago
Attachment #169593 - Flags: review?(justdave)
(Assignee)

Updated

14 years ago
Attachment #169942 - Flags: review?

Comment 23

14 years ago
Comment on attachment 169945 [details] [diff] [review]
Trunk Patch v5

There is some duplication regarding $q->{'query'} .= '&query_format=advanced';
but I'm not sure how to remove it. Otherwise looks good.

Regarding Dave's suggestion about the include/exclude thing, I'm afraid neither
will work since newly created templates could be in both categories and each
one of them will have to be manually reviewed (and added, if suitable, to
IsValidQueryType).
Attachment #169945 - Flags: review? → review+

Updated

14 years ago
Attachment #169946 - Flags: review? → review+

Updated

14 years ago
Status: NEW → ASSIGNED
Whiteboard: patch awaiting review → patch awaiting approval
(Assignee)

Updated

14 years ago
Flags: approval?
Flags: approval2.18?
Flags: approval?
Flags: approval2.18?
Flags: approval2.18+
Flags: approval+
Whiteboard: patch awaiting approval → patch awaiting checkin

Comment 24

14 years ago
Checking in query.cgi;
/cvsroot/mozilla/webtools/bugzilla/query.cgi,v  <--  query.cgi
new revision: 1.135; previous revision: 1.134
done
Checking in userprefs.cgi;
/cvsroot/mozilla/webtools/bugzilla/userprefs.cgi,v  <--  userprefs.cgi
new revision: 1.61; previous revision: 1.60
done
Checking in Bugzilla/Search.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search.pm,v  <--  Search.pm
new revision: 1.72; previous revision: 1.71
done

Checking in query.cgi;
/cvsroot/mozilla/webtools/bugzilla/query.cgi,v  <--  query.cgi
new revision: 1.126.2.4; previous revision: 1.126.2.3
done
Checking in userprefs.cgi;
/cvsroot/mozilla/webtools/bugzilla/userprefs.cgi,v  <--  userprefs.cgi
new revision: 1.58.2.3; previous revision: 1.58.2.2
done
Checking in Bugzilla/Search.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search.pm,v  <--  Search.pm
new revision: 1.57.2.6; previous revision: 1.57.2.5
done
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
Whiteboard: patch awaiting checkin
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.