Posted by Lars Toomre on October 30, 2011 at 7:12pm
9 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | documentation |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
| Issue tags: | docs-cleanup-2011, needs backport to D7, Novice |
Issue Summary
Problem/Motivation
Part of meta-issue #1310084: [meta] API documentation cleanup sprint
Proposed resolution
Correct the following in the core user module:
- Ensure that all
@returnlines are preceded by a blank line. - Ensure that
@seeand@ingrouplines are always at the end of docblocks. - Ensure that all lines in doxygen blocks are 80 characters or fewer (excluding the terminal newline).
- Ensure that all functions, classes, interfaces, and methods have one-line summaries that are clear, use the correct verb tense, and follow specific standards in http://drupal.org/node/1354.
- Make incidental style and grammar corrections only to those docblocks already being updated.
Remaining tasks
Patch needs to be rolled.
User interface changes
None.
API changes
None.
Follow-up issues
None.
Comments
#1
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!
#2
Lars, are you still planning on patching this module?
#3
Nope... Just set up the task item.
#4
I will look at doing this. I will update the meta issue.
#5
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.
#6
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.
#7
Great, will do.
#8
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.
#9
Thanks jhodgdon. Will do.
#10
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.
#11
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. :)
#12
Verb tenses *are* within the scope of this sprint too. Take a look at the issue summary for this issue. :)
#13
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. :)
#14
Ok, changes made as described above.
#15
The last submitted patch, user-documentation_cleanup_sprint-1326666-14.patch, failed testing.
#16
Ok, looks like I have to learn how to rebase...
#17
Rebased.
#18
+++ 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!
#19
tagging
#20
man, you guys are so quick on fixing things o_O
keep it up!
#21
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 :)
#22
Found more stuff!
+++ 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.
+++ 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.
+++ 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 handlerconstructor 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!)+++ 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)+++ b/core/modules/user/user.moduleundefined@@ -284,13 +284,13 @@ function user_external_load($authname) {
+ * @todo Remove $conditions in Drupal 8.
+ * ¶
Trailing whitespace here.
+++ 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."
+++ 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.+++ 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..."
+++ 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?
+++ 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().+++ 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."
+++ 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..."
+++ 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.
+++ 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!
#23
Thanks for the feedback xjm, I'll clean things up!
#24
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.
#25
Since there hasn't been activity for 4+ weeks, I'm going to move this back to unassigned.
#26
Submitting the patch after working on suggestions pointed out in #22. Please review.
#27
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.