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)

3.1.2
enhancement

Tracking

()

RESOLVED FIXED
Bugzilla 3.2

People

(Reporter: mkanat, Assigned: jjclark1982)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 4 obsolete files)

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.
Note: This is a meta-bug for tracking this general task. Patches should be filed in blockers, not on this bug itself.
Keywords: meta
Assignee: query-and-buglist → jjclark1982
Attached patch v1 (obsolete) (deleted) — Splinter Review
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)
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-
Attached patch v2 (obsolete) (deleted) — Splinter Review
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)
Attached patch v3 (obsolete) (deleted) — Splinter Review
update for michael.j.tosh patch
Attachment #298564 - Attachment is obsolete: true
Attachment #298568 - Flags: review?(mkanat)
Attachment #298564 - Flags: review?(mkanat)
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-
Attached patch v4 (obsolete) (deleted) — Splinter Review
re-match regular expressions that had gone out of scope
Attachment #298568 - Attachment is obsolete: true
Attachment #298622 - Flags: review?(mkanat)
Attached patch v5 (deleted) — Splinter Review
set $self->{'user'} unambiguously
Attachment #298622 - Attachment is obsolete: true
Attachment #298627 - Flags: review?(mkanat)
Attachment #298622 - Flags: review?(mkanat)
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+
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
Why am I CC'ed on this bug? A comment would help.
(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.
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
This needs to be relnoted as something that affects customizers/developers.
Keywords: relnote
Blocks: 417507
Added to the release notes for Bugzilla 3.2 in a patch on bug 432331.
Keywords: relnote
Blocks: 472047
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: