From 59bc1dc86af4d8ad60b738c73052b7051dbd2d16 Mon Sep 17 00:00:00 2001 From: Vitaliy Filippov Date: Tue, 7 Oct 2014 18:24:25 +0400 Subject: [PATCH] Bugzilla::Chart, Bugzilla::Series, chart.cgi: remove CGI usage --- Bugzilla/Chart.pm | 40 +++-- Bugzilla/Component.pm | 12 +- Bugzilla/Install/DB.pm | 22 ++- Bugzilla/Product.pm | 13 +- Bugzilla/Series.pm | 153 ++++++------------- chart.cgi | 121 ++++++++------- template/en/default/reports/series.html.tmpl | 2 +- 7 files changed, 165 insertions(+), 198 deletions(-) diff --git a/Bugzilla/Chart.pm b/Bugzilla/Chart.pm index 0cd257b23..d1e397b2d 100644 --- a/Bugzilla/Chart.pm +++ b/Bugzilla/Chart.pm @@ -42,25 +42,23 @@ use List::Util qw(max); sub new { my $invocant = shift; my $class = ref($invocant) || $invocant; - - # Create a ref to an empty hash and bless it - my $self = {}; - bless($self, $class); + my ($params) = @_; - if ($#_ == 0) { - # Construct from a CGI object. - $self->init($_[0]); - } - else { - die("CGI object not passed in - invalid number of args \($#_\)($_)"); + # Create a ref to an empty hash and bless it + my $self = bless {}, $class; + + if (!$params) + { + die(__PACKAGE__ . "->new: missing parameters"); } + $self->init($params); return $self; } sub init { my $self = shift; - my $cgi = shift; + my ($params) = @_; # The data structure is a list of lists (lines) of Series objects. # There is a separate list for the labels. @@ -71,10 +69,10 @@ sub init { # &select0=1&select3=1... # &cumulate=1&datefrom=2002-02-03&dateto=2002-04-04&ctype=html... # >=1&labelgt=Grand+Total - foreach my $param ($cgi->param()) { + foreach my $param (keys %$params) { # Store all the lines if ($param =~ /^line(\d+)$/) { - foreach my $series_id ($cgi->param($param)) { + foreach my $series_id (list $params->{$param}) { detaint_natural($series_id) || ThrowCodeError("invalid_series_id"); my $series = new Bugzilla::Series($series_id); @@ -84,16 +82,16 @@ sub init { # Store all the labels if ($param =~ /^label(\d+)$/) { - $self->{'labels'}[$1] = $cgi->param($param); + $self->{'labels'}[$1] = $params->{$param}; } } # Store the miscellaneous metadata - $self->{'cumulate'} = $cgi->param('cumulate') ? 1 : 0; - $self->{'gt'} = $cgi->param('gt') ? 1 : 0; - $self->{'labelgt'} = $cgi->param('labelgt'); - $self->{'datefrom'} = $cgi->param('datefrom'); - $self->{'dateto'} = $cgi->param('dateto'); + $self->{'cumulate'} = $params->{cumulate} ? 1 : 0; + $self->{'gt'} = $params->{gt} ? 1 : 0; + $self->{'labelgt'} = $params->{labelgt}; + $self->{'datefrom'} = $params->{datefrom}; + $self->{'dateto'} = $params->{dateto}; # If we are cumulating, a grand total makes no sense $self->{'gt'} = 0 if $self->{'cumulate'}; @@ -111,8 +109,8 @@ sub init { $self->{'datefrom'} > $self->{'dateto'}) { ThrowUserError("misarranged_dates", - {'datefrom' => $cgi->param('datefrom'), - 'dateto' => $cgi->param('dateto')}); + {'datefrom' => $params->{datefrom}, + 'dateto' => $params->{dateto}}); } } diff --git a/Bugzilla/Component.pm b/Bugzilla/Component.pm index f3ab3d71b..9894a9bb8 100644 --- a/Bugzilla/Component.pm +++ b/Bugzilla/Component.pm @@ -342,10 +342,14 @@ sub _create_series foreach my $sdata (@series) { - my $series = new Bugzilla::Series( - undef, $self->product->name, $self->name, $sdata->[0], - Bugzilla->user->id, 1, $sdata->[1], 1 - ); + my $series = new Bugzilla::Series({ + category => $self->product->name, + subcategory => $self->name, + name => $sdata->[0], + frequency => 1, + query => $sdata->[1], + public => 1, + }); $series->writeToDatabase(); } } diff --git a/Bugzilla/Install/DB.pm b/Bugzilla/Install/DB.pm index cd04c1cd3..34a4627a4 100644 --- a/Bugzilla/Install/DB.pm +++ b/Bugzilla/Install/DB.pm @@ -2198,9 +2198,14 @@ sub _copy_old_charts_into_database { foreach my $field (@fields) { # Create a Series for each field in this product. - my $series = new Bugzilla::Series(undef, $product, $all_name, - $field, undef, 1, - $queries{$field}, 1); + my $series = new Bugzilla::Series({ + category => $product, + subcategory => $all_name, + name => $field, + frequency => 1, + query => $queries{$field}, + public => 1, + }); $series->writeToDatabase(); $seriesids{$field} = $series->{'series_id'}; } @@ -2209,9 +2214,14 @@ sub _copy_old_charts_into_database { # the same set as new products (see editproducts.cgi.) my @openedstatuses = ("UNCONFIRMED", "NEW", "ASSIGNED", "REOPENED"); my $query = join("&", map { "bug_status=$_" } @openedstatuses); - my $series = new Bugzilla::Series(undef, $product, $all_name, - $open_name, undef, 1, - $query_prod . $query, 1); + my $series = new Bugzilla::Series({ + category => $product, + subcategory => $all_name, + name => $open_name, + frequency => 1, + query => $query_prod . $query, + public => 1, + }); $series->writeToDatabase(); $seriesids{$open_name} = $series->{'series_id'}; diff --git a/Bugzilla/Product.pm b/Bugzilla/Product.pm index 45d0eb3e6..1a48d7bfc 100644 --- a/Bugzilla/Product.pm +++ b/Bugzilla/Product.pm @@ -653,11 +653,14 @@ sub _create_series foreach my $sdata (@series) { - my $series = new Bugzilla::Series( - undef, $self->name, get_text('series_subcategory'), - $sdata->[0], Bugzilla->user->id, 1, - $sdata->[1] . "&product=" . url_quote($self->name), 1 - ); + my $series = new Bugzilla::Series({ + category => $self->name, + subcategory => get_text('series_subcategory'), + name => $sdata->[0], + frequency => 1, + query => $sdata->[1] . "&product=" . url_quote($self->name), + public => 1, + }); $series->writeToDatabase(); } } diff --git a/Bugzilla/Series.pm b/Bugzilla/Series.pm index b6b72f9da..c0ff9e40a 100644 --- a/Bugzilla/Series.pm +++ b/Bugzilla/Series.pm @@ -34,137 +34,78 @@ package Bugzilla::Series; use Bugzilla::Error; use Bugzilla::Util; -sub new { +sub new +{ my $invocant = shift; my $class = ref($invocant) || $invocant; + my ($param) = @_; - # Create a ref to an empty hash and bless it - my $self = {}; - bless($self, $class); + my $self = bless {}, $class; - my $arg_count = scalar(@_); - - # new() can return undef if you pass in a series_id and the user doesn't - # have sufficient permissions. If you create a new series in this way, - # you need to check for an undef return, and act appropriately. - my $retval = $self; - - # There are three ways of creating Series objects. Two (CGI and Parameters) - # are for use when creating a new series. One (Database) is for retrieving - # information on existing series. - if ($arg_count == 1) { - if (ref($_[0])) { - # We've been given a CGI object to create a new Series from. - # This series may already exist - external code needs to check - # before it calls writeToDatabase(). - $self->initFromCGI($_[0]); - } - else { - # We've been given a series_id, which should represent an existing - # Series. - $retval = $self->initFromDatabase($_[0]); - } + if ($param =~ /^(\d+)$/so) + { + # We've been given a series_id, which should represent an existing Series. + $self = $self->initFromDatabase($1); } - elsif ($arg_count >= 6 && $arg_count <= 8) { - # We've been given a load of parameters to create a new Series from. - # Currently, undef is always passed as the first parameter; this allows - # you to call writeToDatabase() unconditionally. - # XXX - You cannot set category_id and subcategory_id from here. - $self->initFromParameters(@_); - } - else { - die("Bad parameters passed in - invalid number of args: $arg_count"); + else + { + $self->set_all($param); } - return $retval; + return $self; } -sub initFromDatabase { +sub initFromDatabase +{ my ($self, $series_id) = @_; my $dbh = Bugzilla->dbh; my $user = Bugzilla->user; - detaint_natural($series_id) - || ThrowCodeError("invalid_series_id", { 'series_id' => $series_id }); + detaint_natural($series_id) || ThrowCodeError("invalid_series_id", { series_id => $series_id }); my $grouplist = $user->groups_as_string; - my @series = $dbh->selectrow_array("SELECT series.series_id, cc1.name, " . - "cc2.name, series.name, series.creator, series.frequency, " . - "series.query, series.is_public, series.category, series.subcategory " . - "FROM series " . - "INNER JOIN series_categories AS cc1 " . - " ON series.category = cc1.id " . - "INNER JOIN series_categories AS cc2 " . - " ON series.subcategory = cc2.id " . - "LEFT JOIN category_group_map AS cgm " . - " ON series.category = cgm.category_id " . - " AND cgm.group_id NOT IN($grouplist) " . - "WHERE series.series_id = ? " . - " AND (creator = ? OR (is_public = 1 AND cgm.category_id IS NULL))", - undef, ($series_id, $user->id)); + my $rows = $dbh->selectall_arrayref( + "SELECT s.series_id, cc1.name category, cc2.name subcategory, s.name," . + " s.creator creator_id, s.frequency, s.query, s.is_public public," . + " s.category category_id, s.subcategory subcategory_id" . + " FROM series s" . + " INNER JOIN series_categories cc1 ON s.category = cc1.id" . + " INNER JOIN series_categories cc2 ON s.subcategory = cc2.id" . + " LEFT JOIN category_group_map cgm ON s.category = cgm.category_id" . + " AND cgm.group_id NOT IN ($grouplist) " . + " WHERE s.series_id = ? AND (s.creator = ? OR (s.is_public = 1 AND cgm.category_id IS NULL))", + {Slice=>{}}, $series_id, $user->id + ); - if (@series) { - $self->initFromParameters(@series); + if (@$rows) + { + %$self = %{$rows->[0]}; return $self; } - else { - return undef; - } + + return undef; } -sub initFromParameters { - # Pass undef as the first parameter if you are creating a new series. +sub set_all +{ my $self = shift; + my ($params) = @_; - ($self->{'series_id'}, $self->{'category'}, $self->{'subcategory'}, - $self->{'name'}, $self->{'creator_id'}, $self->{'frequency'}, - $self->{'query'}, $self->{'public'}, $self->{'category_id'}, - $self->{'subcategory_id'}) = @_; - - # If the first parameter is undefined, check if this series already - # exists and update it series_id accordingly - $self->{'series_id'} ||= $self->existsInDatabase(); -} - -sub initFromCGI { - my $self = shift; - my $cgi = shift; - - $self->{'series_id'} = $cgi->param('series_id') || undef; - if (defined($self->{'series_id'})) { - detaint_natural($self->{'series_id'}) - || ThrowCodeError("invalid_series_id", - { 'series_id' => $self->{'series_id'} }); + $self->{category} = $params->{category} || ThrowUserError("missing_category"); + $self->{subcategory} = $params->{subcategory} || ThrowUserError("missing_subcategory"); + $self->{name} = $params->{name} || ThrowUserError("missing_name"); + $self->{creator_id} = Bugzilla->user->id || undef; + $self->{frequency} = $params->{frequency}; + detaint_natural($self->{frequency}) || ThrowUserError("missing_frequency"); + if (exists $params->{query}) + { + $self->{query} = $params->{query}; + trick_taint($self->{query}); } + $self->{public} = 1 && $params->{public}; - $self->{'category'} = $cgi->param('category') - || $cgi->param('newcategory') - || ThrowUserError("missing_category"); - - $self->{'subcategory'} = $cgi->param('subcategory') - || $cgi->param('newsubcategory') - || ThrowUserError("missing_subcategory"); - - $self->{'name'} = $cgi->param('name') - || ThrowUserError("missing_name"); - - $self->{'creator_id'} = Bugzilla->user->id; - - $self->{'frequency'} = $cgi->param('frequency'); - detaint_natural($self->{'frequency'}) - || ThrowUserError("missing_frequency"); - - $self->{'query'} = $cgi->canonicalise_query("format", "ctype", "action", - "category", "subcategory", "name", - "frequency", "public", "query_format"); - trick_taint($self->{'query'}); - - $self->{'public'} = $cgi->param('public') ? 1 : 0; - - # Change 'admin' here and in series.html.tmpl, or remove the check - # completely, if you want to change who can make series public. - $self->{'public'} = 0 unless Bugzilla->user->in_group('admin'); + $self->{series_id} ||= $self->existsInDatabase(); } sub writeToDatabase { diff --git a/chart.cgi b/chart.cgi index 46ea50ffa..e3a4c6e15 100755 --- a/chart.cgi +++ b/chart.cgi @@ -48,7 +48,6 @@ use lib qw(. lib); use Bugzilla; use Bugzilla::Constants; -use Bugzilla::CGI; use Bugzilla::Error; use Bugzilla::Util; use Bugzilla::Chart; @@ -56,13 +55,9 @@ use Bugzilla::Series; use Bugzilla::User; use Bugzilla::Token; -# For most scripts we don't make $cgi and $template global variables. But -# when preparing Bugzilla for mod_perl, this script used these -# variables in so many subroutines that it was easier to just -# make them globals. -local our $cgi = Bugzilla->cgi; local our $template = Bugzilla->template; local our $vars = {}; +my $ARGS = Bugzilla->input_params; my $dbh = Bugzilla->dbh; my $user = Bugzilla->login(LOGIN_REQUIRED); @@ -72,22 +67,23 @@ if (!Bugzilla->feature('new_charts')) { } # Go back to query.cgi if we are adding a boolean chart parameter. -if (grep(/^cmd-/, $cgi->param())) { - my $params = $cgi->canonicalise_query("format", "ctype", "action"); - print $cgi->redirect("query.cgi?format=" . $cgi->param('query_format') . - ($params ? "&$params" : "")); +if (grep /^cmd-/, keys %$ARGS) +{ + delete $ARGS->{$_} for qw(format ctype action); + $ARGS->{format} = $ARGS->{query_format}; + print Bugzilla->cgi->redirect('query.cgi?' . http_build_query($ARGS)); exit; } -my $action = $cgi->param('action'); -my $series_id = $cgi->param('series_id'); +my $action = $ARGS->{action}; +my $series_id = $ARGS->{series_id}; $vars->{'doc_section'} = 'reporting.html#charts'; # Because some actions are chosen by buttons, we can't encode them as the value # of the action param, because that value is localization-dependent. So, we # encode it in the name, as "action-". Some params even contain the # series_id they apply to (e.g. subscribe, unsubscribe). -my @actions = grep(/^action-/, $cgi->param()); +my @actions = grep /^action-/, keys %$ARGS; if ($actions[0] && $actions[0] =~ /^action-([^\d]+)(\d*)$/) { $action = $1; $series_id = $2 if $2; @@ -97,8 +93,9 @@ $action ||= "assemble"; # Go to buglist.cgi if we are doing a search. if ($action eq "search") { - my $params = $cgi->canonicalise_query("format", "ctype", "action"); - print $cgi->redirect("buglist.cgi" . ($params ? "?$params" : "")); + delete $ARGS->{$_} for qw(format ctype action); + my $params = http_build_query($ARGS); + print Bugzilla->cgi->redirect("buglist.cgi" . ($params ? "?$params" : "")); exit; } @@ -108,7 +105,7 @@ $user->in_group(Bugzilla->params->{"chartgroup"}) object => "charts"}); # Only admins may create public queries -$user->in_group('admin') || $cgi->delete('public'); +$user->in_group('admin') || delete $ARGS->{public}; # All these actions relate to chart construction. if ($action =~ /^(assemble|add|remove|sum|subscribe|unsubscribe)$/) { @@ -120,7 +117,7 @@ if ($action =~ /^(assemble|add|remove|sum|subscribe|unsubscribe)$/) { $series->$action($user->id); } - my $chart = new Bugzilla::Chart($cgi); + my $chart = new Bugzilla::Chart($ARGS); if ($action =~ /^remove|sum$/) { $chart->$action(getSelectedLines()); @@ -137,7 +134,7 @@ elsif ($action eq "plot") { } elsif ($action eq "wrap") { # For CSV "wrap", we go straight to "plot". - if ($cgi->param('ctype') && $cgi->param('ctype') eq "csv") { + if ($ARGS->{ctype} && $ARGS->{ctype} eq "csv") { plot(); } else { @@ -145,20 +142,28 @@ elsif ($action eq "wrap") { } } elsif ($action eq "create") { - assertCanCreate($cgi); - my $token = $cgi->param('token'); - check_hash_token($token, ['create-series']); - - my $series = new Bugzilla::Series($cgi); + assertCanCreate(); + check_hash_token($ARGS->{token}, ['create-series']); + + my $q = { %$ARGS }; + delete $q->{$_} for qw(series_id category newcategory subcategory newsubcategory name frequency public); + my $series = new Bugzilla::Series({ + category => $ARGS->{category}, + subcategory => $ARGS->{subcategory}, + name => $ARGS->{name}, + frequency => $ARGS->{frequency}, + public => $ARGS->{public}, + query => http_build_query($q), + }); ThrowUserError("series_already_exists", {'series' => $series}) - if $series->existsInDatabase; + if $series->existsInDatabase; $series->writeToDatabase(); $vars->{'message'} = "series_created"; $vars->{'series'} = $series; - my $chart = new Bugzilla::Chart($cgi); + my $chart = new Bugzilla::Chart($ARGS); view($chart); } elsif ($action eq "edit") { @@ -167,25 +172,30 @@ elsif ($action eq "edit") { } elsif ($action eq "alter") { my $series = assertCanEdit($series_id); - my $token = $cgi->param('token'); - check_hash_token($token, [$series->id, $series->name]); - # XXX - This should be replaced by $series->set_foo() methods. - $series = new Bugzilla::Series($cgi); + check_hash_token($ARGS->{token}, [ $series->id, $series->name ]); + + $series = new Bugzilla::Series($series_id); + $series->set_all({ + category => $ARGS->{newcategory}, + subcategory => $ARGS->{newsubcategory}, + name => $ARGS->{name}, + frequency => $ARGS->{frequency}, + public => $ARGS->{public}, + }); # We need to check if there is _another_ series in the database with # our (potentially new) name. So we call existsInDatabase() to see if # the return value is us or some other series we need to avoid stomping # on. my $id_of_series_in_db = $series->existsInDatabase(); - if (defined($id_of_series_in_db) && - $id_of_series_in_db != $series->{'series_id'}) + if (defined($id_of_series_in_db) && $id_of_series_in_db != $series->{'series_id'}) { ThrowUserError("series_already_exists", {'series' => $series}); } - + $series->writeToDatabase(); $vars->{'changes_saved'} = 1; - + edit($series); } elsif ($action eq "confirm-delete") { @@ -195,8 +205,7 @@ elsif ($action eq "confirm-delete") { } elsif ($action eq "delete") { my $series = assertCanEdit($series_id); - my $token = $cgi->param('token'); - check_hash_token($token, [$series->id, $series->name]); + check_hash_token($ARGS->{token}, [$series->id, $series->name]); $dbh->bz_start_transaction(); @@ -217,7 +226,7 @@ elsif ($action eq "delete") { view(); } elsif ($action eq "convert_search") { - my $saved_search = $cgi->param('series_from_search') || ''; + my $saved_search = $ARGS->{series_from_search} || ''; my ($query) = grep { $_->name eq $saved_search } @{ $user->queries }; my $url = ''; if ($query) { @@ -226,7 +235,7 @@ elsif ($action eq "convert_search") { delete $params->{$_} for ('format', 'query_format'); $url = '&' . html_quote(http_build_query($params)); } - print $cgi->redirect(-location => correct_urlbase() . "query.cgi?format=create-series$url"); + print Bugzilla->cgi->redirect(-location => correct_urlbase() . "query.cgi?format=create-series$url"); } else { ThrowCodeError("unknown_action"); @@ -236,14 +245,14 @@ exit; # Find any selected series and return either the first or all of them. sub getAndValidateSeriesIDs { - my @series_ids = grep(/^\d+$/, $cgi->param("name")); + my @series_ids = grep(/^\d+$/, list Bugzilla->input_params->{name}); return wantarray ? @series_ids : $series_ids[0]; } # Return a list of IDs of all the lines selected in the UI. sub getSelectedLines { - my @ids = map { /^select(\d+)$/ ? $1 : () } $cgi->param(); + my @ids = map { /^select(\d+)$/ ? $1 : () } keys %{ Bugzilla->input_params }; return @ids; } @@ -265,21 +274,20 @@ sub assertCanEdit { # Check if the user is permitted to create this series with these parameters. sub assertCanCreate { - my ($cgi) = shift; my $user = Bugzilla->user; $user->in_group("editbugs") || ThrowUserError("illegal_series_creation"); # Check permission for frequency my $min_freq = 7; - if ($cgi->param('frequency') < $min_freq && !$user->in_group("admin")) { + if (Bugzilla->input_params->{frequency} < $min_freq && !$user->in_group("admin")) { ThrowUserError("illegal_frequency", { 'minimum' => $min_freq }); } } sub validateWidthAndHeight { - $vars->{'width'} = $cgi->param('width'); - $vars->{'height'} = $cgi->param('height'); + $vars->{'width'} = Bugzilla->input_params->{width}; + $vars->{'height'} = Bugzilla->input_params->{height}; if (defined($vars->{'width'})) { (detaint_natural($vars->{'width'}) && $vars->{'width'} > 0) @@ -312,17 +320,19 @@ sub edit { sub plot { validateWidthAndHeight(); - $vars->{'chart'} = new Bugzilla::Chart($cgi); - my $format = $template->get_format("reports/chart", "", scalar($cgi->param('ctype'))); + my $ARGS = Bugzilla->input_params; + $vars->{'chart'} = new Bugzilla::Chart($ARGS); + + my $format = $template->get_format("reports/chart", "", $ARGS->{ctype}); # Debugging PNGs is a pain; we need to be able to see the error messages - if ($cgi->param('debug')) { - $cgi->send_header(); - $vars->{'chart'}->dump(); + if ($ARGS->{debug}) { + Bugzilla->cgi->send_header(); + $vars->{chart}->dump(); } - $cgi->send_header($format->{'ctype'}); + Bugzilla->cgi->send_header($format->{'ctype'}); disable_utf8() if ($format->{'ctype'} =~ /^image\//); $template->process($format->{'template'}, $vars) @@ -332,13 +342,13 @@ sub plot { sub wrap { validateWidthAndHeight(); - # We create a Chart object so we can validate the parameters - my $chart = new Bugzilla::Chart($cgi); + my $chart = new Bugzilla::Chart(Bugzilla->input_params); $vars->{'time'} = localtime(time()); - $vars->{'imagebase'} = $cgi->canonicalise_query( - "action", "action-wrap", "ctype", "format", "width", "height"); + my $q = { %{ Bugzilla->input_params } }; + delete $q->{$_} for qw(action action-wrap ctype format width height); + $vars->{'imagebase'} = http_build_query($q); $template->process("reports/chart.html.tmpl", $vars) || ThrowTemplateError($template->error()); @@ -347,9 +357,10 @@ sub wrap { sub view { my $chart = shift; + my $ARGS = Bugzilla->input_params; # Set defaults foreach my $field ('category', 'subcategory', 'name', 'ctype') { - $vars->{'default'}{$field} = $cgi->param($field) || 0; + $vars->{'default'}{$field} = $ARGS->{$field} || 0; } # Pass the state object to the display UI. @@ -358,7 +369,7 @@ sub view { # If we have having problems with bad data, we can set debug=1 to dump # the data structure. - $chart->dump() if $cgi->param('debug'); + $chart->dump() if $ARGS->{debug}; $template->process("reports/create-chart.html.tmpl", $vars) || ThrowTemplateError($template->error()); diff --git a/template/en/default/reports/series.html.tmpl b/template/en/default/reports/series.html.tmpl index 3cf939003..2b5f9a847 100644 --- a/template/en/default/reports/series.html.tmpl +++ b/template/en/default/reports/series.html.tmpl @@ -68,7 +68,7 @@ [%# Change 'admin' here and in Series.pm, or remove the check completely, if you want to change who can make series public. %] [% IF user.in_group('admin') %] - Visible to all
(within group restrictions)