Remove $op from hook_user: Documentation
justinrandell - September 18, 2008 - 13:14
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | user.module |
| Category: | bug report |
| Priority: | critical |
| Assigned: | justinrandell |
| Status: | needs work |
Description
first cut, this is just rearranging mostly.
motivation comes from lots of places, good discussion on the dev list here.
all tests pass. (as an aside, huge thanks to all who've worked on simpletest, definitely saved me a bunch of time when creating this patch.)
still needs heaps of code docs, and another cut to take advantage of the fact that we don't have to use the one-size-fits-all API. might be a separate issue or two though.
| Attachment | Size |
|---|---|
| hook_user.patch | 23.23 KB |
| Testbed results | ||
|---|---|---|
| hook_user.patch | failed | Failed: Failed to apply patch. Detailed results |
#1
added some phpdoc for new functions.
took out the
<?phpif ($hook == 'user')
?>
module.inc(thanks chx).all tests pass except for 3 in 'Upload user picture', which are failing on a clean HEAD for me at the moment.
any opinions on whether this will make it into D7?
#2
We shouldn't release D7 with any $ops left in it IMO.
A few things I noticed -
statistics.module is missing phpdoc for Implementation of...
trigger.module has // Nasty hack... - surely // @TODO?
We should be passing $user as an object to all of these hooks, and probably using module_invoke_all for them too, but that could be a followup patch.
I looks like we should remove hook_user_form and just use hook_form_alter() instead unless I'm missing something - again - fine with that being a followup patch.
#3
@catch: thanks for quick review. i like the idea of passing in $user, but the intention of this patch is to just remove $op, then we can make the api better with follow up patches. trying to do as webchick suggests with simple patches...
added phpdoc for statistics.module.
added TODO for trigger.module.
#4
will update this patch once HEAD is fixed.
#5
updated for HEAD, all tests pass.
#6
So there's plenty of followup work to do once this gets in - like use module_invoke_all instead of user_module_invoke, pass it a user object, get rid of the fapi stuff etc. etc. - but that can all be done in subsequent patches. I confirmed that all tests pass, so marking RTBC.
#7
Committed to CVS HEAD.
I'm marking this code needs work until the upgrade docs have been updated.
Thanks! Awesome patch.
#8
Added the update module documentation for remove $op.
#9
Automatically closed -- issue fixed for two weeks with no activity.
#10
api docs haven't been updated yet: http://api.drupal.org/api/function/hook_user/7
#11
On a rather unrelated topic of docs, #314870: UNSTABLE-4 blocker: Add api.php for every core module would really be nice....
#12
Just noticed this still needs docs (now in user.api.php).
#13
this patch rewrites the user.api.php to a removed op
TODO: examples for the hooks
#14
It was missing hook_user_form. I also think that the implementation examples of the hooks should be added after this patch, due to the kittens. Nicely done!
#15
oh damn i deleted it because, i missed it because i read something with hook_nodeapi and remove of $op = 'form'
but i don't trust myself to be able to set to RTBC, anyone else, reading the documentation?
#16
There seems to be some inconsistent line-wrapping going on in the last version.
#17
The last submitted patch failed testing.
#18
this is the new version, just copyed http://drupal.org/files/issues/310212.patch and resolved the reject by hand.
#19
Still some line wrapping issues - i.e.:
+ * The user account is being added. The module should save its+ * custom additions to the user object into the database and set the saved
#20
i'm not sure about the load, login and logout hooks, see the old comment
it seems to be than the last line should be under load, so i did this
* - "load": The user account is being loaded. The module may respond to this* - "login": The user just logged in.
* - "logout": The user just logged out.
* and insert additional information into the user object.
#21
#22
It seems like "and insert additional information into the user object." is the prefix of "The module may respond to this". This patch includes that fix.
Also, looking at other implementations of hook_*, we see that there is an empty line between the first line and the next line of the documentation:
<?php
/**
* Perform periodic actions.
*
* Modules that require to schedule some commands to be executed at regular
* intervals can implement hook_cron(). The engine will then call the hook
* at the appropriate intervals defined by the administrator. This interface
* is particularly handy to implement timers or to automate certain tasks.
* Database maintenance, recalculation of settings or parameters, and
* automatic mailings are good candidates for cron tasks.
*
* @return
* None.
*
* This hook will only be called if cron.php is run (e.g. by crontab).
*/
function hook_cron() {
$result = db_query('SELECT * FROM {site} WHERE checked = 0 OR checked + refresh < :time', array(':time' => REQUEST_TIME));
foreach ($result as $site) {
cloud_update($site);
}
}
?>
This is also fixed in this patch.......... Documentation line margin set to 80.
#23
The last submitted patch failed testing.
#24
reroll
#25
The last submitted patch failed testing.
#26
Any chance we can get a reroll? This is a significant doc bug.
#27
#28
Whoops, wrong patch.
#29
Thanks.
Only the 'view' hook gets a $category. For example,
module_invoke_all('user_logout', NULL, $user);.+ * The module should validate its custom additions to the user object,+ * registering errors as necessary.
Instead of 'user object', say $edit.
#30
Please have a look at #491972: Fix and document hook_user_*().
I created the other issue without knowing about this one. In my patch, I removed all unecessary arguments of those hooks. I haven't yet looked at this, but if this contains better docs, we should merge it and if not, close one of them as duplicate.
Edit: I've also removed those unecessary & from $account, as that is an object...