Fix horrifying mess of crap in user module hooks
Berdir - June 15, 2009 - 09:47
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | user.module |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
Description
The patches does the following things:
- Adding apidocs for all hook_user_* hooks.
- Remove unecessary & from $user and $account
- Remove unecessary arguments (for example, change hook_user_view(&$edit = NULL, $account, $category = NULL) to hook_user_view($account), remove $category from login/logout hook and so on.)
- Remove hook_profile_alter hook. there is simply no need for that, as it is doing exactly the same as hook_user_view($account).
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| user_hook_fixes.patch | 25.4 KB | Idle | Failed: Failed to apply patch. | View details | Re-test |
#1
The last submitted patch failed testing.
#2
Re-roll.
#3
The last submitted patch failed testing.
#4
HEAD is broken : http://testing.drupal.org/node/35
#5
The last submitted patch failed testing.
#6
Re-roll, there was a conflict in system.api.php
#7
The last submitted patch failed testing.
#8
Another re-roll...
#9
The last submitted patch failed testing.
#10
D'oh, another conflict in system.api.php.. nobody wants to review this ? :)
#11
The last submitted patch failed testing.
#12
core was broken
#13
New title, suggested by webchick!
#14
+++ modules/blog/blog.module 13 Jul 2009 11:47:37 -0000@@ -44,7 +44,7 @@ function blog_access($op, $node, $accoun
-function blog_user_view(&$edit, &$user, $category) {
+function blog_user_view($user) {
Since we're cleaning things up anyway, let's rename $user to $account in all hook implementations for consistency.
+++ modules/profile/profile.module 13 Jul 2009 11:47:41 -0000@@ -205,49 +205,49 @@ function profile_block_view($delta = '')
* Implement hook_user_form().
+++ modules/user/user.api.php 13 Jul 2009 11:47:53 -0000
@@ -263,6 +211,181 @@ function hook_user_categories() {
+function hook_user_register(&$edit, $account, $category) {
+ if (variable_get('configurable_timezones', 1)) {
Out of scope for this issue certainly, but since you had your head this deep into user module, do we still need these hooks? Isn't hook_form_alter() just as good, or no?
+++ modules/system/system.api.php 13 Jul 2009 11:47:45 -0000@@ -566,19 +566,6 @@ function hook_link_alter(array &$links,
-function hook_profile_alter(&$account) {
- foreach ($account->content AS $key => $field) {
- // do something
- }
-}
I'm not sure about removing this hook.
This was added back in #74395: hook_profile_alter() - let modules alter the profile page, and $account was passed by reference even in the dark days of Drupal 5. Unfortunately, there's not much there in the way of community discussion around why the change was necessary.
However, the overall pattern of "build up something by querying all modules that want to add primary content, then allow modules to alter the entirety of the primary content" seems pervasive wherever I search for a call to drupal_alter(). So IMO, let's leave it in, with an eye towards removing it in D8 if it really is unnecessary.
Alternately, get Moshe to chime in here and see what he has to say.
+++ modules/user/user.api.php 13 Jul 2009 11:47:53 -0000@@ -12,79 +12,6 @@
-function hook_user($op, &$edit, &$account, $category = NULL) {
(and 1200 other - lines)
This makes me so happy I could squeal like a little girl. ;)
+++ modules/user/user.api.php 13 Jul 2009 11:47:53 -0000@@ -251,9 +178,30 @@ function hook_user_operations() {
+function hook_user_after_update(&$edit, $account, $category) {
@@ -263,6 +211,181 @@ function hook_user_categories() {
+function hook_user_logout($account) {
...
+function hook_user_submit(&$edit, $account, $category) {
...
+function hook_user_validate(&$edit, $account, $category) {
...
+function hook_user_view($account) {
Is there any way I can convince you to add examples for these hooks? I'm not confident that if they don't get added now that they ever will be.
+++ modules/user/user.api.php 13 Jul 2009 11:47:53 -0000@@ -263,6 +211,181 @@ function hook_user_categories() {
+ * The user account is being added.
...
+ * The user account is being changed.
Let's keep past-tense like elsewhere: the user account was just added/changed.
+++ modules/user/user.module 13 Jul 2009 11:47:57 -0000@@ -23,7 +23,7 @@ define('EMAIL_MAX_LENGTH', 64);
+function user_module_invoke($type, &$array, $user, $category = NULL) {
Can we rename $array as $edit now?
20 days to code freeze. Better review yourself.
#15
#16
Done.
Not sure, hook_form_alter is probably enough but as you said, nothing for this issue.
As I said in IRC, the view hook worked differently back in D5: http://api.drupal.org/api/function/user_view/5. New stuff was returned so it was not possible to change things added by other modules, but this is not true anymore. The hook was probably already unecessary in D6, but was not removed. I will pink moshe about this, to be certain.
I added *something*, it does not make much sense, but it's better than nothing I guess...
As discussed, insert/update is called before the account was added/changed, but I changed it in a few other instances...
Can we rename $array as $edit now?
Done.
I discussed this with webchick in #drupal and we found lots and lots of funny things in user.module and profile.module and others.. I fixed some of them in the patch and leaving others for follow-ups or this will never be finished.
Other changes in this patch:
- removed contact_user_validate.. that does just return an array since Drupal 4.6, however the validate hook does not expect an return value :) More "-" lines, yay!
- There is still hook_user_register(&$edit, $account = NULL, $category = NULL), but that goes through _user_forms() and is more complicated to simplify.
- Renamed profile_view_profile to profile_user_view and profile_validate_profile to profile_user_validate, as the current hooks just passed through the arguments. the other hooks handle $register = TRUE/FALSE so I didn't change them.
Follow-Up Stuff:
- user_edit/user_profile_form/user_edit_form is quite a mess, there are submit/validate functions for functions that aren't form functions and other nice stuff.
#17
Committed to CVS HEAD. Great job, Berdir.
I'm marking this 'code needs work' so we can follow up on the user_edit/user_profile_form/user_edit_form stuff and other tidbits.
#18
Also, let's document this sucker.
#19
Added http://drupal.org/update/modules/6/7#hook-user-changes.
This removes the mentioned user_edit_* code that is not used. It also simpliefies user_admin() a bit, by moving create out of it.
#20
very nice work here ... can we please please get rid of the switch on $_POST. Thats pre-fapi code right there.
#21
The last submitted patch failed testing.
#22
@moshe
I tried, but I can't get the cancel stuff working. Attached is the patch that does convert that part to be similiar to the node_admin_content stuff. However, node multiple delete doesn't work for me either, so I'm not sure what is wrong exactly.
The test fails were because I forgot to update the category menu entries.
#23
#24
The last submitted patch failed testing.
#25
This should work now, without $_POST. Not sure what I changed, though.... I moved the multiple_cancel form functions to user.admin.inc and then it worked...
#26
#27
Lovely. Many thanks.
#28
This is a great little clean-up! :D
+++ modules/user/user.admin.inc 13 Aug 2009 11:26:50 -0000@@ -6,24 +6,20 @@
+/**
+ * Form builder; Return user administration forms.
+ *
+ * @ingroup forms
+ */
Convention is to @see document the validation/submit functions associated with a form.
+++ modules/user/user.admin.inc 13 Aug 2009 11:26:50 -0000@@ -6,24 +6,20 @@
+function user_admin(&$form_state) {
While we're at it, can we make this "user_admin_form" to make it more explicit what it is at a glance?
+++ modules/user/user.admin.inc 13 Aug 2009 11:26:50 -0000@@ -42,6 +38,7 @@ function user_filter_form() {
+ $form['#submit'][] = 'user_filter_form_submit';
@@ -153,6 +147,8 @@ function user_admin_account() {
+ $form['#theme'] = 'user_admin_account';
@@ -171,6 +167,8 @@ function user_admin_account() {
+ '#submit' => array('user_admin_account_submit'),
+ '#validate' => array('user_admin_account_validate'),
Why is this necessary? Doesn't the submit function fire automatically based on naming convention of $form_id?
+++ modules/user/user.admin.inc 13 Aug 2009 11:26:50 -0000@@ -973,3 +975,77 @@ function user_modules_uninstalled($modul
+function user_multiple_cancel_confirm(&$form_state) {
PHPDoc please.
Also see note about @see.
18 days to code freeze. Better review yourself.
#29
Hm.. the thing is, there are no submit/validate functions for user_admin_form. It just calls other form functions and they have validate/submit functions. Not sure if I should add all of them as that would be quite a list ;)
As above, $form_id is user_admin_form. That doesn't match.
Working on a re-roll...
#30
Ok, re-rolled without @see for all submit/validate functions, they aren't listed on http://api.drupal.org/api/function/node_admin_content/7 either.
#31
Fixed a recurring spelling error in http://drupal.org/update/modules/6/7#hook-user-changes (unecessary to unnecessary).
Edit: however, I don't have permissions to fix this in http://drupal.org/node/394070. Anyone who does have permissions is welcome to fix it.
#32
@Berdir: Could you take a look at #549726: user_profile_form_validate() not called when submitting user_profile_form ? Maybe related to the recent changes, maybe not, but your work here definitely makes you one of the carbon-based organisms most intimate with the current state of user.module ;-)
#33
The last submitted patch failed testing.
#34
Can someone re-roll this? I don't have time until code freeze I think...
#35
Here is a try.
Re-added the user_edit() helper function to make DamZ happy, as he didn't liked drupal_set_message() in a form callback though I can't really understand why that is bad :)
Untested.
#36
The last submitted patch failed testing.
#37
This patch would be really nice to still get in. AFAICS, it doesn't break any public API function of User module. But we don't want to waste time in re-rolling if it doesn't have a chance, so please let us know.
#38
I absolutely love the issue title here. Hitting the subscribe button.