Breaking off from #720876: Third parameter of user_module_invoke() should be object but user_register_validate() calls it with an array instead. as requested by pwolanin on IRC.

[15:41] <pwolanin> pillarsdotnet:  can you add to the doxygen of http://api.drupal.org/api/drupal/modules--user--user.module/function/user_module_invoke/6
[15:49] <pwolanin> pillarsdotnet:  please add some doxygen for the function I linked to indicate it has to be an object
[15:49] <pwolanin> pillarsdotnet:  other than that it's rtbc
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pillarsdotnet’s picture

Status: Active » Needs review
FileSize
1.26 KB

Patch adds documentation for parameters.

Status: Needs review » Needs work
Issue tags: -Needs backport to D6, -Needs backport to D7

The last submitted patch, user_module_invoke-document_parameters-1192178-1.patch, failed testing.

pillarsdotnet’s picture

Status: Needs work » Needs review
Issue tags: +Needs backport to D6, +Needs backport to D7
jhodgdon’s picture

Status: Needs review » Needs work

To me, the description of $type is a bit confusing, since it's not really the hook name, but the hook name without 'user_'.

Other than that, it looks good.

jhodgdon’s picture

Oh. Also, in D7/8, the paragraph above where your patch is states that it invokes hook_user(), which is not the hook name any more in D7/8. That should also be fixed.

pillarsdotnet’s picture

Status: Needs work » Needs review
FileSize
1.61 KB

Corrected as suggested, and also added an explicit list of valid choices for the $type parameter. Considered adding @see http://api.drupal.org/api/search/8/hook_user_, but that would be misleading, as there are 17 API functions whose names begin with "hook_user_" but only 5 that have signatures compatible with user_module_invoke().

jhodgdon’s picture

So this is interesting. On another issue yesterday, I traced down module_invoke_* and realized that actually the args *are* passed by reference (see http://drupal.org/node/1193514#comment-4629398). So that line that says:

* We cannot use module_invoke() for this, because the arguments need to
* be passed by reference.

is actually incorrect. Maybe there is no good reason for user_invoke, and maybe there is a good reason, but that isn't the reason.

Any insight?

The rest of the doc in the patch looks really good to me...

pillarsdotnet’s picture

FileSize
866 bytes
940 bytes

I wrote a test program which I believe shows that the comment is indeed correct, at least for my version of PHP.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

you are right. I was confused. Within user_module_invoke or module_invoke, everything is pass by reference via PHP's call_user_func functions, but module_invoke itself doesn't take its args by reference.

In which case, the patch in #6 is fine. D8/d7, and then mark as to be ported for d6 please.

pillarsdotnet’s picture

Within user_module_invoke or module_invoke, everything is pass by reference via PHP's call_user_func functions

At least for some versions of PHP < 5.3.0

jhodgdon’s picture

Whatever. In any case, the patch is fine, so let's get it in.

webchick’s picture

Version: 8.x-dev » 6.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed to 8.x and 7.x. Thanks!

Marking to be ported to D6.

webchick’s picture

Issue tags: -Needs backport to D7
pillarsdotnet’s picture

Status: Patch (to be ported) » Needs review
Issue tags: -Needs backport to D6

Status: Needs review » Needs work
Issue tags: +Needs backport to D6

The last submitted patch, user_module_invoke-document_parameters-1192178-6.patch, failed testing.

pillarsdotnet’s picture

Category: task » bug
Status: Needs work » Needs review
FileSize
1.88 KB

Patch backported.

jhodgdon’s picture

Status: Needs review » Needs work

In Drupal 6, the hooks are all hook_user($op = 'whatever'), not hook_user_cancel() etc. So the patch cannot be simply copied from Drupal 7.

pillarsdotnet’s picture

Status: Needs work » Needs review
FileSize
1.53 KB

Corrected.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

That looks quite reasonable to me. Thanks!

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Thanks, committed, pushed.

Automatically closed -- issue fixed for 2 weeks with no activity.