As a general rule, you should avoid AUTOLOAD like the plague. there are many problems with it (not the least of which is that eval you have going on). However, if you must use AUTOLOAD, you should consider providing a can method. If you don't, the module will lie when asked about its capabilities.

Another way of approaching your problem would be to use a delegation class (simplified for clarity).

package TestModule;
require DBI;

use Class::Delegator
  send => [qw/prepare do execute .../],
  to   => '_dbi';

sub _dbi { shift->{DBI} }
       
sub new {
    my $class = shift;
    return unless @_;
    return bless { DBI => DBI->connect(@_) }, $class;
}
[download]

With this approach you get fine-grained control over the exposed interface and you don't have to wonder about whether your AUTOLOAD is exposing something it shouldn't or if your eval is secure.

Hm, yes, I am aware of the potential problems with AUTOLOAD. The approach above does work -- but it has one problem that the use of AUTOLOAD solves: I want to always have all of a DBH's methods sent to my DBH. So, it looks like I'm faced with a tradeoff.

On the one hand, I could do as you suggest above, but I'd have to maintain a list of the methods on my {DBH} attribute. If I miss one, then I have a bug. This also raises the complexity of my test suite.

On the other hand, I could continue to use AUTOLOAD, and deal with the issue. My current version of the AUTOLOAD sub:

# sets up autoload subs that 'inherit' DBI's DBH methods
sub AUTOLOAD {
    my $op = $AUTOLOAD;
    my $self = shift;
    $op =~ s/^.*:://;
    if ( DBI::db->can($op) ) {
        # create a wrapper for a DBH method
        eval "sub $op { return shift->{DBH}->$op(\@_); }";
        $op->($self, @_);
    }
    elsif ( $op =~ /^_/ ) {
        # return the appropriate attribute
        $op =~ s/^_//;
        exists $self->{$op} && return $self->{$op};
        return $self->error("Can't autoload for attribute _$op");
    }
    else {
        return $self->error("Cannot autoload $op");
    }
}
[download]

I don't worry too much about the security of my eval in this case, since this is a module, and doesn't directly process user input. My scripts never use user input to pass to AUTOLOAD either. Finally, I don't eval anything until I've checked that it's a DBH method. That, all in all, seems pretty safe, though I'd love to be enlightened if I'm missing something there.

I like the idea of Class::Delegator (and, actually, it solved another issue I've been pondering recently, so thanks!), but I'm wondering if there's a way to get the important feature that AUTOLOAD is providing me (making all of the DBI dbh methods available) without the issues AUTOLOAD raises.

Any ideas?

<-radiant.matrix->
A collection of thoughts and links from the minds of geeks
The Code that can be seen is not the true Code
"In any sufficiently large group of people, most are idiots" - Kaa's Law

There's no way you can do it perfectly reliably without AUTOLOAD because of Perl's poor introspection. And, as you note, if you miss a method you have a bug. You can either publish which methods you support (thus turning the bug into a "feature") or have your AUTOLOAD handle it. You might want to just check can (which returns a code ref). The following untested bit might help:

if (my $sub = $self->{DBI}->can($op)) {
  no strict 'refs';
  *$op = $sub;
}
[download]

You'd have to customize that for your needs, but it has the benefit of installing the subroutine directly into your namespace so you only take the AUTOLOAD hit on the first call.

Cheers,
Ovid

New address of my CGI Course.

AUTOLOAD does not only create subs, but is used to decide on which sub to call. So, you can either call an existing sub, or create one and then call it. to avoid confusion with caller() and stuff, usually goto is used instead of a simple call. But in your code, there is neither a call nor a goto in AUTOLOAD().

I think I saw a tutorial here somewhere, but the last time I was here was about 2.5 years ago, so there.

perl -e'$b=unpack"b*",pack"H*","59dfce2d6b1664d3b26cd9969503";\
for(;$a<length$b;$a+=9){print+pack"b8",substr$b,$a,8;}'
[download]

Yep, you're correct. Sometimes it's just an extra pair of eyes that helps. Thank you! I updated my node to reflect the solution.

<-radiant.matrix->
A collection of thoughts and links from the minds of geeks
The Code that can be seen is not the true Code
"In any sufficiently large group of people, most are idiots" - Kaa's Law

Regarding your update,

• You didn't call the function in the else.
• In fact, there's no real need to create a function in the else since the program will likely die if you get there.
• You're keeping AUTOLOAD in the call stack by not using goto.
• You're using a regexp where you could be using much fast rindex.

See my (updated) reply below for an implementation without those problems.

Like yosefm already mentioned, you never actually call anything from AUTOLOAD. Here's a simple (only slightly tested) AUTOLOAD I used for a wrapper class:

sub AUTOLOAD {
   my $method = substr($AUTOLOAD, rindex($AUTOLOAD, '::')+2);
   my $code_ref = DBI::db->can($method);

   if (!$code_ref) {
      require Carp;
      Carp::croak("Undefined subroutine $method called");
   }

   # This only works with methods,
   # and only if they are non-static
   # (just like your code).
   local $_[0] = $_[0]->{DBH};
   
   goto($code_ref);
}
[download]

I avoided regexps in case the clobber $1 and similar vars in the caller.

Update: If you wanted to create stubs for possible efficiency:

sub AUTOLOAD {
   my $method = substr($AUTOLOAD, rindex($AUTOLOAD, '::')+2);
   my $code_ref = DBI::db->can($method);

   if (!$code_ref) {
      require Carp;
      Carp::croak("Undefined subroutine $method called");
   }

   my $stub = sub {
      # This only works with methods,
      # and only if they are non-static
      # (just like your code).
      local $_[0] = $_[0]->{DBH};
      &$code_ref;

   };

   { no strict 'refs'; *$method = $stub; }

   goto($stub);
}
[download]

1. Have you considered writing a DBI subclass ? I've written one (DBIx::Chart); it can be a challenge in some ways but may a better approach cuz...

2. are you aware that DBI uses tied blessed hashes ? That may be an issue for your efforts...esp. when DESTROY()'s unexpectedly occur multiple times.

3. I've also written something akin to subclassing (DBIx::Threaded) that actually does implement instances of every little handle method (cuz it turned out I couldn't do a real subclass); you might find it useful to cut/paste from it if AUTOLOAD doesn't meet your needs.

You ought to know that every time you create or delete subroutines, add, remove, or replace them from symbol tables, or modify any @ISA, you invalidate perl's interpreter-wide method cache. You should avoid doing this kind of work during runtime because it forces perl to discard its cached information about method lookups and forces it to do new lookups for every method call from then on (til they're repeated and then it uses the newly cached versions).