diff options
| -rw-r--r-- | Bugzilla/Auth/Login/WWW/CGI.pm | 2 | ||||
| -rwxr-xr-x | Bugzilla/Bug.pm | 9 | ||||
| -rw-r--r-- | Bugzilla/Error.pm | 24 | ||||
| -rw-r--r-- | CGI.pl | 4 | ||||
| -rwxr-xr-x | editclassifications.cgi | 2 | ||||
| -rwxr-xr-x | editcomponents.cgi | 7 | ||||
| -rwxr-xr-x | editmilestones.cgi | 11 | ||||
| -rwxr-xr-x | editusers.cgi | 18 | ||||
| -rwxr-xr-x | editversions.cgi | 3 | ||||
| -rw-r--r-- | globals.pl | 9 | ||||
| -rwxr-xr-x | post_bug.cgi | 7 | ||||
| -rwxr-xr-x | process_bug.cgi | 20 |
12 files changed, 46 insertions, 70 deletions
diff --git a/Bugzilla/Auth/Login/WWW/CGI.pm b/Bugzilla/Auth/Login/WWW/CGI.pm index 42e454f86..47c2b92b7 100644 --- a/Bugzilla/Auth/Login/WWW/CGI.pm +++ b/Bugzilla/Auth/Login/WWW/CGI.pm @@ -184,7 +184,7 @@ sub login { # If we get here, then we've run out of options, which shouldn't happen ThrowCodeError("authres_unhandled", { authres => $authres, - type => $type, }); + type => $type }); } # This auth style allows the user to log out. diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm index a6758d36f..b2261e1ee 100755 --- a/Bugzilla/Bug.pm +++ b/Bugzilla/Bug.pm @@ -511,21 +511,18 @@ sub ValidateTime { # (allow negatives, though, so people can back out errors in time reporting) if ($time !~ /^-?(?:\d+(?:\.\d*)?|\.\d+)$/) { ThrowUserError("number_not_numeric", - {field => "$field", num => "$time"}, - "abort"); + {field => "$field", num => "$time"}); } # Only the "work_time" field is allowed to contain a negative value. if ( ($time < 0) && ($field ne "work_time") ) { ThrowUserError("number_too_small", - {field => "$field", num => "$time", min_num => "0"}, - "abort"); + {field => "$field", num => "$time", min_num => "0"}); } if ($time > 99999.99) { ThrowUserError("number_too_large", - {field => "$field", num => "$time", max_num => "99999.99"}, - "abort"); + {field => "$field", num => "$time", max_num => "99999.99"}); } } diff --git a/Bugzilla/Error.pm b/Bugzilla/Error.pm index 4c6288a28..6eac5c94b 100644 --- a/Bugzilla/Error.pm +++ b/Bugzilla/Error.pm @@ -32,13 +32,15 @@ use Bugzilla::Util; use Date::Format; sub _throw_error { - my ($name, $error, $vars, $unlock_tables) = @_; + my ($name, $error, $vars) = @_; $vars ||= {}; $vars->{error} = $error; - Bugzilla->dbh->bz_unlock_tables(UNLOCK_ABORT) if $unlock_tables; + # Make sure any locked tables are unlocked + # and the transaction is rolled back (if supported) + Bugzilla->dbh->bz_unlock_tables(UNLOCK_ABORT); # If a writable data/errorlog exists, log error details there. if (-w "data/errorlog") { @@ -95,6 +97,10 @@ sub ThrowCodeError { sub ThrowTemplateError { my ($template_err) = @_; + # Make sure any locked tables are unlocked + # and the transaction is rolled back (if supported) + Bugzilla->dbh->bz_unlock_tables(UNLOCK_ABORT); + my $vars = {}; if (Bugzilla->batch) { die("error: template error: $template_err"); @@ -149,16 +155,16 @@ Bugzilla::Error - Error handling utilities for Bugzilla ThrowUserError("error_tag", { foo => 'bar' }); - # supplying "abort" to ensure tables are unlocked - ThrowUserError("another_error_tag", - { foo => 'bar' }, 'abort'); - =head1 DESCRIPTION Various places throughout the Bugzilla codebase need to report errors to the user. The C<Throw*Error> family of functions allow this to be done in a generic and localisable manner. +These functions automatically unlock the database tables, if there were any +locked. They will also roll back the transaction, if it is supported by +the underlying DB. + =head1 FUNCTIONS =over 4 @@ -170,12 +176,6 @@ of variables as a second argument. These are used by the I<global/user-error.html.tmpl> template to format the error, using the passed in variables as required. -An optional third argument may be supplied. If present, the error -handling code will unlock the database tables: it is a Bugzilla standard -to provide the string "abort" as the argument value. In the long term, -this argument will go away, to be replaced by transactional C<rollback> -calls. There is no timeframe for doing so, however. - =item C<ThrowCodeError> This function is used when an internal check detects an error of some sort. @@ -112,7 +112,7 @@ sub CheckFormField (\%$;\@) { $field = $fieldname; } - ThrowCodeError("illegal_field", { field => $field }, "abort"); + ThrowCodeError("illegal_field", { field => $field }); } } @@ -213,7 +213,7 @@ sub CheckEmailSyntax { my ($addr) = (@_); my $match = Param('emailregexp'); if ($addr !~ /$match/ || $addr =~ /[\\\(\)<>&,;:"\[\] \t\r\n]/) { - ThrowUserError("illegal_email_address", { addr => $addr }, 'abort'); + ThrowUserError("illegal_email_address", { addr => $addr }); } } diff --git a/editclassifications.cgi b/editclassifications.cgi index a390ed4c6..deeffdce7 100755 --- a/editclassifications.cgi +++ b/editclassifications.cgi @@ -300,12 +300,10 @@ if ($action eq 'update') { if ($classification ne $classificationold) { unless ($classification) { - $dbh->bz_unlock_tables(UNLOCK_ABORT); ThrowUserError("classification_not_specified") } if (TestClassification($classification)) { - $dbh->bz_unlock_tables(UNLOCK_ABORT); ThrowUserError("classification_already_exists", { name => $classification }); } $sth = $dbh->prepare("UPDATE classifications diff --git a/editcomponents.cgi b/editcomponents.cgi index 12a25905d..d21518fce 100755 --- a/editcomponents.cgi +++ b/editcomponents.cgi @@ -66,7 +66,7 @@ sub CheckProduct ($) # do we have a product? unless ($prod) { - ThrowUserError('product_not_specified'); + ThrowUserError('product_not_specified'); exit; } @@ -585,7 +585,6 @@ if ($action eq 'update') { if ($description ne $descriptionold) { unless ($description) { - $dbh->bz_unlock_tables(UNLOCK_ABORT); ThrowUserError('component_blank_description', {'name' => $componentold}); exit; @@ -603,7 +602,6 @@ if ($action eq 'update') { my $initialownerid = login_to_id($initialowner); unless ($initialownerid) { - $dbh->bz_unlock_tables(UNLOCK_ABORT); ThrowUserError('component_need_valid_initialowner', {'name' => $componentold}); exit; @@ -621,7 +619,6 @@ if ($action eq 'update') { if (Param('useqacontact') && $initialqacontact ne $initialqacontactold) { my $initialqacontactid = login_to_id($initialqacontact); if (!$initialqacontactid && $initialqacontact ne '') { - $dbh->bz_unlock_tables(UNLOCK_ABORT); ThrowUserError('component_need_valid_initialqacontact', {'name' => $componentold}); exit; @@ -638,13 +635,11 @@ if ($action eq 'update') { if ($component ne $componentold) { unless ($component) { - $dbh->bz_unlock_tables(UNLOCK_ABORT); ThrowUserError('component_must_have_a_name', {'name' => $componentold}); exit; } if (TestComponent($product, $component)) { - $dbh->bz_unlock_tables(UNLOCK_ABORT); ThrowUserError('component_already_exists', {'name' => $component}); exit; diff --git a/editmilestones.cgi b/editmilestones.cgi index 7364d4d06..7317e7220 100755 --- a/editmilestones.cgi +++ b/editmilestones.cgi @@ -59,14 +59,14 @@ sub CheckProduct ($) # do we have a product? unless ($product) { - &::ThrowUserError('product_not_specified'); + ThrowUserError('product_not_specified'); exit; } # Does it exist in the DB? unless (TestProduct $product) { - &::ThrowUserError('product_doesnt_exist', - {'product' => $product}); + ThrowUserError('product_doesnt_exist', + {'product' => $product}); exit; } } @@ -506,12 +506,9 @@ if ($action eq 'update') { my $stored_sortkey = $sortkey; if ($sortkey != $sortkeyold) { if (!detaint_natural($sortkey)) { - - $dbh->bz_unlock_tables(UNLOCK_ABORT); ThrowUserError('milestone_sortkey_invalid', {'name' => $milestone, 'sortkey' => $stored_sortkey}); - exit; } @@ -532,12 +529,10 @@ if ($action eq 'update') { if ($milestone ne $milestoneold) { unless ($milestone) { - $dbh->bz_unlock_tables(UNLOCK_ABORT); ThrowUserError('milestone_blank_name'); exit; } if (TestMilestone($product, $milestone)) { - $dbh->bz_unlock_tables(UNLOCK_ABORT); ThrowUserError('milestone_already_exists', {'name' => $milestone, 'product' => $product}); diff --git a/editusers.cgi b/editusers.cgi index 87a8b69bd..8169d0b93 100755 --- a/editusers.cgi +++ b/editusers.cgi @@ -212,8 +212,7 @@ if ($action eq 'search') { canSeeUser($otherUserID) || ThrowUserError('auth_failure', {reason => "not_visible", action => "modify", - object => "user"}, - 'abort'); + object => "user"}); # Cleanups my $login = trim($cgi->param('login') || ''); @@ -231,11 +230,10 @@ if ($action eq 'search') { if ($login ne $loginold) { # Validate, then trick_taint. - $login || ThrowUserError('user_login_required', undef, 'abort'); + $login || ThrowUserError('user_login_required'); CheckEmailSyntax($login); is_available_username($login) || ThrowUserError('account_exists', - {'email' => $login}, - 'abort'); + {'email' => $login}); trick_taint($login); push(@changedFields, 'login_name'); push(@values, $login); @@ -493,19 +491,17 @@ if ($action eq 'search') { 'whine_events WRITE'); Param('allowuserdeletion') - || ThrowUserError('users_deletion_disabled', undef, 'abort'); + || ThrowUserError('users_deletion_disabled'); $editusers || ThrowUserError('auth_failure', {group => "editusers", action => "delete", - object => "users"}, - 'abort'); + object => "users"}); canSeeUser($otherUserID) || ThrowUserError('auth_failure', {reason => "not_visible", action => "delete", - object => "user"}, - 'abort'); + object => "user"}); productResponsibilities($otherUserID) - && ThrowUserError('user_has_responsibility', undef, 'abort'); + && ThrowUserError('user_has_responsibility'); Bugzilla->logout_user_by_id($otherUserID); diff --git a/editversions.cgi b/editversions.cgi index ee4a83d77..60f60057e 100755 --- a/editversions.cgi +++ b/editversions.cgi @@ -406,16 +406,13 @@ if ($action eq 'update') { if ($version ne $versionold) { unless ($version) { - $dbh->bz_unlock_tables(UNLOCK_ABORT); ThrowUserError('version_blank_name'); exit; } if (TestVersion($product,$version)) { - $dbh->bz_unlock_tables(UNLOCK_ABORT); ThrowUserError('version_already_exists', {'name' => $version, 'product' => $product}); - exit; } SendSQL("UPDATE bugs diff --git a/globals.pl b/globals.pl index e71493f6b..44bf7dc3e 100644 --- a/globals.pl +++ b/globals.pl @@ -613,11 +613,11 @@ sub ValidatePassword { my ($password, $matchpassword) = @_; if (length($password) < 3) { - ThrowUserError("password_too_short", undef, 'abort'); + ThrowUserError("password_too_short"); } elsif (length($password) > 16) { - ThrowUserError("password_too_long", undef, 'abort'); + ThrowUserError("password_too_long"); } elsif ((defined $matchpassword) && ($password ne $matchpassword)) { - ThrowUserError("passwords_dont_match", undef, 'abort'); + ThrowUserError("passwords_dont_match"); } } @@ -649,8 +649,7 @@ sub DBNameToIdAndCheck { return $result; } - ThrowUserError("invalid_username", - { name => $name }, "abort"); + ThrowUserError("invalid_username", { name => $name }); } sub get_classification_id { diff --git a/post_bug.cgi b/post_bug.cgi index 6b52b447d..d701a9172 100755 --- a/post_bug.cgi +++ b/post_bug.cgi @@ -318,8 +318,7 @@ if (UserInGroup("editbugs") && defined($::FORM{'dependson'})) { } ThrowUserError("dependency_loop_multi", - { both => $both }, - "abort"); + { both => $both }); } } my $tmp = $me; @@ -378,14 +377,14 @@ foreach my $b (grep(/^bit-\d*$/, keys %::FORM)) { if ($::FORM{$b}) { my $v = substr($b, 4); $v =~ /^(\d+)$/ - || ThrowCodeError("group_id_invalid", undef, "abort"); + || ThrowCodeError("group_id_invalid"); if (!GroupIsActive($v)) { # Prevent the user from adding the bug to an inactive group. # Should only happen if there is a bug in Bugzilla or the user # hacked the "enter bug" form since otherwise the UI # for adding the bug to the group won't appear on that form. $vars->{'bit'} = $v; - ThrowCodeError("inactive_group", undef, "abort"); + ThrowCodeError("inactive_group"); } SendSQL("SELECT user_id FROM user_group_map WHERE user_id = $::userid diff --git a/process_bug.cgi b/process_bug.cgi index ea2180c3c..1fb54d2a4 100755 --- a/process_bug.cgi +++ b/process_bug.cgi @@ -109,7 +109,7 @@ foreach my $field ("estimated_time", "work_time", "remaining_time") { if (UserInGroup(Param('timetrackinggroup'))) { my $wk_time = $::FORM{'work_time'}; if ($::FORM{'comment'} =~ /^\s*$/ && $wk_time && $wk_time != 0) { - ThrowUserError('comment_required', undef, "abort"); + ThrowUserError('comment_required'); } } @@ -241,7 +241,7 @@ if ((($::FORM{'id'} && $::FORM{'product'} ne $::oldproduct) $vars->{'newvalue'} = $::FORM{'product'}; $vars->{'field'} = 'product'; $vars->{'privs'} = $PrivilegesRequired; - ThrowUserError("illegal_change", $vars, "abort"); + ThrowUserError("illegal_change", $vars); } CheckFormField(\%::FORM, 'product', \@::legal_product); @@ -1232,7 +1232,7 @@ foreach my $id (@idlist) { $vars->{'field'} = $col; } $vars->{'privs'} = $PrivilegesRequired; - ThrowUserError("illegal_change", $vars, "abort"); + ThrowUserError("illegal_change", $vars); } } @@ -1251,13 +1251,13 @@ foreach my $id (@idlist) { $vars->{'newvalue'} = "no keywords"; $vars->{'field'} = "keywords"; $vars->{'privs'} = $PrivilegesRequired; - ThrowUserError("illegal_change", $vars, "abort"); + ThrowUserError("illegal_change", $vars); } $oldhash{'product'} = get_product_name($oldhash{'product_id'}); if (!CanEditProductId($oldhash{'product_id'})) { ThrowUserError("product_edit_denied", - { product => $oldhash{'product'} }, "abort"); + { product => $oldhash{'product'} }); } if (defined $::FORM{'product'} @@ -1265,7 +1265,7 @@ foreach my $id (@idlist) { && $::FORM{'product'} ne $oldhash{'product'} && !CanEnterProduct($::FORM{'product'})) { ThrowUserError("entry_access_denied", - { product => $::FORM{'product'} }, "abort"); + { product => $::FORM{'product'} }); } if ($requiremilestone) { # musthavemilestoneonaccept applies only if at least two @@ -1283,7 +1283,7 @@ foreach my $id (@idlist) { # if musthavemilestoneonaccept == 1, then the target # milestone must be different from the default one. if ($value eq $defaultmilestone) { - ThrowUserError("milestone_required", { bug_id => $id }, "abort"); + ThrowUserError("milestone_required", { bug_id => $id }); } } } @@ -1319,7 +1319,7 @@ foreach my $id (@idlist) { next if $i eq ""; if ($id eq $i) { - ThrowUserError("dependency_loop_single", undef, "abort"); + ThrowUserError("dependency_loop_single"); } if (!exists $seen{$i}) { push(@{$deptree{$target}}, $i); @@ -1363,8 +1363,7 @@ foreach my $id (@idlist) { } ThrowUserError("dependency_loop_multi", - { both => $both }, - "abort"); + { both => $both }); } } my $tmp = $me; @@ -1573,6 +1572,7 @@ foreach my $id (@idlist) { shift @oldlist; } else { if ($oldlist[0] != $newlist[0]) { + $dbh->bz_unlock_tables(UNLOCK_ABORT); die "Error in list comparing code"; } shift @oldlist; |