Editing data extracted from mysql through CGI/mod_perl

[hacker]

Hello fellow monks.

I've been trying to figure out the architecture behind querying articles from a mysqldb, populating them in a form which allows editing, then submitting those changes back to the database. I've got all the parts working, except that final UPDATE. I'm not entirely sure my logic is sound here.

I can pull the articles from the db, display them, populate the edit form with the values, but when I change the values in the form and submit, the data in the database is never updated. The form action is specified to be:

<form action=$script?action=editnews&amp;amp;amp;amp;article=15 
      enctype="application/x-www-form-urlencoded">

Ideally, this should allow me to edit the contents of article 15 from the database, and submit it back with updated changes.

Here's the relevant code. I'm still a bit confused as I try to debug Apache::Registry errors and warnings, mysql errors and warnings, and mod_perl/perl errors and warnings, so there may be something very fundamental I'm missing and just haven't caught it yet:

#################################################
# The usual suspects
use strict;
use warnings;
use diagnostics;
use Env;

# Send the errors to the browser
use CGI::Carp qw( fatalsToBrowser );
use CGI qw(:standard);
use CGI qw(:standard start_div end_div);
use DBI;

my ($script,            # this script
    $dbuser,            # database auth user
    $dbpass,            # password to connect to db
    $dbh,               # database handle
    $sth,               # select handle
    $article_arrayref,  # reference to the array from DBI
    $article_row,       # the result from the query
    $article_id,        # id number of the article
    $article_title,     # table article_title
    $article_summary,   # short synopsis of the article
    $article_date,      # date the article was posted
    $article_author,    # author of the article
    $article_body,      # table article_body
    @row                # array holding result
   );

&print_header();
&top_menubar();

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


&bottom_menubar();
&end_page();

#################################################
#
# Pull articles from the db
#
#################################################
sub show_news {
$dbh    = DBI->connect('DBI:mysql:plucker:localhost:3306',
                       $dbuser, $dbpass, {RaiseError => 1});


$sth    = $dbh->prepare ("SELECT article_id, article_title, article_au
+thor,
                          DATE_FORMAT(article_date, '%W %M %D %Y') as
                          my_date, article_body from news_articles
                          where article_id=?");

$sth->execute($vars{article});

$article_arrayref = $sth->fetchall_arrayref() or die "[$DBI::err] $DBI
+::errstr";

foreach $article_row (@{$article_arrayref}) {
        ($article_id, $article_title, $article_author, $article_date,
        $article_body) = @{$article_row};

        print div({-id=>'newsbody'}, h3("$article_title"),
        p({-class=>'fol'},
                a({-href=>"$script?action=editnews&article=$article_id
+"},
                "Edit this article")),
        p("$article_author"), i("$article_date"), "$article_body");

}
 
$sth->finish;
$dbh->disconnect;

}

#################################################
#
# Show the news articles in the edit window
#
#################################################
sub edit_news {

$dbh    = DBI->connect('DBI:mysql:plucker:localhost:3306',
                       $dbuser, $dbpass, {RaiseError => 1});

$sth    = $dbh->prepare ("UPDATE news_articles SET article_title = ? W
+HERE 
              $article_id = ?") or die $dbh->errstr;

$sth->execute($article_title, $article_id) or die die $dbh->errstr;

$sth->finish;
$dbh->disconnect;

$sth    = $dbh->prepare ("SELECT article_id, article_title, article_au
+thor,
                          article_body from news_articles where articl
+e_id
                          like $vars{article}");

$sth->execute or die "Can't execute!";

$article_arrayref = $sth->fetchall_arrayref() 
        or die "[$DBI::err] $DBI::errstr";

foreach $article_row (@{$article_arrayref}) {

        ($article_id, $article_title, $article_author, 
     $article_body) = @{$article_row};

        print start_form(-action=>"$script?action=editnews&article=$ar
+ticle_id"),
                        div({-id=>'newsbody'}, 

                        p("Enter a new article title:"),
                        textfield({-name=>'newstitle',-size=>60, 
                   -value=>"$article_title"}),

                        p("Enter a new article author:"),
                        textfield({-name=>'newsauthor',-size=>60, 
                   -value=>"$article_author"}),

                        p("Edit additional story details"),
                        textarea({-name=>'newsbody',-cols=>60, -rows=>
+30, 
                  -value=>"$article_body"}),

                        submit({-name=>'editnews',
                -value=>'Submit edited changes'})
        );
}
 
$sth->finish;
$dbh->disconnect;

}

Edit kudra, 2002-06-10 added readmore

[talexb]

First, a comment on your coding style .. it seems that you have declared a whack of variables that get set elsewhere (so we're missing the code that sets $article_title, $article_id). You're also passing these global variables into the subroutine edit_news in your mainline, but you don't catch those variables as parameters in the subroutine itself. I suggest you pick one style and stick with it -- globals or parameters (hint: parameters are far more preferable to globals).

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();
  }

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.

[perrin]

I see one major problem here: closures. When you use Apache::Registry, it wraps all of your code inside of another subroutine. Lexical variables like $article_row that you declare outside of your subs and then use in the subs result in closures, meaning that your sub edit_news() will maintain the first value of $article_row forever, kind of like a global variable that you can never reach. This is all explained here in the mod_perl guide.

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.

[Arguile]

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);
}

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 ...

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 = ?

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.

[hacker]

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.

[Arguile]

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

$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 ;)