the hook user expects a user object in its third parameter, $account.
however the user_register_validate function invokes the user hook sending an array instead...
user_module_invoke('validate', $form_state['values'], $form_state['values'], 'account');

This means that I can't user_access($account) in hook_user if $op == 'validate' because user_access expects an object, not an array.
The workaround is to user_access((object)$account);

I've a feeling I'm missing something here, but on the other hand, it's perfectly straight forward.

Files: 
CommentFileSizeAuthor
#26 user_regiser_validate-720876-26-d6.patch948 bytespillarsdotnet
#21 user_register_validate-720876-21.patch880 bytespillarsdotnet
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es).
[ View ]
#17 user_register_validate-720876-17-D6.patch880 bytespillarsdotnet
#14 user_register_validate-720876-14.patch483 bytespillarsdotnet
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user_register_validate-720876-14.patch.
[ View ]
#12 user_register_validate-720876-12.patch880 bytespillarsdotnet
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user_register_validate-720876-12.patch.
[ View ]
#8 user_register_validate.patch491 bytespillarsdotnet
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user_register_validate_1.patch.
[ View ]
#3 user_register_validate.patch459 bytespillarsdotnet
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user_register_validate_0.patch.
[ View ]
#1 user_register_validate.patch491 bytespillarsdotnet
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user_register_validate.patch.
[ View ]

Comments

pillarsdotnet’s picture

Version:6.15» 6.x-dev
StatusFileSize
new491 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user_register_validate.patch.
[ View ]

Same problem here.

Patch solves the problem for me.

pillarsdotnet’s picture

Status:Needs review» Active
Issue tags:-Quick fix, -backport, -quickfix

The bug has apparently been around as long as function itself: Mon, 21 Nov 2005.

pillarsdotnet’s picture

Priority:Critical» Major
Status:Active» Needs review
StatusFileSize
new459 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user_register_validate_0.patch.
[ View ]

Slighly smaller/simpler patch; needs testing.

pillarsdotnet’s picture

Issue tags:+Quick fix, +backport, +quickfix

Tagging. Note that D7 likewise casts $form_state['values'] to an object, in the entity_form_field_validate function.

The user_register_validate function was created Mon, 21 Nov 2005, removed Sat, 10 Oct 2009, and recreated Tue, 30 Nov 2010.

So this patch is technically a backport of the fix incidental to:

pillarsdotnet’s picture

Status:Active» Needs review
Issue tags:+Quick fix, +backport, +CiviCRM, +quickfix

Affects CiviCRM -- that's how I found/fixed it in the first place.

pillarsdotnet’s picture

Title:user_register_validate sends array into hook_user object» user_register_validate() should cast array to object in third parameter of user_module_invoke()

Better title?

pillarsdotnet’s picture

Title:user_register_validate() should cast array to object in third parameter of user_module_invoke()» Third parameter of user_module_invoke() should be object but user_register_validate() calls it with an array instead.

I can't believe I'm bikeshedding this...

pillarsdotnet’s picture

StatusFileSize
new491 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user_register_validate_1.patch.
[ View ]

Patch in #1 was the correct approach. Re-uploading for the sake of clarity.

Gábor Hojtsy’s picture

Looks good. Who else tested this?

pillarsdotnet’s picture

Dunno, but at least one other person (the original reporter) was affected by the problem, and the suggested fix seems self-evident to me. The only other fix I could imagine is to move the cast to user_module_invoke():

