If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

process_bug.cgi needs some cleanup

RESOLVED FIXED in Bugzilla 3.2

Status

()

Bugzilla
Creating/Changing Bugs
--
minor
RESOLVED FIXED
13 years ago
10 years ago

People

(Reporter: Frédéric Buclin, Assigned: Frédéric Buclin)

Tracking

2.19.1
Bugzilla 3.2
Dependency tree / graph

Details

(Assignee)

Description

13 years ago
There are several points in process_bug.cgi which I cannot understand why there
are here. I may misunderstand some of them, but globally, I think this should go
away or be simplified.

Among others:

    if (!CheckCanChangeField('product', $::FORM{'id'}, $::oldproduct,
$::FORM{'product'})) {
        $vars->{'oldvalue'} = $::oldproduct;
        $vars->{'newvalue'} = $::FORM{'product'};
        $vars->{'field'} = 'product';
        ThrowUserError("illegal_change",
                       { oldvalue => $::oldproduct,
                         newvalue => $::FORM{'product'},
                         field => 'product',
                       },
                       "abort");
    }

Instead of:

ThrowUserError("illegal_change", $vars, "abort");


Another example:

    if ($ownerid eq $whoid) {
    if (Param('useqacontact') && ($qacontactid eq $whoid)) {
    if ($reporterid eq $whoid) {

Here, we compare numbers, so why not:

    if ($ownerid == $whoid) {
    if (Param('useqacontact') && ($qacontactid == $whoid)) {
    if ($reporterid == $whoid) {


Another one:

        $::FORM{'resolution'} = $str; # Used later by CheckCanChangeField

But in CheckCanChangeField, we say: return 1 if ($field eq "resolution"). So why
defining a field nobody cares about?


Another one:

foreach my $field ("rep_platform", "priority", "bug_severity",          
                   "summary", "bug_file_loc", "short_desc",
                   "version", "op_sys",
                   "target_milestone", "status_whiteboard") {
    if (defined $::FORM{$field}) {
        if (!$::FORM{'dontchange'}
            || $::FORM{$field} ne $::FORM{'dontchange'}) {
            DoComma();
            $::query .= "$field = " . SqlQuote(trim($::FORM{$field}));
        }
    }
}

Here, $::query is used to update the bugs table, but "summary" is not a valid
attribute of bugs. Btw, this field is never defined (does not exist)!


Another one:

            $qacontact = 0 unless (defined $qacontact && $qacontact != 0);

Why not:

$qacontact = 0 unless defined $qacontact;


Another one:

    my $dupe = trim($::FORM{'id'});

But $::FORM{'id'} is already trimmed by ValidateBugID at the beginning of the file.


Another one:

        my $checkid = trim($::FORM{'id'});
        SendSQL("SELECT bug_id FROM bugs where bug_id = " .  SqlQuote($checkid));
        $checkid = FetchOneColumn();
        if (!$checkid) {
            ThrowUserError("invalid_bug_id",
                           { bug_id => $checkid });
        }

That's exactly what ValidateBugID does when process_bug.cgi is called. Why
checking this twice?

There may be some others... Do not hesitate to mention them.
(Assignee)

Comment 1

13 years ago
All these dependent bugs, except one, are assigned to me.
Status: NEW → ASSIGNED
Depends on: 193125, 266579
(Assignee)

Updated

13 years ago
Depends on: 271797
Most of the code in process_bug.cgi has been there since "The Early Days" and
many of those much more logical function calls you're suggesting have only been
recently introduced.  Nobody has yet gone back and retrofitted process_bug.cgi.
 It's definitely in sore need of a cleanup.  There's been talk of completely
rewriting it, but nobody's ever got around to it.
(Assignee)

Updated

13 years ago
Depends on: 30345
(Assignee)

Updated

13 years ago
Depends on: 264601
I think the summary of this bug is the biggest understatement of the year.
(Assignee)

Updated

13 years ago
Depends on: 212940
(Assignee)

Updated

13 years ago
Depends on: 284701
(Assignee)

Updated

13 years ago
Depends on: 286160
(Assignee)

Updated

13 years ago
Depends on: 289365
(Assignee)

Updated

12 years ago
Depends on: 298196
(Assignee)

Updated

12 years ago
Depends on: 143313
(Assignee)

Updated

12 years ago
Depends on: 242318
(Assignee)

Updated

12 years ago
Depends on: 303401
(Assignee)

Updated

12 years ago
Depends on: 328437
(Assignee)

Updated

12 years ago
Target Milestone: --- → Bugzilla 2.24
(Assignee)

Updated

12 years ago
Depends on: 320058
(Assignee)

Updated

12 years ago
Depends on: 340253

Updated

11 years ago
QA Contact: mattyt-bugzilla → default-qa
(Assignee)

Updated

11 years ago
Target Milestone: Bugzilla 3.0 → Bugzilla 3.2

Updated

11 years ago
Depends on: 367914
(Assignee)

Updated

11 years ago
Depends on: 371774
(Assignee)

Updated

11 years ago
Depends on: 276553
(Assignee)

Updated

11 years ago
Depends on: 347213
(Assignee)

Updated

11 years ago
Depends on: 303183
(Assignee)

Updated

11 years ago
Depends on: 382978
(Assignee)

Comment 4

10 years ago
As far as I'm concerned, this bug is now fixed since the checkin of bug 412836.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.