Closed Bug 173629 Opened 22 years ago Closed 18 years ago

Clean up "my" variable scoping issues for mod_perl

Categories

(Bugzilla :: Bugzilla-General, defect, P1)

2.23
defect

Tracking

()

RESOLVED FIXED
Bugzilla 3.0

People

(Reporter: bbaetz, Assigned: mkanat)

References

()

Details

Attachments

(1 file, 6 obsolete files)

The following script will not work correctly under mod_perl:

my $foo;

sub bar {
  $foo = $foo + 42;
  print $foo;
}

See the URL for why. We need to fix this before we can use mod_perl in bugzilla.
Blocks: mod_perl
For the record, this does generate a compile-time warning when the file is first
loaded, if you try to run it under mod_perl, so that'll be the easiest way to
fix this is just make things run under mod_perl and chase the warnings :)

[Tue Nov 18 02:20:33 2003] process_bug.cgi: Variable "$UserInEditGroupSet" will
not stay shared at /var/www/html/bugzilla/process_bug.cgi line 764.

etc.
Summary: Clean up scripts for mod_perl → Clean up "my" variable scoping issues for mod_perl
For the record, I'm pretty sure that the code should instead be:

our $foo = 0;

sub bar {
  $foo = $foo + 42;
  print $foo;
}

