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.

CommentFileSizeAuthor
#28 310212.patch7.21 KBRobLoach
#27 310212.patch7.5 KBRobLoach
#24 310212_0.patch9.88 KBdawehner
#22 310212.patch9.17 KBRobLoach
#20 user_op_comment_4.patch10.62 KBdawehner
#18 user_op_comment_3.patch7.7 KBdawehner
#14 310212.patch9.64 KBRobLoach
#13 user_op_comment_1.patch9.8 KBdawehner
#5 user.patch25.79 KBAnonymous (not verified)
#3 user.patch25.8 KBAnonymous (not verified)
#1 user.patch25.69 KBAnonymous (not verified)
hook_user.patch23.23 KBAnonymous (not verified)
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anonymous’s picture

FileSize
25.69 KB

added some phpdoc for new functions.

took out the if ($hook == 'user') hack in 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?

catch’s picture

Category: feature » bug
Priority: Normal » Critical

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.

Anonymous’s picture

FileSize
25.8 KB

@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.

Anonymous’s picture

will update this patch once HEAD is fixed.

Anonymous’s picture

FileSize
25.79 KB

updated for HEAD, all tests pass.

catch’s picture

Status: Needs review » Reviewed & tested by the community

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.

Dries’s picture

Status: Reviewed & tested by the community » Needs work

Committed to CVS HEAD.

I'm marking this code needs work until the upgrade docs have been updated.

Thanks! Awesome patch.

RobLoach’s picture

Status: Needs work » Fixed

Added the update module documentation for remove $op.

Anonymous’s picture

Status: Fixed » Closed (fixed)

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

catch’s picture

Status: Closed (fixed) » Needs work

api docs haven't been updated yet: http://api.drupal.org/api/function/hook_user/7

RobLoach’s picture

On a rather unrelated topic of docs, #314870: UNSTABLE-4 blocker: Add api.php for every core module would really be nice....

catch’s picture

Just noticed this still needs docs (now in user.api.php).

dawehner’s picture

FileSize
9.8 KB

this patch rewrites the user.api.php to a removed op

TODO: examples for the hooks

RobLoach’s picture

Title: remove $op from hook_user » Remove $op from hook_user: Documentation
Component: base system » user.module
Status: Needs work » Needs review
FileSize
9.64 KB

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!

dawehner’s picture

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?

Dries’s picture

There seems to be some inconsistent line-wrapping going on in the last version.

Status: Needs review » Needs work

The last submitted patch failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
7.7 KB

this is the new version, just copyed http://drupal.org/files/issues/310212.patch and resolved the reject by hand.

catch’s picture

Status: Needs review » Needs work

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
dawehner’s picture

FileSize
10.62 KB

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.
dawehner’s picture

Status: Needs work » Needs review
RobLoach’s picture

FileSize
9.17 KB

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:

/**
 * 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.

Status: Needs review » Needs work

The last submitted patch failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
9.88 KB

reroll

Status: Needs review » Needs work

The last submitted patch failed testing.

moshe weitzman’s picture

Any chance we can get a reroll? This is a significant doc bug.

RobLoach’s picture

Status: Needs work » Needs review
FileSize
7.5 KB
RobLoach’s picture

FileSize
7.21 KB

Whoops, wrong patch.

moshe weitzman’s picture

Status: Needs review » Needs work

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.

Berdir’s picture

Please have a look at #491972: Fix horrifying mess of crap in user module hooks.

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...

casey’s picture

This issue seems to be fixed.

casey’s picture

Status: Needs work » Fixed

Status: Fixed » Closed (fixed)

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