hook_user* should use $account->uid, not arg(1).

I did a short code review and tested if everything still works, and it looks fine to me. It doesn't cause a change in functionality, except that programmable executions work with this patch. api.drupal.org shows no further usages of user_edit_form() (the only public function whose parameters have changed) apart from the ones that have been ported. The code is even a bit cleaner than before.

I can't see any obstacles for this patch being applied.

This patch also fixes the problem that example for drupal_execute use user_register and they fail to report duplicate user name registrations, getting a database error instead. (Unless you happen to be using a url which has register as arg(1).)

It seems to me that the following actions have to be tested:

• Administrative user account creation by a user with sufficient permissions
• Accessing and saving user profile data of another user by a user with sufficient permissions
• Register as a new user
• Edit own user profile

I would like to get a patch of this size in the development version first. The patch no longer applies cleanly there.

Just wanted to let everyone know that this still works against HEAD.

Is this the patch to apply so that it does not try and validate username when submitting the account edit page in pageroute?

Where do I apply this patch please

Thanks

Can anyone point me in the right direction to apply this patch please? Sorry to bump this again but still need to apply this and am a little confused about where....

Thanks
Gregg

any other newbies with similar problem ... try this link... http://s5.video.blip.tv/1690000331669/Add1sun-ApplyingPatchesToCoreDrupa...
A good introduction to patching and this patch works perfectly to fix the annoying page route edit account issue.

There is so much to learn about Drupal...

This bug not only breaks users made with drupal_execute, but it also prevents modules from using hook_menu_alter to modify the path to the user/edit page, or for modules like 'me' alias where I've used the flexibility of D6 to intercept the %user* loaders and switch out 'me' with the current users uid.

Why do these functions not simply pass on the uid from the account object, instead of trying to get it back out of the request? In fact, why not pass on the entire account object? This would remove a couple of DB hits form the user_load also.

I've re-rolled the above patch with a few minor changes. Please review this so that it can get fixed! :)

Version bump.

Apparently, TortoiseCVS 'make patch' messes up the 'Index' at the top of diff files. Command line it is from now on. :)

Lets try again...

+function _user_edit_validate($user, &$edit) {
+function _user_edit_submit($user, &$edit) {

I think we use $account in such cases, not $user, to prevent unwanted altering of the global $user variable (not [yet!] related to the code this patch touches).

Aside from that, this patch looks good.

Re-roll with sun's suggestion.

More consistent re-roll/back port for D6.

Well that wasn't very clever now was it. Lets see how this goes. :)

Bah. I don't know why I also attached the D6 version that time....

Probably don't try to name your patch ".patch.patch"...

Cross-posted status

Cheers for that Dave. I didn't even notice I had named it patch.patch. :) Would explain it all though. I'll be more careful in future.

Re-roll. Not sure why this failed.

Forgot to do a CVS update before making the patch.

@@ -683,7 +683,7 @@
  */
 function user_user_validate(&$edit, &$account, $category = NULL) {
   if ($category == 'account') {
-    return _user_edit_validate(arg(1), $edit);
+    return _user_edit_validate($account, $edit);
   }
 }
 

found these strings in this patch, I think there should be:

@@ -683,7 +683,7 @@
  */
 function user_user_validate(&$edit, &$account, $category = NULL) {
   if ($category == 'account') {
-    return _user_edit_validate(arg(1), $edit);
+    return _user_edit_validate($account->uid, $edit);
   }
 }
 

If you look further into the patch, you will see that the function _user_edit_validate is also modified, so the patch is correct as is.

sorry, my fault, I missed it.

This actually looks very good. Only one comment:

-  if (user_access('change own username') || user_access('administer users') || !$user->uid) {
+  if (user_access('change own username') || user_access('administer users') || !$uid) {

For extra consistency, that first user_access('change own username') should be user_access('change own username', $account) (but it should go after !$uid, because I'm not sure user_access() will work with a partial user object).

@ Damien I'm not so sure about changing to user_access('change own username', $account). That to me does not make sense.

Shouldn't that permission be checked against the currently logged in user? Unless I'm mistaken, only users with the 'administer users' permission will be able to edit an account that is not their own anyway.

For me changing this would make the code feel less intuitive, as I would read it as "I'm allowed to change the username of an account that I am editing, so long as the owner of the account is allowed to change it".

I'm not against changing this, but I feel it makes more sense the way it is, and I can't see the benefits of changing it. What was your reasoning behind thinking adding the account to user access would make the if test more consistent?

If this is changed here, then it should probably also be changed in user_edit_form.

@cdale: it's just for consistency. The check is in three parts. The logical order would be: I'm allowed to change the user name of $account if:

• $account is a new account (that's actually the last check on !$uid)
• or I'm an user administrator (that's user_access('administer users'))
• or I'm the user and I have the permission to change my own username (that would be $GLOBALS['user']->uid = $uid && user_access('change own username')

Written as it is, the access check works, but makes really little sense.

Fair call. Do you think that the user_access('change own username') in user_edit_form should also be adjusted? My initial impression would be yes.

Would you agree with the following check?

if (!$uid || ($GLOBALS['user']->uid == $uid && user_access('change own username')) || user_access('administer users'))

And in the user_edit_form()

if ($register || ($GLOBALS['user']->uid == $uid && user_access('change own username')) || $admin)

@cdale: looks really better.

@cdale... aside from using $GLOBALS instead of global $user.
EDIT: Oh nevermind, core does it too. Forgive me. :)

Re-roll with above suggestion.

How did this skip my attention for so long? Yes, yes, YES! arg(X) should die.

Committed to CVS HEAD. Changing version and status to D6.

Update and re-roll for D6.

For D6, it would be better not to change function signatures (I know, those are private core functions, but still... we never know what module authors do with them).

I am using the patch #41 on my production site for almost 2 weeks now, and don't observe any issues - so, it shall be RTBC, according to me.

Re-roll with the suggestion in #42.

Bleh, I wish we could change these and not keep the API the same. :(

I've been running this patch just fine with no issues. It's seem simple enough to me, the API does not change, does anyone else think this is RTBC?

Makes complete sense, let's move it in front of Gabor eyes.

Reviewed closely, looks good, committed to Drupal 6, thanks!

Gábor Hojtsy --> Does this mean that the patch is in the 6.11 update, or that it is approved but we still have to patch our 6.10 and 6.11 sites...?

This patch was in the 6.10 release, and will be in any subsequent releases, including 6.11.

subscribing