Revamp hook_user_form/_register/_validate/_submit/_insert/_update

subscribing- I'd be interested in taking a shot at some of this refactoring.

Bumping so I remember to work on this

subscribe

Would love to see this done in 7.x. Probably would make it play nicer with the registry, too.

dear webchick, user module needs a lot of love. Even if we are not going to have a generalized object API (?), then just to bring this module up to D5 standard (overall, much less D7) is a boatload of work and pain for zero (end user) visible gain.

So, it must be done, but we also need some mechanism to make it worthwhile. Maybe X hours of access to webchick and Dries on the secret bat phone? A case of beer? We need some way to make updating these crufty core modules rewarding and fun.

Hm. How about cartoon doodles? :) I like doing those. :D

PICTURE IT. A cartoon detailing the triumph of the spunky hero(ine) __________ battling the evil, soul-sucking cruft of user.module! Action! Mayhem! Romance! Er. Maybe not.

subscribe

subscribing

I need to fix #367006: [meta] Field attach API integration for entity forms is ungrokable, which is why I subscribed. Not sure whether it's about right to still get this into D7.

Renaming $user to $current_user sounds like total scope creep to me. That can be done at any time later on in a follow-up patch. ...which probably reduces this patch to about 60% of it's size.

So I have 2 options now: Either fix the current bug by introducing $form + $form_state to hook_user_form(), or try to redo this patch here from scratch, but limited to user forms. In case of the latter, that would only cover some points of your list.

Taking over.

wow, this might pass already!

uhm, what?

Let's see what the bot thinks now.

Awesome, 13 less :)

Let's try this.

This one should pass?

PASS!

Do it more nicely.

Now we need a RTBC here. :)

Better title. ;)

@nedjo: Sorry for hi-jacking this issue. We can revert later on, or just move the remaining parts into a new issue ;)

Nice cleanup.

Someone should do the same job on hook_comment() soon (see the comments in tracker.module hook implementations for the wtfs).