D7 patch:
diff --git modules/user/user.module modules/user/user.module
index 7f22f7c..43f96e6 100644
--- modules/user/user.module
+++ modules/user/user.module
@@ -80,6 +80,7 @@ function user_help($path, $arg) {
  * be passed by reference.
  */
function user_module_invoke($type, &$edit, $account, $category = NULL) {
+  $account = (object) $account;
   foreach (module_implements('user_' . $type) as $module) {
     $function = $module . '_user_' . $type;
     $function($edit, $account, $category);
D6 backport:
diff --git modules/user/user.module modules/user/user.module
index 2e12c17..39624e9 100644
--- modules/user/user.module
+++ modules/user/user.module
@@ -16,6 +16,7 @@ define('EMAIL_MAX_LENGTH', 64);
  * be passed by reference.
  */
function user_module_invoke($type, &$array, &$user, $category = NULL) {
+  $user = (object) $user;
   foreach (module_list() as $module) {
     $function = $module .'_user';
     if (function_exists($function)) {

Again, it seems self-evident that the patch in #1/#8 is preferable, especially since the D7 patch above is unnecessary and arguably wrong.

Status:Needs review» Needs work

The last submitted patch, user_register_validate.patch, failed testing.

pillarsdotnet’s picture

Status:Needs work» Needs review
StatusFileSize
new880 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user_register_validate-720876-12.patch.
[ View ]

Trying again...

Status:Needs review» Needs work

The last submitted patch, user_register_validate-720876-12.patch, failed testing.

pillarsdotnet’s picture

Issue tags:-CiviCRM
StatusFileSize
new483 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch user_register_validate-720876-14.patch.
[ View ]

Okay, so the testbot doesn't like it when I follow instructions on how to submit patches.

Trying again with "git diff -no-prefix"

pillarsdotnet’s picture

Status:Needs work» Needs review

Go, testbot!

Status:Needs review» Needs work

The last submitted patch, user_register_validate-720876-14.patch, failed testing.

pillarsdotnet’s picture

Status:Needs work» Needs review
StatusFileSize
new880 bytes

oh, tay.
so maybe testbot is trying (once again) to apply D6 patches to D8 core?

pillarsdotnet’s picture

Status:Needs review» Reviewed & tested by the community
Gábor Hojtsy’s picture

Status:Reviewed & tested by the community» Needs review

Does not seem like anybody tested this in the community beyond the patch creator?!

pillarsdotnet’s picture

I guess nobody cares about fixing long-standing bugs in the d6 api. I get quicker results with d8 bugs, in fact.

Added link on user_register_validate() api page.

pillarsdotnet’s picture

StatusFileSize
new880 bytes
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es).
[ View ]

Re-uploading; I think the testbots are fixed...

pillarsdotnet’s picture

Issue tags:-Quick fix, -backport, -quickfix

Status:Needs review» Needs work
Issue tags:+Quick fix, +backport, +quickfix

The last submitted patch, user_register_validate-720876-21.patch, failed testing.

pillarsdotnet’s picture

pillarsdotnet’s picture

pillarsdotnet’s picture

StatusFileSize
new948 bytes

Renaming patch to avoid useless core testing as suggested by ksenzee in IRC.

pillarsdotnet’s picture

pwolanin’s picture

Status:Needs review» Reviewed & tested by the community
Issue tags:+PHP 5.3 compatibility

So, I've been testing this patch and related code. I think Drupal core masks the error due to the isset() check in function user_user(), so the rest of the code is never called when an anonymous user registers. However, any contrib module implementing hook_user('validate') will see a notice or worse from this bug, and according to the OP PHP 5.3 is less graceful in handling the case of an array accessed as an object.

This test code on PHP 5.2.6 gives me:
FALSE
TRUE

<?php
$x = array();
var_dump(isset($x->uid));
echo "\n";
$x['uid'] = 1;
var_dump(isset($x->uid));
echo "\n";

According to pillarsdotnet PHP 5.3 gives FALSE FALSE which reinforces the fact that 5.2 and 5.3 are different about this.

In any case - the patch is trivial and won't cause any harm and makes core respect its own API.

pillarsdotnet’s picture

Gábor Hojtsy’s picture

Status:Reviewed & tested by the community» Fixed

Committed, pushed, thanks.

Status:Fixed» Closed (fixed)
Issue tags:-Quick fix, -backport, -quickfix, -PHP 5.3 compatibility

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