hook_userpoints pass $params by reference

I like the idea.

We need to change the API section of the README.txt and the documentation on drupal.org for example modules to reflect this.

But before I proceed, I need feedback from others on whether there could be side effects, gotchas, ...etc.

Also, I'd rather delay the change till D7 so an API change does not jolt users or affect contrib modules that use the API. It would go nicely with the integer change to float too.

I agree that it is better to delay the change until the Drupal 7 version is released.

The patch should be changed to use module_implements(), rather than module_list().

Because there seems to be agreement on pursuing this change for D7 (rather than D6), I'm changing this issue to 7.x-1.x-dev so that we can track it as part of the ongoing D7 port.

--Ben

Updated the patch and verified that the tests still work, no more testing.

The patch is fine.
I guess module_invoke_all() is not used because the first parameter of the hook needs to be passed by reference, and it's not possible to do that using module_invoke_all() (which calls call_user_func_array()).

Thanks, commited to 7.x-1.x. (Two commits because the patch did not contain the module_invoke_all() change in the .admin.inc file)

Attaching a backport for Drupal 6 in case kbahey wants to reconsider the decision in #3 or a 6.x-2.x is started.

IMHO, this should be a safe change for the following reasons:

- This will not affect modules which use the old hook signature "hook_userpoints($op, $params)", $params is simply not be reference for them. This is perfectly valid.

- To use the new by reference "feature", they need to update that to "hook_userpoints($op, &$params)".

- A problem would be modules which already use the by reference signature, as they would change their behavior in case they change $params (by accident or not). However, these modules are incompatible with PHP 5.3 since that version does not allow to define a function signature with & but call it by value. So these modules are imho plain simply broken...

- The only real issue would be modules that directly currently use module_invoke_all('userpoints', ...). These would need to be updated. Question is, are there any modules that do that?

This obviously went in too fast ;)

Commited another bugfix, the $params argument is optional...

Attaching an updated D6 patch.

While I'm on, also removed the trailing whitespace.

Tagging to make it easier to find patches like this one.

Wouldn't it be better to implement drupal_alter instead?

After all it is meant for cases like this one, where some data needs to be "altered"?

Correct, but drupal_alter() is meant to alter the first argument of the hook. Due to the $op stuff, it can't be used.

That's one of the reasons why $op hooks should be dropped.

Well, provided no module goes doing funny stuff, you can directly pass up to 3 parameters by reference, or more in an array of parameters if needed.

Yeah, but that's irrelevant, it's trivial to create a helper function that only passes the desired argument by reference. One other problem with drupal_alter() is that it enforces an _alter suffix to the hook, which we actually don't want.

The correct way to resolve this in Drupal 7 (although it already is by reference there) is to remove the $op argument and make separate hooks and switch to using an object instead of an array (required for the entity switch anyway). Then it just works.

Closing this as outdated, 6.x version is no longer maintained.