Modules not using proper hook_user parameters

do those modules actually change the value of the parameter?

I agree with Steve. People (eg me) often use core modules as a sceleton for new contrib modules...

Arguments are not *passed* but *received* by reference. I know that Zend Studio complains if you use reference in vain. I even read it why but can't remember. I think I even sent in patches to not pass variables by reference unless necessary.

So a very big negative number on this.

It's a real pity that PHP doesn't have a const &$param argument type...

For the time being, if the spec says it should be passed by reference, then any function that doesn't accept by reference is a bug and should be fixed. If it shouldn't be passed by reference, then the spec should be changed and code updated accordingly. But pick one and be consistent.

Although, doesn't PHP5 change the reference passing system so that arrays are always "by reference", vis, the data is never copied? In that case, by reference or by value wouldn't make any difference, since it becomes by-reference in practice anyway. I suppose in that case sticking with the explicit by-reference is a good thing, as it ensures the same behavior under both PHP4 and PHP5.

(If I'm wrong about PHP5, then ignore only paragraph 3. :-) )

For what it's worth, I agree with #4 and #8. without being able to pass things "const" by reference, changing this will just make the code that tiny bit much more difficult to read for those that touch drupal code in "Maintenance" mode (for example those that try to fix bugs without being intimate with all the code base).

Perhaps the correct fix is to update the documentation to be more clear.

Here's what Zend Studio has to say about this.

What's the fun here, Zend Studio actually gave us a performance warning on system_user so the opposite of the original patch is needed -- one that REMOVES references instead one that ADDs more.

Moving to 6.x-dev queue.

Zend Studio is correct: AFAIK, the PHP interpreter is optimised in such a way that it performs better with by-value parameters than with by-reference parameters. I definitely remember reading somewhere in the PHP manual, something to the effect of: "despite what you think, you should pass variables by value whenever possible, as it's better for performance".

This way it seems to be a "by design". I still leave it open, as we might change some hook implementations to the opposite (see #12).

Is this issue still needed?

Patch needs a reroll, but would suit a novice: setting to needs work for that.

Really don't see why this has been open for so long; it's a no-brainer: our hook implementations should be consistent for several very good reasons, and Zend is just wrong.

Oops, forgot to tag it.

Unassigning so that someone can pick it up for the reroll.

I re-roll the patch for the 6.x-dev branch.

That looks fine to me.

This is not an issue in D7 or D8 (since hook_user() does not exist).

Seems like it was still in debate above without agreement on whether this is the best approach? (I personally would agree following the spec consistently would be best BTW but a little more feedback would be good).

I read through the history here and see several ways to move forward:

1) Leave the function signatures as is (i.e. no patch) as @chx suggests. This leaves the signatures inconsistent across the codebase (which doesn't sit well with me, but would keep Zend Studio happy). Currently, the signatures in 6.x core are:

a) With reference:

blog/blog.module:function blog_user($type, &$edit, &$user) {
contact/contact.module:function contact_user($type, &$edit, &$user, $category = NULL) {
dblog/dblog.module:function dblog_user($op, &$edit, &$user) {
node/node.module:function node_user($op, &$edit, &$user) {
openid/openid.module:function openid_user($op, &$edit, &$account, $category = NULL) {
poll/poll.module:function poll_user($op, &$edit, &$user) {
profile/profile.module:function profile_user($type, &$edit, &$user, $category = NULL) {
statistics/statistics.module:function statistics_user($op, &$edit, &$user) {
trigger/trigger.module:function trigger_user($op, &$edit, &$account, $category = NULL) {
user/user.module:function user_user($type, &$edit, &$account, $category = NULL) {

Other consistencies issue are the use of $type vs. $op and $user vs. $account, but those aren't for this issue.

b) Without reference:

block/block.module:function block_user($type, $edit, &$account, $category = NULL) {
comment/comment.module:function comment_user($type, $edit, &$user, $category = NULL) {
locale/locale.module:function locale_user($type, $edit, &$user, $category = NULL) {
system/system.module:function system_user($type, $edit, &$user, $category = NULL) {

which are all addressed in the patch.

2) Leave signatures as is but added a comment to each function as to why (i.e. $edit is not modified in the code).

3) Apply the patch to get signatures in sync.

My preference is for option 3, but, since this issue is over 6 (!) years old, I imagine there are still people who'll have opposing opinions.

Who reviewed that none of the functions actually change the values now passed by reference? I did not find explanation of that above (or missed it).

• block_user() explicitly sets $edit to an empty array when it is empty, but nothing else:
  

  case 'validate':
        if (empty($edit['block'])) {
          $edit['block'] = array();
        }
        return $edit;
    }
  

• comment_user() and locale_user() do not use the parameter at all.
• system_user() uses the parameter to set the form's theme and timezone fields but does not update $edit.

Patch still applies cleanly.

I think the majority of people here are in favour of the DX improvement of using consistent function signatures throughout core over the perceived performance gain of avoiding the references.

I say "perceived" performance gains because nobody has actually done any profiling to demonstrate a measurable difference before and after, we're just going off an 8 year old screenshot from an IDE. hook_user() is not on a critical performance path, I personally can't imagine any real world scenario where you'd have any kind of performance hit that doesn't fall firmly under the heading of "micro optimisation". I found this stack overflow post that suggests the performance difference is going to be somewhere in the ballpark of 15 micro seconds - http://stackoverflow.com/questions/178328/in-php-5-0-is-passing-by-refer...

We see support for honouring the spec and keeping things consistent in #2, #3, #8, #9, #17, #20, #21, #22

All detractors from this proposal reference the 15 microsecond performance hit and nothing else, which doesn't seem like a strong enough reason to deliberately avoid the spec. If it is, we should be modifying the spec first, then revisiting the whole codebase not just hook_user() in isolation.

In #25, @xjm has gone to the effort of confirming that none of the variables newly passed by reference are modified.

This is RTBC for me.