Clean up API docs for user module

Do you plan to patch this module? If so can you assign the issue to yourself and also add your name to the summary issue? Thanks!

Lars, are you still planning on patching this module?

I will look at doing this. I will update the meta issue.

Ok, here's a patch. Lot's of docblock reformatting to fit within 80 characters and to add line breaks after one sentence summaries. Several functions had no docblock at all, and I have put in one line summaries using the style guide at http://drupal.org/node/1354.

Several functions do not have parameter or return definitions, I will create issues for these.

Seems basically great but there are some unneeded spaces after line end. I found no typos and the comments made sense but I am no native speaker. Please remove the unneeded spaces and I'd say it's RTBC.

Great, will do.

Regarding the functions without param/return - probably the best thing would be to file 1 issue with title "Several functions in user.module lack param/return documentation" and then list them, so we can have one patch rather than a bunch. Thanks!

Regarding the patch in #5 --
a) forms -- at least one doc line you put in is not quite according to our standards:
http://drupal.org/node/1354#forms

+/**
+ * Form validation handler for the user_admin_account() form.
+ */

Should just say "Form validation hanlder for user_admin_account()." (assuming that is the correct name for the form function).

Also, this is not up to standards:

/**
- * Form builder; Request a password reset.
+ * Form builder; request a password reset.

...

+/**
+ * Validation function for the password reset form.
+ */

...

 
+/**
+ * Submit function for the password reset form.
+ */

b)

- * @return boolean TRUE for blocked users, FALSE for active.
+ * @return
+ *   TRUE for blocked users, FALSE for active.

You can leave the "boolean" there... actually it should be "bool". See http://drupal.org/node/1354#functions (scroll down to the Data Types on param/return section)

c)

 /**
- * Helper function to add default user account fields to user registration and edit form.
+ * Helper function to add default user account fields to user registration and
+ * edit form.

First line descriptions must be cut down to 80 characters or less. Also they should start with a verb in the right tense. See
http://drupal.org/node/1354#functions
This occurs in a couple of other places in the patch. Please check the files and make sure all functions conform to the standards, and fix any that don't.

d) typo (contructor):
+ * Form conctructor for the user login block.

Thanks jhodgdon. Will do.

A lot more cleaning up. I've posted a couple more tasks for follow up:

#1419796: Make sure that all form constructors, validators, and submit functions have appropriate @see declarations
#1419792: Move all @ingroup declarations to be at the bottom of docblocks (under all @see declarations)

There's also quite a range of verb tense inconsistencies throughout the module, but altering them all is outside the scope of this sprint I think.

Those two separate cleanup issues you filed should be addressed here on this issue instead, as they are part of the Meta Cleanup task list. Please close those issues as duplicates and include them in this patch. Thanks!

Also, when you make links to other issues on drupal.org, if you type in [#(issue number)], it will turn into a nicer link that shows the issue status and issue title. :)

Verb tenses *are* within the scope of this sprint too. Take a look at the issue summary for this issue. :)

Yes I re-read it after you posted your last reply, and took a deep breath. I'm going to look at finishing this up this week. :)

Ok, changes made as described above.

Ok, looks like I have to learn how to rebase...

Rebased.

+++ b/core/modules/user/user.admin.incundefined
@@ -5,6 +5,9 @@
+/**
+ * Page callback: Shows the user management page.
+ */

Per #1315992: No standard for documenting menu callbacks, I think this needs @param, @return, and an @see to the hook_menu() implementation.

