To get rid of it you could either properly initialize the offending variable or turn the warnings off (get rid of "use warnings" or use a lexical "no warnings").

Apart from that you should not use lexicals in a global way.

When you do

sub ownertbl {
    ( $sortby, $ascdesc, $letter ) = @_;
[download]

Instead do it like this:

sub ownertbl {
 my ( $sortby, $ascdesc, $letter ) = @_;
[download]

if i do:

sub ownertbl {
 my ( $sortby......
[download]

i get an error saying that the variable was already defined in the same scope (something to that effect).

also, the warning isn't really the point. it is there telling me i'm screwing up. however, the point is that these values becoming undefined in my sub is causing my sql statement to fail and my page to pretty much fail in its own right.

What do you expect this line of code to do:

I think it's wrong.

i want to need to define all variables within all of my subroutines. i want $sortby to be defined by $_[0] and $ascdesc to be defined by $_[1] etc.

i want these variables defined by the same variables outside of the sub. however, i don't want any changes to these variables to change their counterparts outside of my sub.

Sorry, I've looked at your code several times and it looks like nonsense to me. For example, it makes no sense to pass $sortby into sub ownertbl and then assign it to itself.

I suggest you start your script with:

use strict;
use warnings;
[download]

use strict;
use warnings;

# Put your subs here before any file-scope variables.
# That makes it clearer that each sub does not depend on
# anything other than the parameters passed to it.

sub ownertbl {
  my ( $sortby, $ascdesc, $letter ) = @_;    # added leading "my"
  # ...
}

# put your other subs here...

# Now define your lexical variables after the subs...
my ($ownsel, $sortby, $letter, $ascdesc, $t);

if ( $cgi->param ) {
  if ( defined ( $cgi->param( 'sortby' ) ) ) {
     $sortby = $_;    # this looks wrong to me
  } else {
     $sortby = "name";
  }
  if (defined ( $cgi->param( 'ascdesc' ) ) ) {
     $ascdesc = $_;   # ditto
  } else {
     $ascdesc =  "desc";
  }
  if (defined ( $cgi->param( 'letter' ) ) ) {
     $letter = $_;    # ditto
  } else {
     $letter = " ";
  }
  ownertbl( $sortby, $ascdesc, $letter );  # now these three variables
+ are properly passed to the sub
} else {
  homepage();
}
[download]

ok, i thought this would be overkill but below is what i have (i have changed some names to protect the innocent but it is all intact). the html::template works fine. and i'd prefer to keep two templates so that i can do some type of js on the results page to popup the search page (though i suppose i could do it with one and use an tmpl_if, doing them separate just seemed logical and would do slightly less processing).

also, i think you have a point of if not defining $_ so i'll change it (might be my issue). though i still think i'm off on my variable scopes...

you might have a point in defining my subs with my variables first, i just figured i'd go ahead and post the whole thing to confirm:

also, there is no need to have the sql connect and disconnect at the beginning and end, but it works for now so until i get this figured out, i figured i'd keep this as is. and there are major security issues with this right now that i'll take care of before i put this up. with those two points aside, i'll take any suggestions you have to make this work / simpler / better.

#!/usr/bin/perl -w


use strict;
use DBI;
use CGI qw( -no_undef_params :standard);
use HTML::Template;

my $param = "";
my ($ownsel, $sortby, $letter, $ascdesc, $t);

my $dbh = DBI->connect('DBI:mysql:dbname;host=localhost',
        'someone', 'something')
    or die "Database connection: $!";
my $cgi = CGI->new;

my $base = $cgi->url;

if ( $cgi->param ) {
    if ( defined ( $cgi->param( 'sortby' ) ) ) {
     $sortby = $_;
  } else {
     $sortby = "name";
  }
    if (defined ( $cgi->param( 'ascdesc' ) ) ) {
     $ascdesc = $_;
  } else {
     $ascdesc =  "desc";
  }
    if (defined ( $cgi->param( 'letter' ) ) ) {
     $letter = $_;
  } else {
     $letter = " ";
  }
    ownertbl( $sortby, $ascdesc, $letter );
} else {
    homepage();
}

sub homepage {
    
    $t = HTML::Template->new( filename => 'home.tmpl',
                        global_vars => '1' 
                  );

    my @alphabet = ();
    for ("A" .. "Z") {
        my %letter;
          $letter { 'letter' } = $_;
        push(@alphabet, \%letter);
    }

    $t->param( alphabet => \@alphabet,
                base => $base,
                sortby => $sortby,
                ascdesc => $ascdesc,
                letter => $letter
            );

    print $cgi->header;
    print $t->output;
}


sub ownertbl {

    ( $sortby, $ascdesc, $letter ) = @_;

    my $sort = "";

    $sort = "ORDER BY " . $sortby . $ascdesc;

  if ( defined ( $letter ) ) {
     $letter = "WHERE " . $sortby . " LIKE '" . $letter . "%'";
  } else {
     $letter = " ";
  }

    my $query = "SELECT field1, field2, field3, field4, field5 
            FROM table
            $sort $letter";

    my $sth = $dbh->prepare( $query );

    $sth->execute( );

    my @ownsel = ();
    push @{$ownsel}, $_ while $_ = $sth->fetchrow_hashref();

    $t = HTML::Template->new( filename => 'ownertbl.tmpl' );

    $t->param(ownerall => \@ownsel,
                base => $base,
                letter => $letter
            );

    print STDERR "base = $base, sort = $sort, sortby = $sortby, ascdes
+c = $ascdesc, letter = $letter\n";
    print $cgi->header;
    print $t->output;
}


$dbh->disconnect;
[download]

When you're initializing your variables, using separate if() blocks is verbose. This is a good place to use a defined-or-expression, a // b, if you have Perl5.10 or later, or a || b for Perl5.8, where 'a' is the default you want to assign if 'a' wasn't specified.

a || b means: if 'a' is true in the boolean sense, use the value 'a', otherwise use 'b'. Boolean false is 'undef', 'zero' and the empty string, everything else is true.. If you legitimately want to pass zero or an empty string as an argument, the Perl5.8 '||' fails, which is why the newer '//' was invented. a // b reads: If 'a' is defined, use 'a', otherwise use 'b'. The Perl5.8 way to achieve that is to use a ternary expression: defined a ? a : b .. if the test in the first chunk is tru, use the value after the question mark '?', otherwise use the chunk after the colon ':'.

The // is often pronounced 'err' cause it's useful for detecting error situations.

#modern way
#
my $sortby = $cgi->param( 'sortby' ) // 'name';

# Perl5.8 way
#
my $sortby = $cgi->param( 'sortby' ) || 'name';

# If first value can be zero or empty string ..
#
my $sortby = ( defined $cgi->param( 'sortby' ) ? $cgi->param( 'sortby'
+ )
             :                                   'name'
             );

# alternate arrangement
#
my $sortby = ( defined $cgi->param( 'sortby' ) 
             ? $cgi->param( 'sortby' )
             : 'name'
             );
[download]

With the 'or' or the 'err', declaring and initializing three variables takes three lines, and the repetition reinforces the concept of what's happening. The ternary condition is lengthier, but still 9 lines instead of 15. You don't actually need the parentheses, I use them because emacs will then line up the next line under the opening parenthesis. In this case I think the entire statement could fit on one line. When it's too long, I prefer the alternate arrangement, the components are clearer: if the test is is true, use line 2, else use line 3. Damian Conway's Perl Best Practices recommends the first version, because the test is out front, and the values are stacked. It's definitely better if you are chaining tests:

my $variable = test1() ? value1
              : test2() ? value2
              : test3() ? value3
              :           default_value
              ;
[download]

On a slightly different angle, I would suggest grouping variables into a structure. Those three variables are all components of the SQL generation process.

use Readonly;
Readonly my %sql_defaults => ( sortby => 'name',
                               ascdesc => 'desc',
                               letter => q{ },
                              );
my %sql_args;
$sql_args{sortby}  = $cgi->param( 'sortby' )  || $sql_default{sortby};
$sql_args{ascdesc} = $cgi->param( 'ascdesc' ) || $sql_default{ascdesc}
+;
$sql_args{letter}  = $cgi->param( 'letter' )  || $sql_default{letter};

# alternately, use a suffix 'for' to merge the three lines:
# 'qw()' does 'quote words'
#
$sql_args{$_} = $cgi->param( $_ ) || $sql_default{$_}
    for qw( sortby ascdesc letter );
[download]

The one problem with using hashes or arrays, is you have to pass a reference in the subroutine call, \%sql_args, rather than the plain hash. In the subroutine, that gets assigned to an ordinary scalar variable $sql_args, and you have to use an arrow to access the components, $sql_args->{sortby}.

While suggesting style improvements, I've pointed the way to fixing your bug: assigning from the $cgi->param() routine, rather than from $_. if() tests do not assign to $_.

As for the process of invoking a subroutine, when you call a subroutine with arguments a, b, c, the value of the variable are assigned into the array @_. In the subroutine, you need to assign teh array into individual variables. But you're assigning them to the global variables they came from! You need a my> before the parentheses to declare new local variables which happen to have the same name as th global variables.

nice. thanks for all of that.

it was late (err early), so i missed chromatic et al's suggestion that if didn't reassign $_. however, the if was getting unwieldy and just didn't look good assigning the param to the scalar so i didn't want to do it like that. in the end, i did this:

if ( $cgi->param ) {
  $sortby = defined ( $cgi->param( 'sortby' ) ) ? 
     $cgi->param( 'sortby' ) : "name";
  $ascdesc = defined ( $cgi->param( 'ascdesc' ) ) ? 
     $cgi->param( 'ascdesc' ) : "desc";
  $letter = defined ( $cgi->param( 'letter' ) ) ? 
     $cgi->param( 'ascdesc' ) : " ";
    ownertbl( $sortby, $ascdesc, $letter );
[download]

which works fairly well but always ends up true (i think) this is because i am defining my cgi vars in all requests to keep my state (so that if one thing changes everything else doesn't go away). maybe i'll try and test  $($cgi->param( 'whatever' )) > 0 and see if i can make this work.

my other issue is the sql paren is doubling up per:

[Sat Oct 16 11:04:23 2010] [error] [client 192.168.15.177] DBD::mysql:
+:st execute failed: You have an error in your SQL syntax; check the m
+anual that corresponds to your MySQL server version for the right syn
+tax to use near 'WHERE name LIKE '%'' at line 3 at /var/www/cgi-bin/o
+wnertbl.pl line 76., referer: http://192.168.15.222/cgi-bin/ownertbl.
+pl
[Sat Oct 16 11:04:23 2010] [error] [client 192.168.15.177] DBD::mysql:
+:st fetchrow_hashref failed: fetch() without execute() at /var/www/cg
+i-bin/ownertbl.pl line 79., referer: http://192.168.15.222/cgi-bin/ow
+nertbl.pl
[Sat Oct 16 11:04:23 2010] [error] [client 192.168.15.177] base = http
+://192.168.15.222/cgi-bin/ownertbl.pl, sort = ORDER BY name, sortby =
+ name, ascdesc = , letter = WHERE name LIKE '%', referer: http://192.
+168.15.222/cgi-bin/ownertbl.pl
[download]

but, i haven't really studied your sql suggestions so i'm hoping with a closer look at that i'll be able to fix this too.

thanks for the help

Whenever I see this in the httpd error log:

You have an error in your SQL syntax; check the manual ... for the right syntax to use near '...'

I know that the actual mistake usually precedes the part that is being quoted in the error message. So the next thing I do is to add something like this to the program, just before the $dbh->prepare( $sql ) call:

warn "$0: preparing sql: $sql\n";
[download]

Then I watch the error log to see what the whole sql statement text looks like, and this makes it easier to spot where I went wrong in putting it together.

In your case (based on other code you've posted earlier in this thread), I'm guessing that you have your "ORDER BY ..." and "WHERE ..." clauses in the wrong order ("WHERE ..." is supposed to come before "ORDER BY ...").