My preference is always to put the mainline inside braces to make it clearer what's going on. In addition, the conditionals you havein your mainline could be organized a little better:

  if ( $vars { action } )
  {
    if ( $vars{action} eq "news" )
    {
      &news if (!$vars{article});
      &show_news if ($vars{article} || $vars{editnews});
    }
    elsif ( $vars{action} eq "editnews" )
    {
      &edit_news($article_title, $article_body);
    }
  }
  else
  {
    &left_content();
    &right_content();
  }
[download]

Second .. what SQL is being generated? Are you sure that it's correct? Perhaps try turning on DBI->trace to see what DBI is doing.

--t. alex

"Nyahhh (munch, munch) What's up, Doc?" --Bugs Bunny

Update Fixed some typos.

To solve this problem, you could either stop using Registry and write normal modules, or switch your subs to accept passed parameters instead of referring to these lexicals that are declared outside of their scope.

Also, your two calls to use CGI are redundant. You can remove the first one.

Your subroutines are constantly acting on globals. This makes it very hard to debug anything, you also need to see the entire script in that case. With mod_perl you also need to remeber that Perl is persistant, check the guide to see how this can effect you. Something along the lines of:

sub show_news {
  my ($dbh, $article_id) = @_;
  my $sth = $dbh->prepare("...");
  $sth->execute($article_id);
}
[download]

Would make everything much easier to debug. You don't have to worry about passing $dbh by copy as it's a reference already (a concern that seems to come up often). As you can see, the only variables the subroutine works on is those passed into it.

You need to read up on DBI a bit more.

$dbh->disconnect;

$sth    = $dbh->prepare(...) or die ...
[download]

Look at the above. You disconnect, thereby destructing the handle, then try to use it in the next line. If you're using Apache::DBI this may not even cause an error as it overides the $dbh->disconnect method. It's certaintly not what you meant though.

You declare RaiseError => 1 then consistantly check for errors manually anways (sort of defeats the purpose no?).

UPDATE news_articles 
SET article_title = ? 
WHERE $article_id = ?
[download]

At best this would only change the article title, not the body or any other part. And where do the variables come from? Pass them into the sub and check to make sure they're what you expect with a print statement or two.

Why do you declare connections in each sub? You're setting yourself up for errors due to inconsistancy. You many want to consider a generalised connection method.

You may want to check out CGI::Application; it's a framework that uses dispatch tables, similar to the way you seem to be setting up your scripts.

Most of these are generalised suggestions but, by taking some of them into account, I think you'll find most of your troubles go away and the ones that don't will be easier to debug.

Great suggestions. I was trying to be pedantic with RaiseError and "..or die" there. Probably cut and paste all over the place trying to track down the source of the problem.

You mentioned using a more generalized conneciton method, which makes sense, however.. how do I pass artguments between subs, yet retain their ability to stay local? In the example above, you mentioned using:

my ($dbh, $article_id) = @_;

..except I'll never know which arguments are passed to $dbh each time. Somtimes it may be one argument, sometimes 20 arguments, so I'll have to construct that inside the sub (not global), after I know the arguments that are going to be passed, right? In this case, that means I have to recreate that handle in each sub where it's used.

Herein lies one part of the equation I'm not familiar with; passing locals between subs as arguments, but never knowing how many arguments will be passed beforehand.

$dbh = DBI->connect('conn_str', 'user', 'pass', { <args> });
[download]

$dbh is just a handle. It simply refers to the connection we just made to the database. As such, you can pass it anywhere and it will always represent that connection to the database. When we say my $sth = $dbh->prepare('SQL'); we're saying; use this database connection, prepare this SQL code, and return a reference to that code object. It doesn't matter what the SQL is or how many arguments you pass in $sth->execute(), as we're still using the same connection. You can $sth->finish() that statement and start another on the same $dbh because, as stated above, we're still using that same connection. No matter where you pass it or what methods you call on it (excluding disconnect of course :) it's still the same connection.

If there's a case where you use different logins or connection strings, that's when you'd need to pass a different handle. With our hypothetical generalised connection method in our application modules, we'd pass which type of connection we wanted returned. A standard approach is to make a read only user for most use and other users with varying write permissions (when you still want connection pooling).

If you don't get that last bit, don't sweat it. Keep it simple until you've got the basics of DBI down. Also you might want to check out Programming the Perl DBI. If you like your books online http://safari.oreilly.com has all their titles at really great subscription prices.

BTW, not trying to be heavy handed or anything. My HS english teacher just said repeating in threes is a good literary device ;)