+++ b/core/modules/user/user.admin.incundefined
@@ -1004,14 +1016,19 @@ function user_admin_role_submit($form, &$form_state) {
 /**
- * Form submit handler for the user_admin_role() form.
+ * Form submission handler for user_admin_role().
+ *
+ * Called when a user clicks on the 'Delete Role' button. Redirects the user to
+ * a confirmation dialogue.
  */

I think this one needs @see to its bretheren (other validation and submission handlers for the form).

+++ b/core/modules/user/user.moduleundefined
@@ -284,13 +284,12 @@ function user_external_load($authname) {
+ * @todo Remove $conditions in Drupal 8.

I think there's usually a blank line between between the @todo and any @see/@ingroup, though I'm not positive. (http://drupal.org/node/1354 does not specify.)

+++ b/core/modules/user/user.pages.incundefined
@@ -6,7 +6,8 @@
+ * Menu callback; Retrieves a JSON object containing autocomplete suggestions
+ * for existing users.

Let's reword this so it's less than one 80-character line. Also, I think we need to change the semicolon to a colon, plus add the @param, @return, and @see (As described above).

+++ b/core/modules/user/user.pages.incundefined
@@ -87,7 +98,7 @@ function user_pass_submit($form, &$form_state) {
- * Menu callback; process one time login link and redirects to the user page on success.
+ * Processes one time login link and redirect to user edit page on success.

If this is a menu callback, can we retain the prefix (but add a colon) and reword the rest to fit in 80 chars? I just looked and it's not a menu callback, so disregard. Your change is correct.

+++ b/core/modules/user/user.pages.incundefined
@@ -464,7 +482,7 @@ function user_cancel_methods() {
- * Menu callback; Cancel a user account via e-mail confirmation link.
+ * Menu callback; cancel a user account via e-mail confirmation link.

Per the standard (http://drupal.org/node/1354#menu-callback), I think it should be:
Menu callback: Cancel....

Also, the @param/@return/@see.

+++ b/core/modules/user/user.testundefined
@@ -1284,7 +1286,8 @@ class UserTimeZoneFunctionalTest extends DrupalWebTestCase {
+   * Tests the display of dates and time when user-configurable time zones are
+   * set.

Let's also reword this so that it's less than 80 chars.

Thanks @cam8001!

tagging

man, you guys are so quick on fixing things o_O

keep it up!

Incorporating suggestions from #18.

I looked in a few other core modules, and it seems that @todo declarations are generally preceded by a blank line, so I've amended the docblock for user_load_multiple() to that standard. I read with interest #1315992: No standard for documenting menu callbacks :)

Found more stuff!

1. +++ b/core/modules/user/user.admin.incundefined
   @@ -5,6 +5,17 @@
   + * @param string $callback_arg
   + *   Action to take when viewing the page.
   

   I was going to say that it would be good to have a list of allowed values here... but looking at user_admin(), it's a bit... odd. Maybe a more detailed explaination? I'm also almost tempted to open a followup issue to un-!@#$ this function... maybe after the user entity class conversion.

2. +++ b/core/modules/user/user.admin.incundefined
   @@ -815,10 +831,10 @@ function theme_user_permission_description($variables) {
   - * Form to re-order roles or add a new one.
   + * Form constructor for the user roles add and order form.
   

   Little awkward; maybe "Form constructor for a form to add or reorder user roles"? I'm not 100% sure though.

3. +++ b/core/modules/user/user.admin.incundefined
   @@ -921,11 +937,11 @@ function theme_user_admin_roles($variables) {
   - * Form to configure a single role.
   + * Form constructor for the configure role form.
   

   I'd say "Form submission handler constructor for the role configuration form." EDIT: constructor, not submission handler. (Assuming this is a constructor and not a submission handler; I should have quoted more lines!)

4. +++ b/core/modules/user/user.api.phpundefined
   @@ -195,19 +195,20 @@ function hook_user_format_name_alter(&$name, $account) {
   + *   - label: Required. The label for the operation, displayed in the dropdown
   + *     menu.
   + *   - callback: Required. The function to call for the operation.
   + *   - callback arguments: Optional. An array of additional arguments to pass
   + *     to the callback function.
   

   In these we can omit the "Required." The "optional" should be formatted thus:
   - keyname: (optional) The foody bar baz thing. (Reference: http://drupal.org/node/1354#lists)

5. +++ b/core/modules/user/user.moduleundefined
   @@ -284,13 +284,13 @@ function user_external_load($authname) {
   + * @todo Remove $conditions in Drupal 8.
   + * ¶
   

   Trailing whitespace here.

6. +++ b/core/modules/user/user.moduleundefined
   @@ -2238,9 +2264,11 @@ function user_login_finalize(&$edit = array()) {
   + * Load $user object and perform standard login tasks. The user is then
   + * redirected to the My Account page. Setting the destination in the query
   + * string overrides the redirect.
   

   Should be: "Loads the user object and performs standard login tasks."

7. +++ b/core/modules/user/user.moduleundefined
   @@ -3229,6 +3259,9 @@ function user_multiple_role_edit($accounts, $operation, $rid) {
   + * Form constructor for the cancel multiple user accounts form.
   

   I'd specify here that it's a confirmation form, perhaps:
   Form constructor for the multiple user account cancellation confirmation form.

8. +++ b/core/modules/user/user.moduleundefined
   @@ -3294,9 +3327,8 @@ function user_multiple_cancel_confirm($form, &$form_state) {
   + * Submit handler for user_multiple_cancel_confirm().
   

   Should be: "Form submission handler for..."

9. +++ b/core/modules/user/user.moduleundefined
   @@ -3487,16 +3517,18 @@ function user_preferred_language($account, $default = NULL) {
     * @param $language
   - *   Optional language to use for the notification, overriding account language.
   + *  Optional language to use for the notification, overriding account language.
   ...
     * @return
   - *   The return value from drupal_mail_system()->mail(), if ends up being
   - *   called.
   + *  The return value from drupal_mail_system()->mail(), if it ends up being
   + *  called.
   

   I think the indentation here was correct before the change?

10. +++ b/core/modules/user/user.moduleundefined
    @@ -3675,7 +3707,9 @@ function user_form_field_ui_field_edit_form_alter(&$form, &$form_state, $form_id
     /**
      * Additional submit handler for the 'Edit field instance' form.
      *
    - * Make sure the 'user_register_form' setting is set for required fields.
    + * Makes sure the 'user_register_form' setting is set for required fields.
    + *
    + * @see user_form_field_ui_field_edit_form_alter()
    

    I think this should start with the summary:
    Form submission handler for field_ui_field_edit_form().

11. +++ b/core/modules/user/user.pages.incundefined
    @@ -6,7 +6,15 @@
    + * @return string
    + *   A JSON format list of username suggestions matching the specified string.
    

    I'd say "JSON-formatted string."

12. +++ b/core/modules/user/user.pages.incundefined
    @@ -87,7 +105,7 @@ function user_pass_submit($form, &$form_state) {
    - * Menu callback; process one time login link and redirects to the user page on success.
    + * Processes one time login link and redirects to user edit page on success.
    

    "one-time" should be hyphenated. Also, I think it would be better to say: "Processes a one-time login link..."

13. +++ b/core/modules/user/user.testundefined
    @@ -1284,7 +1286,7 @@ class UserTimeZoneFunctionalTest extends DrupalWebTestCase {
    +   * Tests the display of dates and times with user-configurable time zones.
    

    Let's make "timezone" one word here.

14. +++ b/core/modules/user/user.testundefined
    @@ -1284,7 +1286,7 @@ class UserTimeZoneFunctionalTest extends DrupalWebTestCase {
    diff --git a/core/update.php b/core/update.php
    
    diff --git a/core/update.php b/core/update.php
    index e7e4e5c..309cb87 100644
    
    index e7e4e5c..309cb87 100644
    --- a/core/update.php
    
    --- a/core/update.php
    +++ b/core/update.phpundefined
    
    +++ b/core/update.phpundefined
    +++ b/core/update.phpundefined
    

    This whole section at the end of the patch is from a different commit... be sure to rebase before rolling your patch. ;)

Also, in general, adding parameter datatypes is not part of this cleanup sprint, but since what you've added is for new parameter documentation for the menu callbacks, which are fairly self-evident, I think it's okay.

Thanks!

Thanks for the feedback xjm, I'll clean things up!

I must apologise for the lag here, I was away for the weekend and have since been very unwell with a super-cold. Not forgotten! Will look at this week.

Submitting the patch after working on suggestions pointed out in #22. Please review.

Looks pretty good! I think most of the items in review #22 are taken care of. A few details:
4.

+ *   - label: The label for the operation, displayed in the dropdown
+ *     menu.

This has to be rewrapped -- the word "menu" will fit on the previous line.

6.

@@ -2211,9 +2237,11 @@ function user_login_finalize(&$edit = array()) {
 }
 
 /**
- * Submit handler for the login form. Load $user object and perform standard login
- * tasks. The user is then redirected to the My Account page. Setting the
- * destination in the query string overrides the redirect.
+ * Form submission handler for user_login().
+ *
+ * Loads the $user object and performs standard login tasks. The user is then
+ * redirected to the My Account page. Setting the destination in the query
+ * string overrides the redirect.
  */
 function user_login_submit($form, &$form_state) {

Should not be $user, but just "user" without the $.

7.

@@ -3202,6 +3232,9 @@ function user_multiple_role_edit($accounts, $operation, $rid) {
   }
 }
 
+/**
+ * Form constructor for the multiple user account cancellation confirmation form.
+ */
 function user_multiple_cancel_confirm($form, &$form_state) {

That made the line go longer than 80 characters, so it needs some kind of a rewrite.

Other things I noticed:
a)

@@ -2766,7 +2796,7 @@ function user_mail_tokens(&$replacements, $data, $options) {
 /*** Administrative features ***********************************************/

Take any lines like this you see out.

b)

+++ b/core/modules/user/tests/user_form_test.module
@@ -23,7 +23,7 @@ function user_form_test_menu() {
 }
 
 /**
- * A test form for user_validate_current_pass().
+ * Form constructor for test form for user_validate_current_pass().
  */
 function user_form_test_current_password($form, &$form_state, $account) {

This one-line description needs "the" or something in it to help it read better, and it should have an @see to the corresponding submission handler.

c)

@@ -104,7 +115,7 @@ function user_filter_form() {
 }
 
 /**
- * Process result from user administration filter form.
+ * Form submission handler for user_filter_form_submit().
  */
 function user_filter_form_submit($form, &$form_state) {

First line should refer to user_filter_form() not _submit().

d)

@@ -642,11 +660,11 @@ function user_admin_settings() {
 }
 
 /**
- * Menu callback: administer permissions.
+ * Form constructor for the administer permissions form.
  *
- * @ingroup forms
  * @see user_admin_permissions_submit()
  * @see theme_user_admin_permissions()
+ * @ingroup forms
  */
 function user_admin_permissions($form, $form_state, $rid = NULL) {

This needs documentation of the $rid parameter. Similarly, user_admin_role() needs param docs, and user_admin_role_delete_confirm().

e) user.api.php

@@ -357,14 +358,15 @@ function hook_user_view($account, $view_mode, $langcode) {
 /**
  * The user was built; the module may modify the structured content.

This doesn't follow the standards at http://drupal.org/node/1354#hooks
And just below that in the same hook:
f)

+ * This hook is called after the content has been assembled in a structured
+ * array and may be used for doing processing which requires that the complete
+ * user content structure has been built.

second line should be
array, and may be used for doing processing that requires...
[comma, which/that]

g)

+++ b/core/modules/user/user.entity.inc
@@ -12,6 +12,9 @@
  */
 class UserController extends DrupalDefaultEntityController {
 
+  /**
+   * Implements DrupalDefaultEntityController::attachLoad().
+   */
   function attachLoad(&$queried_users, $revision_id = FALSE) {

This function is an override of a base class method, not an implementation of an interface method.

h) As in #6 above:

@@ -375,7 +377,8 @@ function user_load_by_name($name) {
  *   serialized and saved in the {users.data} column.
  *
  * @return
- *   A fully-loaded $user object upon successful save or FALSE if the save failed.
+ *   A fully-loaded $user object upon successful save or FALSE if the save
+ *   failed.

This should not be talking about $user but just "user" without the $. It would probably be good to check throughout the module for this and fix all of them. The only exception is if it's really referring to "global $user", such as here:

+ * current user, based on username. Either way, the global $user object is
  * populated and login tasks are performed.

i)

@@ -1178,6 +1178,9 @@ function user_user_presave(&$edit, $account) {
   }
 }
 
+/**
+ * Form constructor for the user login block.
+ */
 function user_login_block($form) {

I don't think this is really a formal form constructor... why doesn't it have $form_state as an argument if it is? Check that...

j)

@@ -1974,15 +1988,17 @@ function user_get_authmaps($authname = NULL) {
 }
 
 /**
- * Save mappings of which external authentication module(s) authenticated
- * a user. Maps external usernames to user ids in the users table.
+ * Saves a list of which external authentication module(s) authenticated a user.
+ *
+ * Maps external usernames to user ids in the users table.

use ID not id to refer to an identifier. The latter word "id" is a psychological concept.

k)

 /**
- * A FAPI validate handler. Sets an error if supplied username has been blocked.
+ * Form validation handler for user_login().
+ *
+ * Sets an error if supplied username has been blocked.
  */
 function user_login_name_validate($form, &$form_state) {

Needs "the" in that "Sets an error" sentence.

l)

@@ -2186,10 +2211,11 @@ function user_authenticate($name, $password) {
 }
 
 /**
- * Finalize the login process. Must be called when logging in a user.
+ * Finalizes the login process.
  *
- * The function records a watchdog message about the new session, saves the
- * login timestamp, calls hook_user op 'login' and generates a new session. *
+ * Must be called when logging in a user. Records a watchdog message about the
+ * new session, saves the login timestamp, calls hook_user op 'login' and
+ * generates a new session.
  */
 function user_login_finalize(&$edit = array()) {

Wow that bit about calling hook_user op 'login' is outdated! We haven't had hook_user() with ops since Drupal 6. It's now hook_user_login() and all function references should also have () after them.

m)

+ *   Status constant indicating if role was created or updated. Failure to write
+ *   the user role record will return FALSE. Otherwise, SAVED_NEW or
+ *   SAVED_UPDATED is returned depending on the operation performed.
  */
 function user_role_save($role) {
   if ($role->name) {

Needs "the" in first line "if the role was...".

n) Another "needs the" spot:

@@ -2910,7 +2940,7 @@ function user_role_save($role) {
 }
 
 /**
- * Delete a user role from database.
+ * Deletes a user role from database.

o)

@@ -3612,7 +3644,7 @@ function user_block_user_action(&$entity, $context = array()) {
 /**
  * Implements hook_form_FORM_ID_alter().
  *
- * Add a checkbox for the 'user_register_form' instance settings on the 'Edit
+ * Adds a checkbox for the 'user_register_form' instance settings on the 'Edit
  * field instance' form.
  */
 function user_form_field_ui_field_edit_form_alter(&$form, &$form_state, $form_id) {

First line needs to say what form is being altered. See 2nd example in:
http://drupal.org/node/1354#hookimpl

p) This is out of scope for API docs and should be removed from the patch:

+++ b/core/modules/user/user.test
@@ -165,7 +165,7 @@ class UserRegistrationTestCase extends DrupalWebTestCase {
     $this->assertEqual($new_user->signature, '', t('Correct signature field.'));
     $this->assertTrue(($new_user->created > REQUEST_TIME - 20 ), t('Correct creation time.'));
     $this->assertEqual($new_user->status, variable_get('user_register', USER_REGISTER_VISITORS_ADMINISTRATIVE_APPROVAL) == USER_REGISTER_VISITORS ? 1 : 0, t('Correct status field.'));
-    $this->assertEqual($new_user->timezone, variable_get('date_default_timezone'), t('Correct time zone field.'));
+    $this->assertEqual($new_user->timezone, variable_get('date_default_timezone'), t('Correct timezone field.'));

q) Generally, even after this patch user.test is a mess. It needs to be cleaned up. It has such "lovely" "docs" as:
- No docblock on the classes at all, and many of the methods [but note that getInfo(), setUp() and tearDown() should not have doc blocks]
- wrong verb tenses
- This kind of a mess:

  /**
   * Do the test:
   *  GD Toolkit is installed
   *  Picture has invalid dimension
   *
   * results: The image should be uploaded because ImageGDToolkit resizes the picture
   */
  function testWithGDinvalidDimension() {

r) After looking at this file and seeing all the stuff in there that wasn't fixed in this patch, I am wondering if similar problems exist in the other files -- it would be good to look them all over when rolling the next patch, and make sure that all the functions are documented, and documented properly.

Here's a patch that includes everything but the test documentation. I will start that now against what is currently in trunk, but I just wanted to get this stuff up and reviewed so that I don't get too out of sync with trunk before getting any feedback.

I had a bit of confusion rebasing this, so if it is poorly formatted, I may need to re-roll.

[removed duplicate post]

#28: user-documentation_cleanup_sprint-1326666-28.patch queued for re-testing.

Looks like the patch needs a re-roll...

rerolled

I have attempted to fix these issues a few times, but I just can't get the old, out of date patches to apply at all to the vastly different code thats now on trunk. I really don't want to manually reintegrate the patch.

Does anyone have any tips on how to get those old changes onto the current trunk short of doing it all manually?

First of all, probably drnikki and cam8001 should figure out who is working on this, and assign the issue, because obviously having two people both working on it wouldn't be great! :)

Second, rerolling. There are some suggestions here: http://drupal.org/patch/reroll

Also, if you use the "patch" command to apply the patch instead of "git apply", there are some "fuzz" options that can help, and it will also apply as much as it can and create a .rej file to show you which parts it can't figure out. That will probably reduce the parts that need to be applied manually quite a bit.

I'm happy to step aside if you're happy to step up, drnikki :P

@cam8001 - i honestly dont mind doing it. I just picked it because I was at the NYC camp sprint and was looking for something to work on. I also certainly didn't mean to overstep. I'm on IRC if you want to chat - drnikki - and we can figure out why both of our patches failed. :)

taking it

rerolled the patch from #26 so it applies to 8.x. I couldn't get the later patches to apply to anything cleanly. I'm guessing I'll have to do those manually once this passes testing.

NIce work!

Including notes from above.

This looks great, thanks! And in contrast to some other cleanup issues that have been sitting around for 4-5 weeks with no review, I decided to review this one today...

A few things to fix up:

a) Form constructor functions: When there are arguments other than $form and $form_state, they need to be documented. This applies to quite a few of the functions in this patch, such as

function user_admin_permissions($form, $form_state, $rid = NULL) {

b)

+ * Called when a user clicks on the 'Delete Role' button. Redirects the user to
+ * a confirmation dialogue.
+ *
+ * @see user_admin_role_delete_confirm()
+ * @see user_admin_role_delete_confirm_submit()
  */
 function user_admin_role_delete_submit($form, &$form_state) {

- The button is actually 'Delete role' (lower-case r)
- I have mostly seen the spelling "dialog" in documentation. Let's standardize on that (applies elsewhere in this patch also).

c) I think the code changes in the hook_help() are correct, but they need to be put on a separate issue.

d) in user_set_authmaps:

+ * Maps external usernames to user ids in the users table.

Should be IDs. "id" is a psychological term. "ID" means identifier.

e)

+ * new session, saves the login timestamp, calls hook_user op 'login' and
+ * generates a new session.
  */
 function user_login_finalize(&$edit = array()) {

This isn't accurate. In D7 it would be hook_user_login() not hook_user op 'login'.

f)

@@ -2623,7 +2643,7 @@ function user_mail_tokens(&$replacements, $data, $options) {
 /*** Administrative features ***********************************************/

We should get rid of junk lines like this.

g)

/**
- * Delete a user role from database.
+ * Deletes a user role from database.

Needs the: "from the database".

h)

+/**
+ * Form constructor for the multiple user account cancellation confirmation form.
+ */
 function user_multiple_cancel_confirm($form, &$form_state) {

- This line exceeds 80 characters.
- Missing the @ingroup forms and the @see to the validate/submit functions.

i) In user_preferred_language():

+ * This user preference can be set on the user account editing page, and is only
+ * available if there are more than one languages enabled on the site. If the

Grammar: ... if there is more than one language enabled...

I can move out the help text once this passes. I don't want to create too much confusion.

Also, re: "Form constructor for the multiple user account cancellation confirmation form." - it's *just* too long, so I added a line break, but I think a reword might be better. Just not sure which would be better.

Thanks for reviewing! It's almost done....

Thanks! I think it still needs a bit more work... Looking at the interdiff:

a)

+ * @param account
+ *   user account object
+ *

This doesn't follow our standards for param documentation (should start with capital letter and end with a period), and it should be a bit more grammatical, like "A user account object". Also, the parameter is $account (needs a $). The other added params have the same problems.

b) And regarding this change:

/**
- * Form constructor for multiple user account cancellation confirmation form.
+ * Form constructor for the multiple user account
+ * cancellation confirmation form.
+ *
+ * @see user_multiple_cancel_confirm_submit()
+ * @ingroup forms
  */
 function user_multiple_cancel_confirm($form, &$form_state) {

We really really really do not want the first-line description to wrap to two lines, or to exceed 80 characters. How about making it:

"Form constructor for the multiple account cancellation confirmation form."
?

c) Also I think you didn't get all the "dialogue" spellings in there. :)

In the spirit of teamwork.

Also:

██ cam8001 @ ~/Sites/drupal/core/modules/user
██ 21:54:51 $ grep -nr dialogue *

██ cam8001 @ ~/Sites/drupal/core/modules/user
██ 21:54:52 $ 

I think we got them all?

Edit: Mucked up the interdiff sorry :( Plz ignore

This looks mostly great! Just a couple of things to address:

a) This coding change to user_help() seems to contradict itself (one change goes in one direction and the other seems to reverse it) and is *not* part of the API docs cleanup -- please file a separate issue and remove this from the patch:

-      $output .= '<dd>' . t('<em>Roles</em> are used to group and classify users; each user can be assigned one or more roles. By default there are two roles: <em>anonymous user</em> (users that are not logged in) and <em>authenticated user</em> (users that are registered and logged in). Depending on choices you made when you installed Drupal, the installation process may have defined more roles, and you can create additional custom roles on the <a href="@roles">Roles page</a>. After creating roles, you can set permissions for each role on the <a href="@permissions_user">Permissions page</a>. Granting a permission allows users who have been assigned a particular role to perform an action on the site, such as viewing a particular type of content, editing or creating content, administering settings for a particular module, or using a particular function of the site (such as search).', array('@permissions_user' => url('admin/people/permissions'), '@roles' => url('admin/people/roles'))) . '</dd>';
+      $output .= '<dd>' . t('<em>Roles</em> are used to group and classify users; each user can be assigned one or more roles. By default there are two roles: <em>anonymous user</em> (users that are not logged in) and <em>authenticated user</em> (users that are registered and logged in). Depending on choices you made when you installed Drupal, the installation process may have defined more roles, and you can create additional custom roles on the <a href="@roles">Roles page</a>. After creating roles, you can set permissions for each role on the <a href="@permissions_user">Permissions page</a>. Granting a permission allows users who have been assigned a particular role to perform an action on the site, such as viewing a particular type of content, editing or creating content, administering settings for a particular module, or using a particular function of the site (such as search).', array('@permissions_user' => url('admin/people/permissions'), '@roles' => url('admin/people/permissions/roles'))) . '</dd>';
       $output .= '<dt>' . t('Account settings') . '</dt>';
       $output .= '<dd>' . t('The <a href="@accounts">Account settings page</a> allows you to manage settings for the displayed name of the anonymous user role, personal contact forms, user registration, and account cancellation. On this page you can also manage settings for account personalization (including signatures and user pictures), and adapt the text for the e-mail messages that are sent automatically during the user registration process.', array('@accounts'  => url('admin/config/people/accounts'))) . '</dd>';
       $output .= '</dl>';
@@ -59,8 +59,8 @@ function user_help($path, $arg) {
     case 'admin/people/create':
       return '<p>' . t("This web page allows administrators to register new users. Users' e-mail addresses and usernames must be unique.") . '</p>';
     case 'admin/people/permissions':
-      return '<p>' . t('Permissions let you control what users can do and see on your site. You can define a specific set of permissions for each role. (See the <a href="@role">Roles</a> page to create a role). Two important roles to consider are Authenticated Users and Administrators. Any permissions granted to the Authenticated Users role will be given to any user who can log into your site. You can make any role the Administrator role for the site, meaning this will be granted all new permissions automatically. You can do this on the <a href="@settings">User Settings</a> page. You should be careful to ensure that only trusted users are given this access and level of control of your site.', array('@role' => url('admin/people/roles'), '@settings' => url('admin/config/people/accounts'))) . '</p>';
-    case 'admin/people/roles':
+      return '<p>' . t('Permissions let you control what users can do and see on your site. You can define a specific set of permissions for each role. (See the <a href="@role">Roles</a> page to create a role). Two important roles to consider are Authenticated Users and Administrators. Any permissions granted to the Authenticated Users role will be given to any user who can log into your site. You can make any role the Administrator role for the site, meaning this will be granted all new permissions automatically. You can do this on the <a href="@settings">User Settings</a> page. You should be careful to ensure that only trusted users are given this access and level of control of your site.', array('@role' => url('admin/people/permissions/roles'), '@settings' => url('admin/config/people/accounts'))) . '</p>';
+    case 'admin/people/permissions/roles':

b)

@@ -3096,7 +3121,7 @@ function user_multiple_cancel_confirm($form, &$form_state) {
 }
 
 /**
- * Submit handler for mass-account cancellation form.
+ * Form submission handler for user_multiple_cancel_confirm().
  *
  * @see user_multiple_cancel_confirm()
  * @see user_cancel_confirm_form_submit()

I think the @see lines here can be omitted -- the first is covered by the new summary line, and I think (?) the second points to the function that is being documented (not sure about that)?

c)

@@ -92,7 +110,7 @@ function user_pass_submit($form, &$form_state) {
 }
 
 /**
- * Menu callback; process one time login link and redirects to the user page on success.
+ * Processes a one-time login link and redirects to user edit page on success.
  */
 function user_pass_reset($form, &$form_state, $uid, $timestamp, $hashed_pass, $action = NULL) {

This function needs some @param docs... also is it a form constructor? is it a page callback? If so, document as such.

d)

@@ -457,10 +484,18 @@ function user_cancel_methods() {
 }
 
 /**
- * Menu callback; Cancel a user account via e-mail confirmation link.
+ * Menu callback: Cancels a user account via an e-mail confirmation link.

I think this should be starting with "Page callback"?

a) Reverted to the same as head.

b) Removed the redundant @see. The other one refers to a different form entirely, as the multiple user cancel workflow is a bit complex and requires a reference I think.

c) This has been changed to "Form constructor: processes a one-time login link."

d) Changed.

Note that the following functions have been removed entirely from head (ea25df), so they have fallen out of the patch.

user_register_form
user_register_validate
user_register_submit
user_profile_form_validate
user_profile_form_submit

Issue for their removal is here:

#1499596: Introduce a basic entity form controller

Rebased for current head.

Thanks @cam8001 for keeping this issue moving. I reviewed the full patch and just found a couple things:

+++ b/core/modules/user/user.admin.incundefined
@@ -681,11 +700,14 @@ function user_admin_settings_submit($form, &$form_state) {
+ * @param rid
+ *   (optional) role id

The parameter name should be listed with the $: $rid, and the description needs capitalization and a period. Also, we should be a little more specific. I looked up user_admin_permissions() in the API docs and I'd suggest:

A role ID. When specified, the admin form will be built for a single role. Otherwise, the form will be built for all roles.

+++ b/core/modules/user/user.admin.incundefined
@@ -979,10 +999,14 @@ function theme_user_admin_roles($variables) {
+ * @param object $role
+ *   A user role object.

@@ -1046,14 +1070,25 @@ function user_admin_role_submit($form, &$form_state) {
+ * @param object $role
+ *   A user role object.

Let's explain here how the role is used in the function (see previous example).

+++ b/core/modules/user/user.moduleundefined
@@ -38,10 +38,10 @@ const USER_REGISTER_VISITORS = 'visitors';
-  global $user;
+global $user;

This change is not correct.

Thanks for the patch! I reviewed about 2/3 of it carefully (skimmed the rest too), and found a number of one-time or recurring things that need to be cleaned up:

a)

+ * @return array
+ *  A Form API renderable array.
+ *
+ * @see user_menu()
+ */
 function user_admin($callback_arg = '') {

Needs another space of indent in the @return -- same here (second line):

+ * @return string|null
+ *   A string expressing a possible syntax issue or NULL if no syntax
+ *  validation issues were detected.
  */
 function user_validate_name($name) {

b)

/**
- * Form builder; Return form for user administration filters.
+ * Form construcotr to return a form for user administration filters.
  *
- * @ingroup forms
  * @see user_filter_form_submit()
+ *
+ * @ingroup forms
  */
 function user_filter_form() {

constructor is misspelled; also take out "to return a" and rewrite slightly -- standards are at http://drupal.org/node/1354#forms

c) Could use a "the":

+ * Form constructor for user administration page.
  *
- * @ingroup forms
  * @see user_admin_account_validate()
  * @see user_admin_account_submit()
+ *
+ * @ingroup forms
  */
 function user_admin_account() {

d)

/**
- * Menu callback: administer permissions.
+ * Form constructor for the administer permissions form.
...
 function user_admin_permissions($form, $form_state, $rid = NULL) {

This is a page callback, so the first line should have prefix "Page callback: " on it.

e) First line verb tenses in user.api.php are incorrect for hooks, see
http://drupal.org/node/1354#hooks

f) This line in user_load_multiple has an extra space after .:

+ *   (optional) An array of entity IDs. Defaults to NULL.  If omitted, all
+ *   entities are loaded.

As does this one in user_password():

+ *   (optional) The desired length of the generated random password.  Defaults

g) First @param needs description:

+ *
+ * @param array $element
+ *
+ * @param mixed $input
+ *   This variable is not used by this function, but is included for consistency
+ *   with similar functions in other modules.
+ * @param array $form_state
+ *   The current state of the form, passed by reference.
+ *
+ * @return string
+ *   The preferred language code.
  */
 function _user_language_selector_langcode_value($element, $input, &$form_state) {

h)

+/**
+ * Form constructor for the user log in block.
+ *
+ * @param array $form
+ *   A FAPI form definition array.
+ *
+ * @return array
+ *   The modified FAPI form defintion array.
+ */
 function user_login_block($form) {

Omit @param/@return for form constructors and needs @ingroup and @see to validate/submit functions.

i)

+ *   TRUE if the current user is considered anonymous; othersie, FALSE.
+ */
 function user_is_anonymous() {

spelling: othersie

j) user_register() and user_register_access() and similar functions - we don't ever say "Menu callback" -- should be "Page callback" and "Access callback" -- see http://drupal.org/node/1354#menu-callback -- this applies to user_page_title() too.

k)

+ *   Returns a Boolean, a user ID or the original argument passed in.
+ *
  * @todo D8: Remove.
  */
 function user_uid_only_optional_to_arg($arg) {

Needs comma before "or" -- applies to user_uid_optional_to_arg() too.

l)

+ * @return string
+ *   The title of the page to use depending upon wether the current user is
+ *   logged in and the requested path.
  */
 function user_menu_title() {

wether is misspelled ... not sure what this means either? maybe a little rewrite or a comma?

m) Did I already say this? e.g. is often mispunctuated... should be "the text; e.g., a, b, or c".

Here we go!

I ran some spellchecks through and cleaned up all I could find.

This patch needs to have all the cleanups that are not specifically part of our written-down standards, or that are not cleaning up actual documentation, removed from it.

For instance:

- * @ingroup forms
  * @see user_filter_form_submit()
+ *
+ * @ingroup forms

We do not have a standard that requires @ingroup to be separated from @see by a blank line; nor a standard that says @ingroup comes after @see. Putting changes like this into the patch wastes reviewer/committer time. Please supply a new patch without this type of change, and then I'll be happy to review it fully.

Not sure how those got in! Heres an amended patch.

Thanks! Looks better.

A few things to address:

a) In user.api.php:

- * @see user_cancel_methods()
  * @see user_cancel_confirm_form()
+ * @see user_cancel_methods()
  */
 function hook_user_cancel_methods_alter(&$methods) {

I think this order was more logical previously (was change in a misguided attempt to alphabetize @see which is not in our standards). There are still a bunch of these changes in the patch. Please remove them. The one for user_login_default_validators() in user.module is especially blatantly more illogical as an alphabetical list too.

b) This change at the top of user.module is also applying a non-existent "standard" that seems to be "use lines for vendor classes should be separate from our own classes":

-use Drupal\Core\Database\Query\SelectInterface;
-use Drupal\Core\Entity\EntityInterface;
-use Drupal\file\File;
-use Drupal\Core\Template\Attribute;
-use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
-
 /**
  * @file
  * Enables the user registration and login system.
  */
 
+use Drupal\Core\Database\Query\SelectInterface;
+use Drupal\Core\Entity\EntityInterface;
+use Drupal\Core\Template\Attribute;
+use Drupal\file\File;
+
+use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
+

Moving the @file to the top is good, but leave the use lines as they were.

c) user.module

* @param string $authname
- *   The external authentication username.
+ *   The external authentication username as a string.

"as a string"... this is not adding any useful information. Just clutters up the documentation.

Same with this addition of "defaults to NULL":

- *   (optional) An array of entity IDs. If omitted, all entities are loaded.
+ *   (optional) An array of entity IDs. Defaults to NULL. If omitted, all
+ *   entities are loaded.

Same with "as a string" here:

+ * @param string $name
+ *   The name to verify as a string.

There are a bunch of other examples... check the patch.

d) user.module

-      // If the image does not have a valid Drupal scheme (for eg. HTTP),
+      // If the image does not have a valid Drupal scheme (for e.g. HTTP),

if you are going to fix this line really, take out "for" and put a comma after e.g. (e.g. means "for example").

e) user.module - Access callbacks by convention do not have @return docs

f) user.module

+ *
+ * @return string
+ *   The title of the page to use depending upon whether the current user is
+ *   logged in, and what the current url path is.
  */
 function user_menu_title() {

url -> URL

g) user.module

+ * @return string
+ *   If $account is an object, an unsanitized string with the username to
+ *   display. The code receiving this result must ensure that check_plain() is
+ *   called on it before it is printed to the page; otherwise, an empty string.
  */
 function user_page_title($account) {

The wording here is unclear to me. The "otherwise" clause belongs in the first sentence, not the second.

h) user.module

- * Sets a form error if user has not been authenticated, or if too many
- * logins have been attempted. This validation function should always
- * be the last one.
+ * This function sets a form error if user has not been authenticated, or if too
+ * many logins have been attempted. This validation function should always be
+ * the last one to be called.

This kind of change, where you add "This function..." before some perfectly good existing documentation, is not necessary at all and adds clutter. Please remove these changes -- there are several in the patch.

So's this one later in the same file:

- *   Unused parameter required by the token_replace() function.
+ *   This is an unused parameter required by the token_replace() function.
  */
 function user_mail_tokens(&$replacements, $data, $options) {

And this added "a" does not add clarity in the same file:

/**
- * Fetches a user role by role ID.
+ * Fetches a user role by a role ID.

i) user.module

+ *   (optional) A keyed array of settings. Defaults to an empty array. Supported
+     array keys are:
  *   - langcode: A language code to be used when generating locale-sensitive
- *    URLs. If langcode is NULL the users preferred language is used.
+ *    URLs. If langcode is NULL, the user's preferred language is used.
 

Bad list indentation and missing a * on one line.

j) user.module

+ * @ingroup batch
  */
 function user_cancel($edit, $uid, $method) {

This is not a good @ingroup. The batch topic is for the core basic batch functions. Not sure why this was added? Take it out of every place it was added (there are several in the patch).

k) user.module

/**
- * Page callback wrapper for user_view().
+ * Page callback: Creates a wrapper for user_view().
+ *
+ * @param mixed $account
+ *   The user object.
+ *
+ * @throws Symfony\Component\HttpKernel\Exception\NotFoundHttpException
+ *
+ * @see user_menu()
  */
 function user_view_page($account) {

huh? First line description isn't illuminating.... Say what the page shows.

l) user.module

+ *
+ * @see user_access()
  */
 function user_role_delete_access($role) {

This @see is not appropriate.

m) user.module - doc for _user_mail_notify():

+ * Sends notification emails to users.

Should be "e-mail messages". We use a hyphen as per the style guide, and e-mail is not countable so it cannot be pluralized with an s.

n)

+ * @return array
+ *   The key will be 'destination' unless 'user/login' is found within the
+ *   desired path. In that event, the value will be set to 'user'; otherwise,
+ *   the value will be the path provided via the destination query string or,
+ *   if not available, the current path.
  */
 function user_login_destination() {

This documentation does not make sense to me. I cannot figure out what is being returned.

o) user.pages.inc

/**
- * Process variables for user-profile.tpl.php.
+ * Processes variables for user-profile.tpl.php.
  *
- * The $variables array contains the following arguments:
- * - $account
+ * @param array $variables
+ *   An associative array contains the following arguments:
+ *   - $account: A user, node or comment object with 'name', 'uid' and 'picture'
+ *     fields.

The @param needs some attention. Arrays don't hav earguments and $account is not the array key.

p) user.pages.inc

+ *   (optional) A unix timestamp of when the cancellation request was generated.
+ *   Defaults to zero.
+ * @param string $hashed_pass
+ *   (optional) A unique hash based on the $account's password and $timestamp.
+ *   Defaults to an empty string.
+ *
+ * @throws
  *
  * @see user_cancel_confirm_form()
  * @see user_cancel_url()
+ * @see user_menu()
  */
 function user_cancel_confirm($account, $timestamp = 0, $hashed_pass = '') {

unix -> UNIX
And that @throws needs the name of an exception class.
And don't say "the $account". If you want to use $account, don't use "the". If you want to use "the", don't use a $ on "account".

q) While you're at it, you could fix the first line for user_page() in user.pages.inc -- totally inaccurate.

I am sprinting at a Drupal drop-in tommorow, so I will clean up these issues then. Could I ask that no one else works on this patch until then? It seems a lot of issues were introduced at some point in this comment queue which I am now going to have to track down and clean up. I'm not sure where a lot of these things come from, so I want to get back to a clean state.

cam8001 - thanks! Assign the issue to yourself. :)

I would like to add documentation on the $method parameter to user_cancel().

#62 no longer applies to 8.x-dev and I'm not able to re-roll right now, but here's a small patch for user_cancel.

Whoops added to wrong sub-section.

These issues are a lot of work with very little tangible payoff, so I'm closing the rest of them as "won't fix". Your efforts on working on this issue were appreciated... it was just my fault for starting a task that was very difficult to get right.

Let's instead put our effort into fixing and reviewing documentation that is really unclear and/or wrong, and I hope that the people who worked on these issues are not afraid to jump into a more reasonable issue!