Bugzilla mis-uses perl subroutine prototypes

RESOLVED FIXED in Bugzilla 2.22

Status

()

Bugzilla
Bugzilla-General
RESOLVED FIXED
13 years ago
13 years ago

People

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

Tracking

2.21
Bugzilla 2.22
Bug Flags:
approval +

Details

Attachments

(2 attachments)

(Assignee)

Description

13 years ago
I had a brief discussion with somebody in #perl on FreeNode, and I also just
re-read the "perlsub" documentation.

The actual purpose of perl's function prototypes is to *change the way a
subroutine is called*. For example, to make a subroutine that behaves like an
internal subroutine does. perlsub goes over it in much more detail, in the
"Prototypes" section.

So really, it's something that should be used rarely, if at all.

Patch forthcoming to remove them.
(Assignee)

Updated

13 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → Bugzilla 2.22
(Assignee)

Comment 1

13 years ago
Created attachment 191793 [details] [diff] [review]
Remove prototypes from Bugzilla subroutines

OK, here's a patch that removes the prototypes everywhere that they aren't
actually being useful or having their intended effect.

I didn't touch contrib code.
Attachment #191793 - Flags: review?(justdave)
(Assignee)

Comment 2

13 years ago
Comment on attachment 191793 [details] [diff] [review]
Remove prototypes from Bugzilla subroutines

This patch touches so many files that it's almost guaranteed to bitrot quite
quickly.
Attachment #191793 - Flags: review?(LpSolit)

Comment 3

13 years ago
Comment on attachment 191793 [details] [diff] [review]
Remove prototypes from Bugzilla subroutines

Here is what's left with your patch applied:

