Code cleanup Perl

For good examples of coding, see pm/Taranis/Constituent/*.pm (since release 3.7.0) and scripts/mod_tools/phishing_checker/phishing_overview.pl (since release 3.6.0). Please study that code.

Table of Contents Validation Changes to Database access Changes to Config Perl syntax improvements

Validation

Do check the pattern of parameters very early in any each sub which receives form fields, using val_int and val_text Only a few modules have been rewritten: have a look at the constituent handling and phishing checker for examples how code should look.

Changes to Database access

Module Taranis::DB is to replace Taranis::Database everywhere: it is so much simpler hence safer to use. Do not forget to read the docs on the glorious module DBIx::Simple.

Summarizing the changes which should be applied to existing code:

• From new() remove the creation of a dbh attribute.
• Remove any access to $self->{dbh}
• Remove explicit use of the Sql wrapper for the use SQL::Abstract::More module.
• Replace call to Database into Database->simple
• For records, use new add/get/deleteRecord()
• For images, use new add/get/deleteBlob()
• Replace structures like

my $stmnt = "some query";
Database->prepare( $stmnt );
Database->executeWithBinds( @binds );
my $record = Database->fetchRow();

by

my $record = Database->simple->query("some query", @binds)->hash;

• Replace structures like

my ( $stmnt, @bind ) = $self->{sql}->select( $table, $select, \%where );
$self->{dbh}->prepare( $stmnt );
$self->{dbh}->executeWithBinds( @bind );

while ( $self->nextObject() ) {     #XXX sometimes $self->{dbh}->nextObject
    push ( @records, $self->getObject() );
}

by

my @records = Database->simple->select($table, $select, \%where)->hashes;

See also the new "Taranis::Database::allRecords()". In the ajax handlers, above is often done in two places: partially in the 'pm/'-core, partially in the handler. Move both pieces into a new method inside the core module.

• The 'pm/' modules contain a lot of database error checking which can be removed. In ancient history, the 'RaiseError' flag on the database got set, which made all attempts to gracefully handle errors useless. But the related code did not get cleaned-up Example:

my ( $stmnt, @bind ) = $self->{sql}->delete( $table, $where );
$self->prepare( $stmnt );
my $result = $self->executeWithBinds( @bind )
if ( defined($result) && ( $result !~ m/(0E0)/i ) ) {
    if ( $result > 0 ) {
        return 1;
    } elsif ( defined( $self->{db_error_msg} ) ) {
        return 0;
    }
} else {
    $self->{db_error_msg} = "Delete failed, corresponding id not found in database.";
    return 0;
}

Checking for the number of deleted items here is dangerous: not in a transaction... it happens that other threads remove the item while you are doing this as well. So, first simplification:

my ( $stmnt, @bind ) = $self->{sql}->delete( $table, $where );
$self->prepare( $stmnt );
my $result = $self->executeWithBinds( @bind )
return 1;

Or better:

Database->simple->delete($table, $where);
return 1;

Changes to Config

• Replace constructs like

$log_error_enabled = ( Config->{'syslog'} =~ /^on$/i ) ? 1 : 0;

by

$log_error_enabled = Config->isEnabled('syslog');

Perl syntax improvements

• Do not use the same reference more than once: use helper variables.
• Inline variables which are only used once, unless that adds complication to readibility.
• Switch 'use warnings;' on, per module: may produce many warnings.
• Replace "my (%kvArgs) = @_" by "my %kvArgs = @_", because %kvArgs already enforces LIST context. Yes, kvArgs is a stupid name.
• Replace "($condition) ? 1 : 0" by "$condition || 0". The parenthesis around the condition are superfluous. When setting-up template variables or database fields, you need an explicit '0' instead of 'undef' which is the default for false.
• Replace

sub saveDossierDetails {
    my ( %kvArgs) = @_;
    my ( $dossierID );
  
    if ( $kvArgs{id} =~ /^\d+$/) {
        $dossierID = $kvArgs{id};

by

sub saveDossierDetails(%) {
    my %kvArgs = @_;
    my $dossierID = val_int $kvArgs{id};

    if($dossierID) {

This is safe, because database ids are never zero or undef.

• Remove useless use of indirection:

my ($vars, $tpl);
my $tt = Taranis::Template->new();

if($id) {
    $vars->{something} = 42;
} else {
    $vars->{message} = "No permission";
}

my $html = $tt->processTemplate( $tpl, $vars, 1);

into:

my (%vars, $tpl);

if($id) {
    $vars{something} = 42;
} else {
    $vars{message} = "No permission";
}

my $tt = Taranis::Template->new;
my $html = $tt->processTemplate($tpl, \%vars, 1);