Note that the "= 0" part is important.
Reassigning bugs that I'm not actively working on to the default component owner
in order to try to make some sanity out of my personal buglist.  This doesn't
mean the bug isn't being dealt with, just that I'm not the one doing it.  If you
are dealing with this bug, please assign it to yourself.
Assignee: justdave → general
QA Contact: mattyt-bugzilla → default-qa
(In reply to comment #2)
> For the record, I'm pretty sure that the code should instead be:
> 
> our $foo = 0;

> Note that the "= 0" part is important.


Looks correct. See also the comment following the multirun1.pl script, reported
below:

http://perl.apache.org/docs/general/perl_reference/perl_reference.html#Remedies_for_Inner_Subroutines

"To avoid this you can put _local_ in front of the _our_ declaration of all
variables other than simple scalars. This has the effect of restoring the
variable to its previous value (usually undefined) upon exit from the current
scope. As a side-effect _local_ also initializes the variables to _undef_. So,
if you recall that thing I said about adding explicit initialization when you
replace _my_ by _our_, well, you can forget it again if you replace _my_ with
_local our_."
Depends on: 342114
Attached patch v1 (obsolete) — Splinter Review
As far as I know, this should fix *most* of the places where the "my" thing is a problem. It might even fix them all.

If we have any remaining after this patch, we can start to file bugs for individual problems.
Assignee: general → mkanat
Status: NEW → ASSIGNED
Attachment #226642 - Flags: review?
Attachment #226642 - Flags: review? → review?(wicked+bz)
OS: Linux → All
Priority: -- → P1
Hardware: PC → All
Target Milestone: --- → Bugzilla 2.24
Version: unspecified → 2.23
Attached patch v2 (obsolete) — Splinter Review
I just read the mod_perl docs again, and they point out that "our" variables stick around until the next page call, which means that we'd be wasting memory with them unless we made them "local our" which causes them to die at the end of the CGI.
Attachment #226642 - Attachment is obsolete: true
Attachment #226645 - Flags: review?(wicked+bz)
Attachment #226642 - Flags: review?(wicked+bz)
Comment on attachment 226645 [details] [diff] [review]
v2

wrong patch!
Attachment #226645 - Attachment is obsolete: true
Attachment #226645 - Flags: review?(wicked+bz)
Attached patch Real v2 (obsolete) — Splinter Review
Oh, you're right! Here's the right patch.
Attachment #226902 - Flags: review?(LpSolit)
Attached patch v3 (obsolete) — Splinter Review
I found a few others that I had missed, by clicking around on my mod_perl installation.
Attachment #226902 - Attachment is obsolete: true
Attachment #227239 - Flags: review?(LpSolit)
Attachment #226902 - Flags: review?(LpSolit)
Attached patch v3.1 (obsolete) — Splinter Review
There was a typo in sanitycheck.cgi. I fixed it.
Attachment #227239 - Attachment is obsolete: true
Attachment #227824 - Flags: review?(LpSolit)
Attachment #227239 - Flags: review?(LpSolit)
Comment on attachment 227824 [details] [diff] [review]
v3.1

The part for process_bug.cgi is completely bitrotten. CheckCanChangeField is no longer in process_bug.cgi.

5 out of 12 hunks FAILED -- saving rejects to file process_bug.cgi.rej
Attachment #227824 - Flags: review?(LpSolit) → review-
Attached patch v4 (obsolete) — Splinter Review
Okay, I fixed the bitrot. I also noticed a few more places in process_bug.cgi that I forgot to fix, and I fixed them.
Attachment #227824 - Attachment is obsolete: true
Attachment #227844 - Flags: review?(LpSolit)
Comment on attachment 227844 [details] [diff] [review]
v4

>Index: editusers.cgi

> sub edit_processing {

>-    $editusers || $user->can_see_user($otherUser)
>+    user->in_group('editusers') || $user->can_see_user($otherUser)

Must be $user->in_group, not user->in_group. Else it looks good. I did a few tests on a non mod_perl installation and that's the only problem I found. Must be fixed on checkin. r=LpSolit
Attachment #227844 - Flags: review?(LpSolit) → review+
Flags: approval?
Flags: approval? → approval+
Okay, I did the checkin fix and un-bitrotted it.

Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.116; previous revision: 1.115
done
Checking in buglist.cgi;
/cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v  <--  buglist.cgi
new revision: 1.338; previous revision: 1.337
done
Checking in chart.cgi;
/cvsroot/mozilla/webtools/bugzilla/chart.cgi,v  <--  chart.cgi
new revision: 1.21; previous revision: 1.20
done
Checking in duplicates.cgi;
/cvsroot/mozilla/webtools/bugzilla/duplicates.cgi,v  <--  duplicates.cgi
new revision: 1.59; previous revision: 1.58
done
Checking in editclassifications.cgi;
/cvsroot/mozilla/webtools/bugzilla/editclassifications.cgi,v  <--  editclassifications.cgi
new revision: 1.23; previous revision: 1.22
done
Checking in editflagtypes.cgi;
/cvsroot/mozilla/webtools/bugzilla/editflagtypes.cgi,v  <--  editflagtypes.cgi
new revision: 1.40; previous revision: 1.39
done
Checking in editsettings.cgi;
/cvsroot/mozilla/webtools/bugzilla/editsettings.cgi,v  <--  editsettings.cgi
new revision: 1.8; previous revision: 1.7
done
Checking in editusers.cgi;
/cvsroot/mozilla/webtools/bugzilla/editusers.cgi,v  <--  editusers.cgi
new revision: 1.125; previous revision: 1.124
done
Checking in editwhines.cgi;
/cvsroot/mozilla/webtools/bugzilla/editwhines.cgi,v  <--  editwhines.cgi
new revision: 1.18; previous revision: 1.17
done
Checking in enter_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/enter_bug.cgi,v  <--  enter_bug.cgi
new revision: 1.140; previous revision: 1.139
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.334; previous revision: 1.333
done
Checking in query.cgi;
/cvsroot/mozilla/webtools/bugzilla/query.cgi,v  <--  query.cgi
new revision: 1.168; previous revision: 1.167
done
Checking in reports.cgi;
/cvsroot/mozilla/webtools/bugzilla/reports.cgi,v  <--  reports.cgi
new revision: 1.86; previous revision: 1.85
done
Checking in request.cgi;
/cvsroot/mozilla/webtools/bugzilla/request.cgi,v  <--  request.cgi
new revision: 1.36; previous revision: 1.35
done
Checking in sanitycheck.cgi;
/cvsroot/mozilla/webtools/bugzilla/sanitycheck.cgi,v  <--  sanitycheck.cgi
new revision: 1.118; previous revision: 1.117
done
Checking in showdependencygraph.cgi;
/cvsroot/mozilla/webtools/bugzilla/showdependencygraph.cgi,v  <--  showdependencygraph.cgi
new revision: 1.53; previous revision: 1.52
done
Checking in token.cgi;
/cvsroot/mozilla/webtools/bugzilla/token.cgi,v  <--  token.cgi
new revision: 1.44; previous revision: 1.43
done
Checking in userprefs.cgi;
/cvsroot/mozilla/webtools/bugzilla/userprefs.cgi,v  <--  userprefs.cgi
new revision: 1.102; previous revision: 1.101
done
Checking in votes.cgi;
/cvsroot/mozilla/webtools/bugzilla/votes.cgi,v  <--  votes.cgi
new revision: 1.45; previous revision: 1.44
done
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Here's the un-bitrotted and fixed version that I checked in.
Attachment #227844 - Attachment is obsolete: true
Keywords: relnote
Blocks: 361980
No longer blocks: 361980
Added to the release notes as part of bug 349423.
Keywords: relnote
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: