399371 - Search.pm should use named subroutines instead of closures

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, enhancement, P1)

Description

[Max Kanat-Alexander]

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.

Comment 1

[Max Kanat-Alexander]

Note: This is a meta-bug for tracking this general task. Patches should be filed in blockers, not on this bug itself.

Comment 2

[Jesse Clark]

Attachment: v1

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.

Comment 3

[Max Kanat-Alexander]

Comment on attachment 293223 [details] [diff] [review]
v1

It seems that specifying two CC charts (using the normal user search boxes) breaks this.

Comment 4

[Jesse Clark]

Attachment: v2

Fixed incorrect dereferences that were causing some functions to crash, and updated some calls to $dbh->sql_in.

Comment 5

[Jesse Clark]

Attachment: v3

update for michael.j.tosh patch

Comment 6

[Max Kanat-Alexander]

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.

Comment 7

[Jesse Clark]

Attachment: v4

re-match regular expressions that had gone out of scope

Comment 8

[Jesse Clark]

Attachment: v5

set $self->{'user'} unambiguously

Comment 9

[Max Kanat-Alexander]

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.

Comment 10

[Max Kanat-Alexander]

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.

Comment 11

[Frédéric Buclin]

Why am I CC'ed on this bug? A comment would help.

Comment 12

[Max Kanat-Alexander]

(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.

Comment 13

[Max Kanat-Alexander]

Checking in Bugzilla/Search.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search.pm,v  <--  Search.pm
new revision: 1.151; previous revision: 1.150
done

Comment 14

[Max Kanat-Alexander]

This needs to be relnoted as something that affects customizers/developers.

Comment 15

[Max Kanat-Alexander]

Added to the release notes for Bugzilla 3.2 in a patch on bug 432331.