303669 - Bugzilla mis-uses perl subroutine prototypes

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Max Kanat-Alexander]

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.

Comment 1

[Max Kanat-Alexander]

Attachment: 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.

Comment 2

[Max Kanat-Alexander]

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.

Comment 3

[Frédéric Buclin]

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

Comment 4

[Max Kanat-Alexander]

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

Comment 5

[Max Kanat-Alexander]

Attachment: v2

OK, I removed it from PopulateEnumTable, too.

Comment 6

[Frédéric Buclin]

Comment on attachment 191962 [details] [diff] [review]
v2

r=LpSolit

Comment 7

[Max Kanat-Alexander]

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

Comment 8

[Joel Peshkin]

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

Comment 9

[Joel Peshkin]

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

[Frédéric Buclin]

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?

Comment 11

[Frédéric Buclin]

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

[Frédéric Buclin]

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

[Frédéric Buclin]

(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

[Joel Peshkin]

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

Comment 15

[Max Kanat-Alexander]

OK, bug 304044 fixed the regression, so requesting approval again.

Comment 16

[Max Kanat-Alexander]

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