+++ modules/user/user.module	22 Sep 2009 01:19:38 -0000
@@ -1819,25 +1813,30 @@ function user_edit_form($form, &$form_st
   $form['account']['mail'] = array('#type' => 'textfield',
...
   );
   if (!$register) {
+    $form['account']['mail']['#default_value'] = $account->mail;
+  }
+  if (!$register) {
     $form['account']['pass'] = array('#type' => 'password_confirm',

In advance... this looks kinda wonky when looking at the patch only. But when looking at the actual code, it makes more sense, because all of those form elements are basically set up separately.

+++ modules/user/user.module	22 Sep 2009 01:19:38 -0000
@@ -1855,7 +1854,7 @@ function user_edit_form($form, &$form_st
     $form['account']['status'] = array(
...
-      '#default_value' => isset($edit['status']) ? $edit['status'] : 1,
+      '#default_value' => 1,

Looks wrong. This patch should fix.

This review is powered by Dreditor.

+++ modules/user/user.module	22 Sep 2009 02:06:01 -0000
@@ -1876,11 +1875,10 @@ function user_edit_form($form, &$form_st
-      $default = empty($edit['roles']) ? array() : array_keys($edit['roles']);
...
-        '#default_value' => $default,
+        '#default_value' => !empty($account->roles) ? array_keys($account->roles) : array(),

empty() makes no sense. Actually never made sense when looking at the old code. ;)

Even more with the new code, which turns $account into http://api.drupal.org/api/function/drupal_anonymous_user/7 here. So we basically always have $account->roles set up.

Anyway, replacing that with isset().

This review is powered by Dreditor.

This is awesome. One thing we should really do is to give each category its own form, aliased to user_profile_form with a hook_forms(). That would allow us to remove the crappy #user_category stuff. But this can definitely wait for another issue.

sub

This is completely off-topic, but I'm always amazed at how an innocent looking thread, with beautiful continuity, has a one year gap in the middle of it. I ♥ open source, and you too Drupal! :D

Very nice clean-up. Committed to CVS HEAD. Thanks for the resurrection, sun.

What about docs?

Yep, needs docs.

.

Thank you, thank you, thank you. Great change.

Yay !! Sun++++

c123456 (Christian, please get a proper nick ;) asked me why && !$admin slipped into the condition for user pictures here. I have no idea, so here comes a quick follow-up.

Committed to CVS HEAD. Thanks.

Reverting status for documentation.

ok, this was nice - but unfortunately, it left some weird things behind:

• hook_user_validate() was previously used to validate the form elements added via hook_user_form() and hook_user_register(). Of course, modules are supposed to add further #validate or #element_validate properties to the form now - just like in any other form_alter situation.
• Strangely, hook_user_validate() was previously also used to attach data to the $edit array (submitted form values) in case the form element was not present on the user form. (?!) See http://api.drupal.org/api/function/block_user_validate/7 for one of those weird examples.
• hook_user_submit() was previously used to prepare the submitted form values before user_save() is invoked. See http://api.drupal.org/api/function/user_user_submit/7
• hook_user_insert() was and still is used to prepare/alter properties on the $user object after user_save() has been invoked, which are stored in {users}.data. See http://api.drupal.org/api/function/contact_user_insert/7 for an example. user_save() contains some special code that allows to set a custom $user->my_value property to NULL to remove it from the stored data. Note that #347988: Move $user->data into own table intends to improve or remove that facility altogether, so please do not follow up in this issue about that. Regardless of that issue, this issue will have to resolve the current weirdness.

In general, the remaining things should be easy to solve since there are only a few instances in core - as long as we have an agreement.

Proposal:

1. Drop hook_user_validate(). hook_form_alter() implementations have to add #validate or #element_validate handlers accordingly. Form values that have to end up in the submitted form values need to use the #access property to hide the form element in the form, but still submit the value.
2. Drop hook_user_submit(). hook_form_alter() implementations have to add #submit handlers.

   This bears a problem though: In order to add a form submit handler before the user form's main submit handler (where user_save() is invoked), a module would have to use

   array_unshift($form['#submit'], 'mymodule_user_submit');
   

   Actually, I'm not even sure why a module would want or have to run a submit handler before the main submit handler, so this may not be an issue at all.

3. hook_user_insert()... In an ideal world, in order to prepare/remove data from {users}.data, modules should invoke a form submit handler before the main user form's submit handler. This would allow us to remove the duplicate user_save() for {users}.data that is happening after hook_user_insert()/hook_user_update() have been invoked.

   However, that won't work out, because user data should also be alterable by just invoking user_save(), i.e. without having a form in the process at all.

   Hence, it seems like we want to move all hook_user_validate() and hook_user_submit() implementations, which do not store data in a custom storage, to hook_user_insert()/hook_user_update(), to properly prepare the $user properties for {users}.data. For example:

   function block_user_validate(&$edit, $account, $category) {
     if (empty($edit['block'])) {
       $edit['block'] = array();
     }
     return $edit;
   }
   

   Becomes:

   // Blocks are only configurable for already registered users.
   function block_user_update(&$edit, $account, $category) {
     // Remove the $user->block property in case no blocks are configurable.
     if (empty($edit['block'])) {
       $edit['block'] = NULL;
     }
     return $edit;
   }
   

There is already a patch for the first step of the way: #585834: Kill hook_user_validate().

To be able to move forward in #585834: Kill hook_user_validate() we need an agreement here. Please comment on the proposal in #41.

ok. What I am doing here:

- Remove hook_user_validate() + hook_user_submit(). All of that is dealt with via hook_form_alter().

- Rename user_edit_form() to user_account_form(). Always invoking the corresponding _validate() and _submit() handler. And also revamping this stub form builder by moving conditions into proper #access properties, which allows us to clean up the corresponding _validate() and _submit() functions, which was the No. 1 trigger for totally senseless contrib modules, which have to do crazy workarounds just to achieve form elements that already exist in core, but simply can't be activated.

- Solving the users.data storage problem I outlined earlier by cleaning up hook names: hook_user_update() becomes hook_user_presave(), allowing modules to set $user properties to NULL, so they don't get stored in users.data. Additionally, that strangely named hook_user_after_update() becomes hook_user_update().

If tests pass, this patch is fairly complete.

oh, neaty - just forgot to remove a stale variable :)

This one will pass.

Green! :)

btw, there's even more to do here: hook_user_categories() and all the magic around it no longer makes sense. Profile module and contributed modules now use the Menu API and Form API to add tabs to the user account page(s). That, we can tackle in a separate issue though.

Hint: In an ideal world, we'd just have user_save($account), just like we have node_save($node) -- all those strange arguments date back to D...

+++ modules/openid/openid.module	4 Oct 2009 15:28:15 -0000
@@ -452,7 +452,7 @@ function openid_authentication($response
-      $account = user_save('', $form_state['values']);
+      $account = user_save(drupal_anonymous_user(), $form_state['values']);

The first time I saw this with drupal_anonymous_user()...it was a major WTF flag for me because it is not clear what is going on with that.

Everything else looks and works good. Very nice cleanups.

I'm on crack. Are you, too?

Yeah, I can see that. As visible from that snippet though, we passed an empty string to user_save() previously, while user_save() expects an $account, which equally looked like a WTF to me.

Hence, I changed that to pass the anonymous user account (it's an $account at least). That also guarantees that all hook_user_* implementations can safely check $account->uid without having to check whether it's set first.

So the question is: What do we want to pass in case a new account shall be created?

Unfortunately, we can't look at other APIs, because as mentioned before, most use a more modern pattern of just user_save($account), where ->uid is either =0 (insert) or >0 (update).

Two options to resolve this:

1. $account = drupal_anonymous_user();
   $account = user_save($account, $form_state['values']);
   

2. $account = user_save(NULL, $form_state['values']);
   

   ...while user_save() would internally call drupal_anonymous_user() in case $account is not an object (i.e. NULL, '', 0, or whatever).

Implemented option 2) of #49, which keeps the existing behavior, i.e. user_save(NULL, $edit) creates a new account.

This should be ready to fly now.

+++ modules/block/block.module	8 Oct 2009 12:47:04 -0000
@@ -425,43 +425,34 @@ function block_form_user_profile_form_al
+    // Only display the fieldset if there are any personalizable blocks.

Personalizable isn't a real word, maybe it's a neologism. But I found 775k mentions on google so seems common enough.

That's my only nitpick, and it doesn't matter, so RTBC.

I'm on crack. Are you, too?

Great clean-up! However, since you already turned this into a kitten slaughter-fest, let's finish the job.

+++ modules/user/user.api.php	8 Oct 2009 12:47:05 -0000
@@ -218,7 +196,32 @@ function hook_user_categories() {
+ * A user account is about to be created or updated.
+ *
+ * The module should save its custom additions to the user object into the
+ * database and set the saved fields to NULL in $edit.

Ok, but /why/ do I want to use this hook?

+++ modules/user/user.api.php	8 Oct 2009 12:47:05 -0000
@@ -237,10 +240,33 @@ function hook_user_insert(&$edit, $accou
+ * Use this if (probably along with 'insert') if you want to reuse some
+ * information from the user object.

This documentation is completely useless. When do I want to use this hook? Also, add a @see to hook_user_insert() (and vice-versa)

+++ modules/user/user.module	8 Oct 2009 12:54:56 -0000
@@ -308,6 +308,13 @@ function user_load_by_name($name) {
+  // If $account is empty, then the caller intends to create a new account. We
+  // load the anonymous user account, which will never be overwritten, but
+  // allows to pass a proper $account object to hook_user_*() implementations.
+  if (empty($account)) {
+    $account = drupal_anonymous_user();
+  }

I like this from a consistency/predictability standpoint, but are we sure this is wise? If anonymous is treated like any other old $account, might I not accidentally do something evil to the anon user account, like add them to the admin role? :P

+++ modules/user/user.module	8 Oct 2009 12:54:56 -0000
@@ -3032,48 +3049,82 @@ function user_block_user_action(&$object
+function user_register($form, &$form_state) {

Let's name this user_register_form(), for consistency with user_account_form().

+++ modules/user/user.module	8 Oct 2009 12:54:56 -0000
@@ -3032,48 +3049,82 @@ function user_block_user_action(&$object
-  $mail = $form_state['values']['mail'];
...
+  $form_state['values']['init'] = $form_state['values']['mail'];
+

Where does mail actually get set, then?

I'm on crack. Are you, too?

Tagging absolutely critical clean-ups for D7. Do not touch this tag.

Ok, but /why/ do I want to use this hook?

Yes, User API makes no sense. It never did. But we only have a few days left.

I like this from a consistency/predictability standpoint, but are we sure this is wise? If anonymous is treated like any other old $account, might I not accidentally do something evil to the anon user account, like add them to the admin role?

Yes, the code already takes care for that, and, the tests do, too. In fact, every single test that is run.

Let's name this user_register_form(), for consistency with user_account_form().

I take no responsibility for that scope-creepish-patch-bloat. :P

Where does mail actually get set, then?

Unconditionally here:

function user_account_form(&$form, &$form_state) {
...
  $form['account']['mail'] = array(
    '#type' => 'textfield',
    '#title' => t('E-mail address'),
    '#maxlength' => EMAIL_MAX_LENGTH,
    '#description' => t('A valid e-mail address. All e-mails from the system will be sent to this address. The e-mail address is not made public and will only be used if you wish to receive a new password or wish to receive certain news or notifications by e-mail.'),
    '#required' => TRUE,
    '#default_value' => (!$register ? $account->mail : ''),
  );
...
}

Re-rolled against HEAD. This one will pass again.

Cool, thanks. Those docs still need a bit of work, but we can definitely handle those in follow-up patches.

I'm very happy to blow this inane part of user module away. :) And +1 for more consistency with comment/node APIs.

Committed to HEAD! This needs to be documented in the upgrade docs.

.

Not quite. This is still critical because we lack docs.

#653068: Update documentation is incomplete

+++ modules/profile/profile.module	22 Sep 2009 00:55:32 -0000
@@ -378,7 +364,17 @@ function _profile_form_explanation($fiel
+function profile_form_alter(&$form, &$form_state, $form_id) {
+  if ($form_id == 'user_register' || $form_id == 'user_profile_form') {
+    $register = ($form['#user']->uid > 0 ? FALSE : TRUE);
+    $form = array_merge($form, profile_form_profile($form['#user'], $form['#user_category'], $register));
+  }
+}

When I wrote this patch, I wasn't really aware of #300481: Add hook_form_FORM_ID_post_alter() to run after non-specific hook_form_alter()...

this needs to be fixed, potentially also in all other locations these patches touched.

Powered by Dreditor.

However, aside from that, added docs to http://drupal.org/node/727352

These update docs are now on the main update page http://drupal.org/update/modules/6/7

I think this can be closed. If there is an issue still pending, I suggest opening a new issue.