[drupal-devel] [bug] module_invoke brokes default values

[chx]

Issue status update for http://drupal.org/node/17770

 Project:      Drupal
 Version:      cvs
 Component:    base system
 Category:     bug reports
 Priority:     critical
 Assigned to:  chx
 Reported by:  chx
 Updated by:   chx
 Status:       patch
 Attachment:   http://drupal.org/files/issues/module_invoke.patch (1.84 KB)

Here is a fix for module_invoke_all and module_invoke.
Turns out that the node_invoke and nove_invoke_nodeapi and user_invoke
does not need replacement.
There was a related bug in user.module, where necessary NULL parameters
were not in the module_invoke call, that's fixed, too.
Also, module_invoke_all uses module_implements which is good if one
hook (say hook_nodeapi) is called more than once and does no harm if it
is called only once.


chx



Previous comments:
------------------------------------------------------------------------

February 22, 2005 - 22:26 : chx

Attachment: http://drupal.org/files/issues/module_inc_no_default_nulls.patch (685 bytes)

Try this: module_invoke('taxonomy', 'get_tree', 1); and compare the
results with taxonomy_get_tree(1); 
What happens is that taxonomy_get_tree is called by the arguments 1,
NULL, NULL, NULL which breaks it.
Hence the patch.
Every Drupal version I have met (4.4, 4.5, CVS) contains this error. I
think this led module authors to not use module_invoke (at least I did
so until now) which will be quite a problem if we decide on breaking up
the modules into smaller pieces.


------------------------------------------------------------------------

February 22, 2005 - 22:35 : killes at www.drop.org

I have no idea how else we should fix this. +1.


------------------------------------------------------------------------

February 22, 2005 - 23:49 : chx

Attachment: http://drupal.org/files/issues/module_inc_no_default_nulls_0.patch (625 bytes)

Of course, lambda is another solution. When I objected on IRC that
call_user_func_array is slower, Bart Jansens pointed out that
slashdotted pages are cached thus module_invoke is not called. So if
this solution is accepted, the credit goes to him.
But one of the solutions shall be accepted.


------------------------------------------------------------------------

February 23, 2005 - 00:29 : chx

Attachment: http://drupal.org/files/issues/themefix_0.patch (712 bytes)

The message was wrong (but funny).


------------------------------------------------------------------------

February 23, 2005 - 00:30 : chx

Sorry, posted to the wrong issue. :(


------------------------------------------------------------------------

February 23, 2005 - 07:16 : Dries


The function module_invoke_all needs to be updated as well.
Won't this break arguments passed by reference? Did you test such
scenario?



------------------------------------------------------------------------

February 23, 2005 - 07:32 : chx

module_invoke_all, sure, just tell me which approach is accepted (lambda
or switch(func_num_args())
References? module_invoke does not accept references, that's why we
have node_invoke and nove_invoke_nodeapi (these may be wrong, too) and
user_invoke (this one has only one NULL, in a very specific place, so
most probably it won't break anything).
So, let's agree on one way or another and I'll patch module_invoke_all,
node_invoke, node_invoke_nodeapi too.


------------------------------------------------------------------------

February 23, 2005 - 09:41 : Dries

Bart's approach is preferred.