Re: Plugin architecture

Eric Wilhelm wrote:
> # from Christopher J. Madsen
> # on Saturday 15 March 2008 17:55:
> 
>> $plugin->pre_METHOD($builder, $parameters, $context, $return)
>> $plugin->post_METHOD($builder, $parameters, $context, $return)
> 
>> * To completely override a method, set $_[-1] to an appropriate value
>>    and return 'done'.
> 
> I don't like this return-by-reference scheme.  Can we say that the 
> $return is an object and use methods for that?

How does that work with post hooks?  (In a pre hook, $return is output 
only.  In a post hook, you must pass the current return value in.  It 
may or may not get modified by the hook.)  How does it handle 
scalar/list context?

What happens if you call done but don't return immediately?  What if you 
call both done and continue?  (Although we don't really need continue. 
If you didn't call done, that means processing should continue.)

> Of course, return() doesn't carry a consistent meaning in Module::Build 
> right now, so maybe that needs to be specified.  What meaning should 
> return values have for plugins?

I felt the return value of post hooks is ignored.  Pre hooks return 
'done'/'continue'.  If we make done a method, I'd say we ignore the 
return value of pre hooks also.

> Also, note that die() is probably the best way to deal with an error 
> condition.

Sure.

> The priority and ordering are a concern when any plugin can abort the 
> action based on run order.  This seems like it provides a lot of room 
> for subtle bugs.  How would a plugin declare that it is incompatible 
> with another or otherwise avoid the silent error of "only one of my 
> plugins ran"?

I don't think it would.  That's up to the person selecting the plugins 
to run.  Hopefully, user & system level plugins would be used only for 
things unlikely to conflict with other plugins.

I don't think many pre hooks will need to short-circuit evaluation.  But 
I think it needs to be possible to prevent the default method from being 
called.  I suppose we could say that if a pre hook short-circuits, the 
remaining pre hooks are still called (but any values they try to return 
would be ignored) and only the default method gets skipped.  But I'm not 
sure that's better.

> So, do plugins only get hooks on "ACTION_*", or is everything hookable?  

No, that was just my examples.  Any method is fair game.  That was one 
of my main goals.

> I'm a little concerned about what overhead is being added when no hooks 
> are defined (but I suppose we could redefine our method with a hooked 
> wrapper when a hook request appears.)

That's what I'm thinking.  When a method is hooked, we define something like

sub Module::Build::method_name {
   _call_method_with_hooks(@_, 'method_name')
}

So we'd have overhead only on the methods that actually got hooked. 
This is made simpler because M::B is essentially an empty class.

> The requirement of get_hooks() is nice from M::B's reduced coding point 
> of view, but it requires the plugin author to repeat themselves.

True.  One point I didn't mention about subclassing:  What if I want to 
subclass a plugin but prevent it from hooking one of the methods that it 
normally hooks?  With get_hooks, I just eliminate that method from the list.

I suppose I could just add an empty method to my subclass:

sub pre_METHOD_I_DONT_WANT_HOOKED {}

But then we still get the overhead of hooking the method, even though we 
didn't need to.  And what about future versions of the superclass 
hooking new methods?

Of course, if we put the symbol-grabbing get_hooks in a base class, then 
I can just override that.  That may be the best approach.

One reason I didn't want a base class for plugins is that I'd like them 
to be able to use any object style they liked.  On the other hand, 
get_hooks wouldn't actually need any per-object data, so if that's all 
the base class does, it wouldn't need to care what kind of object the 
plugin is using.

-- 
Chris Madsen                                          [email protected]
   --------------------  http://www.cjmweb.net  --------------------