Closed
Bug 399371
Opened 17 years ago
Closed 17 years ago
Search.pm should use named subroutines instead of closures
Categories
(Bugzilla :: Query/Bug List, enhancement, P1)
Tracking
()
RESOLVED
FIXED
Bugzilla 3.2
People
(Reporter: mkanat, Assigned: jjclark1982)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 4 obsolete files)
(deleted),
patch
|
mkanat
:
review+
|
Details | Diff | Splinter Review |
Right now Search.pm has a long list of regexes pointing to closures that modify, essentially, global variables. This makes for a pretty confusing architecture.
Instead, the regexes should point to named subroutine references. These subroutines can take named arguments, and return a hashref that contains the important data.
Then it will be easy to read through the regexp list, it will be easier to have functions refer to each other, and it will make the whole flow of Search.pm easier to understand.
Reporter | ||
Comment 1•17 years ago
|
||
Note: This is a meta-bug for tracking this general task. Patches should be filed in blockers, not on this bug itself.
Keywords: meta
Reporter | ||
Updated•17 years ago
|
Assignee: query-and-buglist → jjclark1982
Assignee | ||
Comment 2•17 years ago
|
||
This patch changes all anonymous closures into named methods, with references to external variables they modify passed as named arguments.
Closures that were simply inline redirects or errors were left inline (content must be used with matches, etc).
Logic that could be moved to the regular expression list was not modified (_flagtypes_name checks for type not changed in the code).
The $funcsbykey{",$t"}->() structure is still used but should definitely be re-evaluated for Bug 67036.
Attachment #293223 -
Flags: review?(mkanat)
Reporter | ||
Comment 3•17 years ago
|
||
Comment on attachment 293223 [details] [diff] [review]
v1
It seems that specifying two CC charts (using the normal user search boxes) breaks this.
Attachment #293223 -
Flags: review?(mkanat) → review-
Assignee | ||
Comment 4•17 years ago
|
||
Fixed incorrect dereferences that were causing some functions to crash, and updated some calls to $dbh->sql_in.
Attachment #293223 -
Attachment is obsolete: true
Attachment #298564 -
Flags: review?(mkanat)
Assignee | ||
Comment 5•17 years ago
|
||
update for michael.j.tosh patch
Attachment #298564 -
Attachment is obsolete: true
Attachment #298568 -
Flags: review?(mkanat)
Attachment #298564 -
Flags: review?(mkanat)
Reporter | ||
Comment 6•17 years ago
|
||
Comment on attachment 298568 [details] [diff] [review]
v3
Quick review: Various internal functions use $1 now, without a regular expression being in that context. That's not very safe, I think.
Attachment #298568 -
Flags: review?(mkanat) → review-
Assignee | ||
Comment 7•17 years ago
|
||
re-match regular expressions that had gone out of scope
Attachment #298568 -
Attachment is obsolete: true
Attachment #298622 -
Flags: review?(mkanat)
Assignee | ||
Comment 8•17 years ago
|
||
set $self->{'user'} unambiguously
Attachment #298622 -
Attachment is obsolete: true
Attachment #298627 -
Flags: review?(mkanat)
Attachment #298622 -
Flags: review?(mkanat)
Reporter | ||
Comment 9•17 years ago
|
||
Comment on attachment 298627 [details] [diff] [review]
v5
This looks fine to me, and it's mostly just moving already-existing code into subroutines.
I did some really, really basic tests on it, but of course with this level of changes, only a full QA run would actually catch any minor issues. So I'm going to say we should check it in.
Attachment #298627 -
Flags: review?(mkanat) → review+
Reporter | ||
Comment 10•17 years ago
|
||
This is a huge patch that will bitrot instantly if anybody touches Search.pm, so I think we should check it in pretty soon, unless anybody else wants to test this and say something about it.
Status: NEW → ASSIGNED
Flags: approval+
Keywords: meta
Priority: -- → P1
Target Milestone: --- → Bugzilla 3.2
Comment 11•17 years ago
|
||
Why am I CC'ed on this bug? A comment would help.
Reporter | ||
Comment 12•17 years ago
|
||
(In reply to comment #11)
> Why am I CC'ed on this bug? A comment would help.
"unless anybody else wants to test this and say something about it."
Just wanted to give you a chance to say something about it if you wanted to.
Reporter | ||
Comment 13•17 years ago
|
||
Checking in Bugzilla/Search.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search.pm,v <-- Search.pm
new revision: 1.151; previous revision: 1.150
done
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 14•17 years ago
|
||
This needs to be relnoted as something that affects customizers/developers.
Keywords: relnote
Reporter | ||
Comment 15•16 years ago
|
||
Added to the release notes for Bugzilla 3.2 in a patch on bug 432331.
Keywords: relnote
You need to log in
before you can comment on or make changes to this bug.
Description
•