Re: [SM-DEVEL] Overriding already assigned template values in plugins (new hook)

[Paul L. <pa...@sq...>]

See below, but upshot is that I hope the changes enumerated herein
will unify our plugin model a bit more so we have a more consistent
API in SM v2.0 and address the issues Alexandros has found and those
mentioned by Thijs just today.


On 11/6/06, Paul Lesniewski <pa...@sq...> wrote:
> On 11/6/06, Alexandros Vellis <av...@no...> wrote:
> > On Mon, 6 Nov 2006 07:51:21 -0800
> > "Paul Lesniewski" <pa...@sq...> wrote:
> >
> > > - function concat_hook_function($name,$parm=NULL) {
> > > + function concat_hook_function($name,&$parm=NULL) {
> >
> > Bleh. My "favorite" of them PHP quirks.
> >
> > Argument passing by reference AND default value assignment do NOT work
> > together in PHP4. :/ ("As of PHP 5, default values may be passed by
> > reference.").
>
> ARGH!
>
> > So if we go down this route, I believe we have these choices:
> >
> > 1) make $params get passed by reference, and ditch the default null
> > only for concat_hook_function().  This actually makes sense. After a
> > quick grep, all the hooks for concat() have the $params argument. The
> > select_range plugin has a patch with a new plugin hook that does not
> > have the $params argument, but it is incompatible with SM 1.5 anyway.
> > And I've put similar hooks in avelsieve too, but noone uses them yet
> > and I can fix them instantly. :)
>
> OK, but I'd really like more consistency with all hooks.  I was
> actually looking at how do_hook() gets its args differently than the
> others and considering fixing it.  My ideal is that ALL hook functions
> require arguments (must be an array and can be set null by the caller
> if no args) and get called like:
>
> do_hook[_function/etc]('hook_name', array('arg1', 'arg2', 'arg3'));
>
> And the hook function:
>
> function do_hook[_function/etc]($hook_name, &$hook_args) {
> ...
>         // if core is sanitized this might be a waste of cycles...
>         if (!is_array($hook_args)) error_box('Bad hook call!!!');
> ...
>         $function($hook_name, $hook_args);
> ...
> }
>
> Note this would disrupt many many hooks and associated plugins.
> Currently only do_hook() passes the hook name with the rest of the
> hook arguments, but not even in this format.  It is a big hassle, but
> again, this is DEVEL and we are shooting for a more consistent,
> grown-up product with v2.0.....
>
> This change might make do_hook_function() and maybe
> filter_hook_function() obsolete.
>
> > 2) ditch the default null for all hook functions (ugly, and needs
> > checking in a lot of places, possibly makes some plugins more
> > incompatible etc.)
>
> I think that's what I am suggesting above.  What about it is ugly?
> What checking does it need?  It needs associated changes in all hook
> calls, but no checking, just a one-time code cleanup.  We are losing
> plugin compatibility in 1.5.2 anyway....  definitely a big hassle,
> but....
>
> > 3) think of some other hack (ideas?)
>
> In the core:
>
> if (check_php_version(5))
>     $result = @concat_hook_function('hook_name', $args);
> else
>     $result = @concat_hook_function('hook_name', &$args);
>
> Something like that.  ;-)  I believe PASSING the argument by reference
> generates an error notice that such behavior is deprecated, but it
> could work for the life of PHP4.
>
> > 4) Leave it as is. But plugins touching this hook and modifying the
> > object inside $params[1] will have different results in PHP4/PHP5.
>
> Seems like a support nightmare.
>


OK, here is what I've come up with for a redesigned plugin subsystem.
One of the main changes is that the arguments passed from the core to
the plugins should always be contained in an array, and that array is
passed by reference.  Because of limitations with PHP4, even when no
arguments are passed, we have to make sure to pass an empty array
(can't pass by reference whilst giving a default value in the
prototype).

do_hook()
  - prototype changed to: (string $name, array &$args)
  - plugins called with:  plugin_function($args)

REMOVED filter_hook_function() because same functionality
can be achieved with the new incarnation of do_hook()
(filter_hook_function() was only added to 1.5.2 recently,
so the effect should be little to nothing)

do_hook_function()
  - prototype changed to: (string $name, array &$args)
  - plugins called with:  $ret = plugin_function($args, $ret)
  This hook has always been poorly designed (previously
  only the return value from the LAST plugin registered
  on the hook would be used) and even with this change,
  it is hard-to-impossible for all plugins on the hook to
  know what the other plugins (at least the ones AFTER)
  do with the return value unless we do something
  strange like iterate through all the plugins twice...
  It'd be nice if we could deprecate this.  I will try to
  see if we can remove it from the core.

concat_hook_function()
  - prototype changed to: (string $name, array &$args)

boolean_hook_function()
  - prototype changed to: (string $name, array &$args, int
$priority=0, boolean $tie=false)


I am working through the core to try and fix at least some of the
critical hook calls before I commit everything, but we will have to
watch for bugs and anomalous behavior both with how the hooks are
called in the core as well as how the plugins use the arguments passed
to them.