Re: [cgiapp] Re: register_hooks() proposal

[Cees Hek <suppressed>]

Mark Stosberg wrote:
> I think it's still an interesting an idea, and I'm seen it come up
> several times in discussion since then. I'm not opposed to it, I just  
> feel conservative about moving it directly to CGI::Application proper.

The patch does not alter any of the current functionality in 
CGI::Application.  It is fully backwards compatible, and provides a new 
feature that in my opinion is sorely lacking.

> I think it would rather see it released separately, let people play it 
> and get a feel for it, and then consider merging into the main
> code-line. If you have a plug-in that uses it as an example, I think
> that would go even further to speed adoption. 

I don't mind releasing it separately, but that would require overriding 
and duplicating most of the functionality of CGI::Application (since it 
needs to slightly alter the way the hooks are called, and the hooks are 
spread throughout the codebase).  I wouldn't want to have to maintain a 
fork like that for very long.

> I have also thought of some more concrete feedback that I think would
> ease my mind about the addition:
>
> - I think it's important to be able to view what all the hooks are doing
>   and in what order. Here's a "use case". Say that the feature becomes
>   popular, and I'm using three plug-ins that have registered hooks in
>   my pre-run mode. I see some strange behavior that I think is related.
>   How do I debug that? 

You debug it the same way you debug any other program.  Either you start 
adding print statements to the code, or you run it through a debugger. 
I don't really see how that is relevant.

>   I think there should be addition or option to dump() that prints out 
>   all the hooks in the order they are called in. 

Sure, I can see the usefulness of that.

However, that can be handled by the developer as well.  I use a module 
called Sub::WrapPackages which uses a module mentioned elsewhere in this 
thread called Hook::LexWrap.  What it does is wrap every method call in 
a package, and executes a bit of code before the method is called, and 
after it is called.  In the method wrapper, I print to my debug log the 
name of the method and the parameters being passed.  Here is a quick 
example of how I use it (will probably need a bit of altering to get it 
to work for you):

sub log_subroutine_calls {
  my $class = shift;
  eval {
    require Sub::WrapPackages;
    Sub::WrapPackages->import(
      packages => suppressed,
      pre => sub {
          local $Log::Log4perl::caller_depth = 3;
          my $logger = Log::Log4perl->get_logger('Functions') || return;
          $logger->debug("calling $_[0](".join(', ', @_[1..$#_]).")");
      },
      post => sub {
          local $Log::Log4perl::caller_depth = 3;
          my $logger = Log::Log4perl->get_logger('Functions') || return;
          $logger->debug("returning from $_[0] (".join(', ', 
@_[1..$#_]).")");
                            }
    );
    1;
  } or do {
    print STDERR "Failed to install Sub::WrapPackages:  suppressed";
  };
}

Now if you want to see the execution path of your modules, you call it 
like this:

log_subroutine_calls('CGI::Application', 'My::CGI::BaseClass', 
'CGI::Apllication::Plugin::Session');

That will tell you whenever a method in any one of those three modules 
is called and the parameters that were passed and returned.  If you are 
passing complex data structures, then you can change the above to use 
Data::Dumper to dump the parameters, but then your log file will grow 
incredibly fast...

>   When hooks are registered, the process is more convenient to set up,
>   but I'm afraid it my be more difficult to debug and cause confusion.
>   Perhaps an explicit example of a plug-in using it would cure me of
>   this. :)

I guess you will have to wait for my Auth plugin then.  I can't promise 
a completion time right now though.

> - I wonder if the REALLY_FIRST, FIRST... position system could be
>   simplified/improved. The way I think about it, either want you
>   plug-in to run at one of three times at a particular stage:
>
>   - before everything else
>   - after everything else
>   - you don't care.

Sure, those are the important ones, and I'd be happy if it was left at 
that.  REALLY_FIRST and REALLY_LAST would only be used in extreme cases 
where it is critical that they run first or last.  I could see it being 
important to run an Auth plugin before any other plugins are run...

But to simplify things, I don't think it would be a great loss to scrap 
the REALLY_FIRST and REALLY_LAST sections.

>   Here's an idea for an alternative:
>
>   Just use a simple array at each stage. You can append to either
>   end if you want to try to be really first or really last, or do
>   either if you don't care. 
>
>   If know you some plug-in needs to be "really first" or "really last",
>   "use" it after the other ones, causing it to be last thing added to
>    one end of the array.

That system breaks down very quickly though.  All developers that "don't 
care" will just append to the end of the array, and hence a module can't 
indicate that it is important to run at the end.  ie if a method that 
wanted to run last is registered very early, it could push onto the end 
of the array, but what if other modules that "don't care" get registered 
 afterwards?

>    As it is, I find the names very confusing. "middle" doesn't seem
>    useful, "first" is not really first, and even "really first" is only 
>    a suggestion, since more than one thing can register there! Those
>    names don't work for me.

I honestly don't see the confusion here (especially if we take out the 
REALLY_FIRST and REALLY_LAST sections).  It just needs to be explained 
explicitly in the docs.

Maybe if I throw out some usage examples it will seems simpler...

# I need to run as late as possible in the teardown hook
$self->register_hook('teardown', \&disconnect_db, 'LAST');

# I don't care where I run
$self->register_hook('teardown', \&flush_session,);

# I would like to run as soon as possible in the teardown stage
$self->register_hook('teardown', \&log_execution_time, 'FIRST');



Here we have three methods all wanting to run at the teardown stage that 
are indicating a preference of where they want to run...  If we start 
playing with arrays, and pushing and unshifting, you are really just 
complicating the matter with no real benefit...

I think it would probably be beneficial to have a method that allows you 
to flush the hooks (ie remove them), and to generate a list of already 
registered hooks, but I really didn't want to complicate the patch 
unnecesarily.  What I provided seemed simple, small, and fully backwards 
compatible with previous version.  What is still needed is documentation 
and tests and I will make time to provide those if we can agree on a 
method of implementation.


Cheers,

Cees

---------------------------------------------------------------------
Web Archive:  http://www.mail-archive.com/suppressed/
             http://marc.theaimsgroup.com/?l=cgiapp&r=1&w=2
To unsubscribe, e-mail: suppressed
For additional commands, e-mail: suppressed