Re: Callback support is oin

David Wheeler Fri, 25 Mar 2005 15:07:06 -0800

On Mar 23, 2005, at 2:01 AM, Tim Bunce wrote:

I can certainly live with that. Are return values currently ignored?
An exception will be thrown if anything is returned.


Okay, good to note in the docs.

I think I'd be inclined to have them mean nothing, and leave it up to
callbacks to throw exceptions when something exceptional happens.


Sure, but we may come up with interesting non-exceptional behavior.
Like, for example, allowing the callback to say 'don't call the
method but return @foo instead' - effectively replacing the call.

Ah, yes, that'd be very interesting. Good foresight. I knew there was a reason you were put in charge of the DBI. ;-)

Oh, I guess I didn't include any docs in my original patch. I'll take a
shot at this in the next couple of days.
Let's let Steven have a crack at the docs first.


Sure, happy to have one less thing on my plate. :-)

1. callbacks are only passed the handle, not the other method args.


That seems smart to me. If you passed it via method args (I assume you
mean \%attrs), then would it last only for the execution of that
method, or become permanent? The former would be okay, I think.


Good question. Given:

    $dbh->{Callbacks}->{prepare} => sub {
        my ($dbh, $sql, $attr) = @_;
        $attr->{foo} = 42;
        return;
    };

and

    A) $dbh->prepare($sql, { foo => 1 })
    B) $dbh->prepare($sql, \%attr)

then for A the effect is temporary but for B it's permenant.


Sure, that makes sense. But if I did this:

  $dbh->prepare($sql, { Callbacks => { prepare => sub { ... } });

...well, I guess it'd be pretty dumb to do that. Callbacks should only apply to the database handle, I think. But this might be interesting as a one-time thing, such that the callback applied only to the single call to prepare():

  $dbh->prepare($sql, { Callback => sub { ... } });

But certainly isn't a crucial feature to have, IMO.

You can avoid that problem by doing:

    $dbh->{Callbacks}->{prepare} => sub {
        $_[2] = { %{ $_[2] }; # shallow copy \%attr first
        my ($dbh, $sql, $attr) = @_;
        $attr->{foo} = 42;
        return;
    };


Sure.

But all the above will have problems with

    $dbh->prepare($sql);

since there's no \%attr argument (or slot on the stack) to manipulate.
I can see a way to fudge around that in the C code, but it won't be
pretty :)


Wouldn't it just be undef?

So when do the execute now?
Pre.


Okay.

3. The method name isn't available via a localized $_
Is it available at all?
Not currently. Probably only needs a couple of lines of C code to
add that though.


Yeah, that'd be neat.

5. A DBI::Callbacks module to provide an interface for managing
  callbacks, including a way to define callbacks to be applied
  to all future child statement handles (by using a post-prepare).

Not sure I understand what you mean here. Would you care to elaborate?


Currently each method can only have one callback. Two bits of code
trying to add/remove callbacks will tread on each others toes.

What I've not said explicitly so far is that it would be nice to
optionally support multiple callbacks per method by allowing
callbacks to be arrays of code refs:

  $dbh->{Callbacks}->{prepare} => [ [EMAIL PROTECTED], [EMAIL PROTECTED], ]

but managing that would be a pain, an API would help:

  $h->callback_add( $method_name, $code_ref [, $pre_or_post ]);
  $h->callback_delete( $method_name, $code_ref [, $pre_or_post ]);

But that's some way down the road. The bigger short term gain is
that the docs for Callbacks won't clutter the main DBI docs.
(Similar to DBI::Profile being separated out.)

I gotcha. Seems like you're talking about syntactic sugar for the most part, though, eh?

Let's let Steven have a crack at the docs first. Hopefully he can
absorb my extra comments and examples from this thread.


Yes, there are some good nuggets here.

That may not be enough now, though, I honestly don't know.


It's enough for the 'explicit' callbacks written into connect and
connect_cached[1]. But not for the generic implicit callbacks
handled by the dispatcher.


Ah, something would have to be added to the PP dispatcher, no?

Regards and many thanks,


You're welcome, but it's not over yet... :)

Take a look at the comment block in t/70callbacks.t - I've not
given it much thought yet but the implication is that the
connect and connect_cached callbacks may need to change slightly
(in name, or timing, or both) to be more consistent with the generic
callbacks.


Yes, I saw it:

The big problem here is that conceptually the Callbacks attribute is # applied to the $dbh _during_ the $drh->connect() call, so you can't set a callback on "connect" on the $dbh because connect isn't called on the dbh, but on the $drh.

So a "connect" callback would have to be defined on the $drh, but that's cumbersom for the user and then it would apply to all future connects using that driver.

The best thing to do is probably to special-case "connect", "connect_cached" and (the already special-case) "connect_cached.reused".

That seems reasonable to me. Since the drh isn't really available in user space, it doesn't matter so much to me. The other option I can think of, though, is to perhaps make those callbacks configurable via a class method, so then they apply to all uses of connect() and connect_cached() across the board. But woe betide the module that uses DBI that doesn't know that some other module has set up such a global callback.

The current approach makes sense to me. In what other way can you see that it might make sense to change their names?

Cheers,

David

Re: Callback support is oin

Reply via email to