[root@antares bugzilla]# grep -nPR '^sub\s+\w+\s*\(' *
checksetup.pl:1753:sub PopulateEnumTable ($@) {
contrib/BugzillaEmail.pm:46:sub findUser($) {
contrib/bug_email.pl:129:sub storeAttachments( $$ )
contrib/bug_email.pl:184:sub horLine( )
contrib/bug_email.pl:262:sub Reply( $$$$ ) {
contrib/bug_email.pl:291:sub getEnumList( $ )
contrib/bug_email.pl:424:sub FetchAllSQLData( )
contrib/bug_email.pl:462:sub BugMailError($ $ )
contrib/bug_email.pl:491:sub getWarningText()
contrib/bug_email.pl:510:sub getErrorText()
contrib/bug_email.pl:534:sub generateTemplate()
contrib/bug_email.pl:653:sub extractControls( $ )
contrib/gnats2bz.pl:841:sub map_username_to_realname() {
contrib/mysqld-watcher.pl:89:sub sendEmail($$$$) {
t/Support/Systemexec.pm:8:sub system($$@) {
t/Support/Systemexec.pm:11:sub exec($$@) {

As t/Support/Systemexec.pm redefines system() and exec(), it seems logical to
use prototypes. You don't want to remove prototypes from contrib/ scripts
(why?), why not. So this left us with PopulateEnumTable in checksetup.pl, line
1753. Fix this missing one and you have my r+.
Attachment #191793 - Flags: review?(justdave)
Attachment #191793 - Flags: review?(LpSolit)
Attachment #191793 - Flags: review-
(Assignee)

Comment 4

13 years ago
I left PopulateEnumTable in there on purpose. It may actually be having an
effect. :-) (It's not just $$, that is.)
(Assignee)

Comment 5

13 years ago
Created attachment 191962 [details] [diff] [review]
v2

OK, I removed it from PopulateEnumTable, too.
Attachment #191962 - Flags: review?(LpSolit)

Comment 6

13 years ago
Comment on attachment 191962 [details] [diff] [review]
v2

r=LpSolit
Attachment #191962 - Flags: review?(LpSolit) → review+

Updated

13 years ago
Flags: approval?
Flags: approval? → approval+
(Assignee)

Comment 7

13 years ago
Checking in buglist.cgi;
/cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v  <--  buglist.cgi
new revision: 1.301; previous revision: 1.300
done
Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.422; previous revision: 1.421
done
Checking in editclassifications.cgi;
/cvsroot/mozilla/webtools/bugzilla/editclassifications.cgi,v  <-- 
editclassifications.cgi
new revision: 1.15; previous revision: 1.14
done
Checking in editcomponents.cgi;
/cvsroot/mozilla/webtools/bugzilla/editcomponents.cgi,v  <-- 
editcomponents.cginew revision: 1.55; previous revision: 1.54
done
Checking in editflagtypes.cgi;
/cvsroot/mozilla/webtools/bugzilla/editflagtypes.cgi,v  <--  editflagtypes.cgi
new revision: 1.21; previous revision: 1.20
done
Checking in editgroups.cgi;
/cvsroot/mozilla/webtools/bugzilla/editgroups.cgi,v  <--  editgroups.cgi
new revision: 1.54; previous revision: 1.53
done
Checking in editkeywords.cgi;
/cvsroot/mozilla/webtools/bugzilla/editkeywords.cgi,v  <--  editkeywords.cgi
new revision: 1.27; previous revision: 1.26
done
Checking in editproducts.cgi;
/cvsroot/mozilla/webtools/bugzilla/editproducts.cgi,v  <--  editproducts.cgi
new revision: 1.89; previous revision: 1.88
done
Checking in editvalues.cgi;
/cvsroot/mozilla/webtools/bugzilla/editvalues.cgi,v  <--  editvalues.cgi
new revision: 1.5; previous revision: 1.4
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.271; previous revision: 1.270
done
Checking in sanitycheck.cgi;
/cvsroot/mozilla/webtools/bugzilla/sanitycheck.cgi,v  <--  sanitycheck.cgi
new revision: 1.99; previous revision: 1.98
done
Checking in userprefs.cgi;
/cvsroot/mozilla/webtools/bugzilla/userprefs.cgi,v  <--  userprefs.cgi
new revision: 1.81; previous revision: 1.80
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.85; previous revision: 1.84
done
Checking in Bugzilla/BugMail.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/BugMail.pm,v  <--  BugMail.pm
new revision: 1.43; previous revision: 1.42
done
Checking in Bugzilla/Classification.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Classification.pm,v  <-- 
Classification.pm
new revision: 1.3; previous revision: 1.2
done
Checking in Bugzilla/Component.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Component.pm,v  <--  Component.pm
new revision: 1.2; previous revision: 1.1
done
Checking in Bugzilla/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB.pm,v  <--  DB.pm
new revision: 1.60; previous revision: 1.59
done
Checking in Bugzilla/Field.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Field.pm,v  <--  Field.pm
new revision: 1.3; previous revision: 1.2
done
Checking in Bugzilla/Group.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Group.pm,v  <--  Group.pm
new revision: 1.4; previous revision: 1.3
done
Checking in Bugzilla/Milestone.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Milestone.pm,v  <--  Milestone.pm
new revision: 1.3; previous revision: 1.2
done
Checking in Bugzilla/Product.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Product.pm,v  <--  Product.pm
new revision: 1.4; previous revision: 1.3
done
Checking in Bugzilla/Template.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template.pm,v  <--  Template.pm
new revision: 1.28; previous revision: 1.27
done
Checking in Bugzilla/Token.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Token.pm,v  <--  Token.pm
new revision: 1.32; previous revision: 1.31
done
Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.68; previous revision: 1.67
done
Checking in Bugzilla/Util.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Util.pm,v  <--  Util.pm
new revision: 1.35; previous revision: 1.34
done
Checking in Bugzilla/Version.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Version.pm,v  <--  Version.pm
new revision: 1.3; previous revision: 1.2
done
Checking in docs/makedocs.pl;
/cvsroot/mozilla/webtools/bugzilla/docs/makedocs.pl,v  <--  makedocs.pl
new revision: 1.8; previous revision: 1.7
done
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED

Comment 8

13 years ago
Still haven't figured out why, but this breaks the process of adding comments
somewhere in Bug.pm

Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 9

13 years ago
Backed this out.  Problem effects Appendcomment, but could have many other
issues as well.

	LpSolit	joel: the problem is if you have an 'undef' parameter in the middle of
parameters passed to the routine
	LpSolit	$a, $b, $c is read as $a, $c if $b is undefined
	joel	commentprivacy?
	LpSolit	yes
	LpSolit	if the prototype is set, we have $a, $b, $c as expected, even with $b =
undef
	LpSolit	there is a sort of "concatenation", where undefined parameters are ignored
<snip>
LpSolit	myk: it seems that AppendComment($a, $b, $c) with $b=undef is read as
AppendComment($a, $c)
<snip>
myk	joel: sounds like you should back it out pending resolution of this issue

Comment 10

13 years ago
I still don't understand why this doesn't work as expected. AppendComment() has
been moved from globals.pl into Bug.pm in bug 283561, and when this routine was
still in globals.pl, it didn't use prototypes. Does it mean routines in .pm
modules have to use prototypes? Or was AppendComment() broken when it was in
globals.pl?
Flags: approval+

Comment 11

13 years ago
OK, here is what I found:

# Testcase 1:
my $v1 = 4;
my $v2 = undef;
my $v3 = "toto";
Test($v1, $v2, $v3);

Testcase 1 works as expected.


# Testcase 2:
my $v1 = 4;
my $v2 = undef;
my $v3 = "toto";
$cgi->param(-name => 'c1', -value => $v2)
Test($v1, $cgi->param('c1'), $v3);

Testcase 2 doesn't work: Test() is seen as Test($v1, $v3).

Comment 12

13 years ago
oh, and Test() doesn't use prototypes:

sub Test {
    my @test = @_;
    my $string = "# of params: " . scalar(@test) . "\n";
    for (my $i = 0; $i < scalar(@test); $i++) {
        $string .= "P" . $i . ": " . $test[$i] . "\n";
    }
    die $string;
}

Comment 13

13 years ago
(In reply to comment #9)

OK, what I said on IRC was wrong. AppendComment() is called with
$cgi->param('commentprivacy') as a parameter so we are in the testcase 2
mentioned above. But $cgi->param('commentprivacy') is undefined when the user
doesn't check the "Private Comment" checkbox.

Note that writing Test($v1, scalar $cgi->param('c1'), $v3); in the testcase 2
fixes the problem.

Comment 14

13 years ago
That makes sense.   The docs describe the $ prototype as forcing the argument to
be trated as a scalar.

Updated

13 years ago
Depends on: 304044
(Assignee)

Comment 15

13 years ago
OK, bug 304044 fixed the regression, so requesting approval again.
Flags: approval?
Flags: approval? → approval+
(Assignee)

Comment 16

13 years ago
Checking in buglist.cgi;
/cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v  <--  buglist.cgi
new revision: 1.305; previous revision: 1.304
done
Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.425; previous revision: 1.424
done
Checking in editclassifications.cgi;
/cvsroot/mozilla/webtools/bugzilla/editclassifications.cgi,v  <-- 
editclassifications.cgi
new revision: 1.17; previous revision: 1.16
done
Checking in editcomponents.cgi;
/cvsroot/mozilla/webtools/bugzilla/editcomponents.cgi,v  <-- 
editcomponents.cginew revision: 1.58; previous revision: 1.57
done
Checking in editflagtypes.cgi;
/cvsroot/mozilla/webtools/bugzilla/editflagtypes.cgi,v  <--  editflagtypes.cgi
new revision: 1.24; previous revision: 1.23
done
Checking in editgroups.cgi;
/cvsroot/mozilla/webtools/bugzilla/editgroups.cgi,v  <--  editgroups.cgi
new revision: 1.57; previous revision: 1.56
done
Checking in editkeywords.cgi;
/cvsroot/mozilla/webtools/bugzilla/editkeywords.cgi,v  <--  editkeywords.cgi
new revision: 1.30; previous revision: 1.29
done
Checking in editproducts.cgi;
/cvsroot/mozilla/webtools/bugzilla/editproducts.cgi,v  <--  editproducts.cgi
new revision: 1.92; previous revision: 1.91
done
Checking in editvalues.cgi;
/cvsroot/mozilla/webtools/bugzilla/editvalues.cgi,v  <--  editvalues.cgi
new revision: 1.7; previous revision: 1.6
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.274; previous revision: 1.273
done
Checking in sanitycheck.cgi;
/cvsroot/mozilla/webtools/bugzilla/sanitycheck.cgi,v  <--  sanitycheck.cgi
new revision: 1.102; previous revision: 1.101
done
Checking in userprefs.cgi;
/cvsroot/mozilla/webtools/bugzilla/userprefs.cgi,v  <--  userprefs.cgi
new revision: 1.84; previous revision: 1.83
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.87; previous revision: 1.86
done
Checking in Bugzilla/BugMail.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/BugMail.pm,v  <--  BugMail.pm
new revision: 1.45; previous revision: 1.44
done
Checking in Bugzilla/Classification.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Classification.pm,v  <-- 
Classification.pm
new revision: 1.5; previous revision: 1.4
done
Checking in Bugzilla/Component.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Component.pm,v  <--  Component.pm
new revision: 1.4; previous revision: 1.3
done
Checking in Bugzilla/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB.pm,v  <--  DB.pm
new revision: 1.62; previous revision: 1.61
done
Checking in Bugzilla/Field.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Field.pm,v  <--  Field.pm
new revision: 1.5; previous revision: 1.4
done
Checking in Bugzilla/Group.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Group.pm,v  <--  Group.pm
new revision: 1.6; previous revision: 1.5
done
Checking in Bugzilla/Milestone.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Milestone.pm,v  <--  Milestone.pm
new revision: 1.5; previous revision: 1.4
done
Checking in Bugzilla/Product.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Product.pm,v  <--  Product.pm
new revision: 1.6; previous revision: 1.5
done
Checking in Bugzilla/Template.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template.pm,v  <--  Template.pm
new revision: 1.30; previous revision: 1.29
done
Checking in Bugzilla/Token.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Token.pm,v  <--  Token.pm
new revision: 1.35; previous revision: 1.34
done
Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.72; previous revision: 1.71
done
Checking in Bugzilla/Util.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Util.pm,v  <--  Util.pm
new revision: 1.37; previous revision: 1.36
done
Checking in Bugzilla/Version.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Version.pm,v  <--  Version.pm
new revision: 1.5; previous revision: 1.4
done
Checking in docs/makedocs.pl;
/cvsroot/mozilla/webtools/bugzilla/docs/makedocs.pl,v  <--  makedocs.pl
new revision: 1.10; previous revision: 1.9
done
Status: REOPENED → RESOLVED
Last Resolved: 13 years ago13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.