Eliminate "my" variables from the root level of modules

RESOLVED FIXED in Bugzilla 3.0

Status

()

Bugzilla
Bugzilla-General
--
enhancement
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: Max Kanat-Alexander, Assigned: Max Kanat-Alexander)

Tracking

2.23
Bugzilla 3.0
Bug Flags:
approval +

Details

Attachments

(1 attachment, 1 obsolete attachment)

17.48 KB, patch
Frédéric Buclin
: review+
Details | Diff | Splinter Review
(Assignee)

Description

12 years ago
Certain modules have variables declared with "my" outside of any subroutine.

This is fine in CGI, but in mod_perl it could get weird.

Under mod_perl, "my" variables are only defined once, at the time Apache starts up. After that they're still defined, but they stick around. They live forever and never get reset.

So it's just safer to use constants, or something else, or just get rid of the variables if possible.
(Assignee)

Comment 1

12 years ago
Created attachment 227829 [details] [diff] [review]
v1

Just to make it clear, my intention with this bug is to make mod_cgi and mod_perl behave identically when it comes to these variables.

I did leave around the "my $columns" variables in certain bugs. Those will go away when we make those .pm files use Bugzilla::Object. In the mean time, it would have been a hassle to eliminate them, and it's not going to cause a big issue.

In fact, most of these would have been fine if left alone, but there's a chance that in the future somebody would come along and make some change, and then there would be a mysterious bug that only showed up on mod_perl, and not in mod_cgi. Or some customizer would have a problem that they couldn't figure out.
Attachment #227829 - Flags: review?
(Assignee)

Updated

12 years ago
Attachment #227829 - Flags: review? → review?(justdave)
(Assignee)

Updated

12 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

11 years ago
Attachment #227829 - Flags: review?(justdave) → review?(LpSolit)
(Assignee)

Comment 2

11 years ago
Created attachment 228884 [details] [diff] [review]
v1.1

There was some bitrot; I fixed it.
Attachment #227829 - Attachment is obsolete: true
Attachment #228884 - Flags: review?(LpSolit)
Attachment #227829 - Flags: review?(LpSolit)

Comment 3

11 years ago
Comment on attachment 228884 [details] [diff] [review]
v1.1

Looks good. Works correctly on a non-mod_perl installation. r=LpSolit
Attachment #228884 - Flags: review?(LpSolit) → review+

Updated

11 years ago
Flags: approval?

Comment 4

11 years ago
Comment on attachment 228884 [details] [diff] [review]
v1.1

>Index: Bugzilla/Template.pm

>+sub _load_constants {
>+    use Bugzilla::Constants ();

Nit: just one thing: it doesn't make sense to use it in a routine AFAIK.
Flags: approval? → approval+
(Assignee)

Comment 5

11 years ago
Great. This puts mod_perl into a testable condition.

Checking in Bugzilla/BugMail.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/BugMail.pm,v  <--  BugMail.pm
new revision: 1.84; previous revision: 1.83
done
Checking in Bugzilla/Config.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config.pm,v  <--  Config.pm
new revision: 1.65; previous revision: 1.64
done
Checking in Bugzilla/FlagType.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/FlagType.pm,v  <--  FlagType.pm
new revision: 1.31; previous revision: 1.30
done
Checking in Bugzilla/Template.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template.pm,v  <--  Template.pm
new revision: 1.56; previous revision: 1.55
done
Checking in Bugzilla/Token.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Token.pm,v  <--  Token.pm
new revision: 1.46; previous revision: 1.45
done
Checking in Bugzilla/Search/Quicksearch.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search/Quicksearch.pm,v  <--  Quicksearch.pm
new revision: 1.11; previous revision: 1.10
done
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.