Many of the Doxygen comments in the user module don’t follow the coding standards on http://drupal.org/node/1354

The patch fixes any Doxygen comment that didn’t follow the formatting standards and sprinkles some extra bits of documentation into a few. No code changes.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

JohnAlbin’s picture

FileSize
10.59 KB

Wrong patch.

JohnAlbin’s picture

Status: Needs review » Needs work

Need to re-roll.

TR’s picture

Version: 6.x-dev » 8.x-dev
Assigned: Unassigned » TR
Priority: Minor » Normal
Status: Needs work » Needs review
Issue tags: +Needs backport to D7
FileSize
40.86 KB

The attached patch fixes Doxygen comments in the user module according to http://drupal.org/node/1354. In addition to coding standards changes, comments were added for several undocumented functions.

Nothing was changed except comments.

Patch applies to both Drupal 8.x and Drupal 7.x.

Ralt’s picture

Overall looks good, but I'm not sure "user object" should be named "$user object".

TR’s picture

Both "$user object" and "user object" were being used in user.module. I changed them all to be $user for consistency and because the Doxygen comments used this style for other objects like $account.

Ralt’s picture

Status: Needs review » Reviewed & tested by the community

Ok, then this patch is really good for readability. Marking as RTBC (no need to test since only comments are changed).

pillarsdotnet’s picture

Status: Reviewed & tested by the community » Needs work

This patch should be broken into separate issues for each modified documentation block.

jhodgdon’s picture

Component: user.module » documentation

This should be in the documentation component unless it affects code, which it doesn't.

pillarsdotnet: NO. Please. Don't. Break. Up. Documentation. Patches. It's silly.

However, the patch does need work for a few items:
a)

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

Please don't take types out of @param statements. The types are helpful for some people and they *do* conform to our standards.

b)

- * Fetches a user object based on an external authentication source.
+ * Fetches a $user object based on an external authentication source.

...
  * @return
- *   A fully-loaded user object if the user is found or FALSE if not found.
+ *   A fully-loaded $user object if the user is found or FALSE if not found.

Why did you put $user in here? It's not $user, which is a Drupal global. It's a particular user's account. Same goes to other places you did the same thing in this patch. Please put them back the way they were.

That's where I stopped reviewing...

jhodgdon’s picture

By the way, if you have any other all-doc issues, could you please move them to the documentation component too, or at least tag them "needs documentation", so that the Documentation Team is aware of them? Thanks.

TR’s picture

Re: #8 a)

This one function in the user.module is the ONLY place in core that types a @param with "string".

The documentation standards (http://drupal.org/node/1354) say:
If the data type of a parameter or return value is not obvious or expected to be of a special class or interface, it is recommended to specify the data type in the @param or @return directive
The standards go on to show an example where the types "object" and "array" are documented in this manner.

The use of "@param string" in the user.module is in a context where the argument type is pretty obvious. I'm not against using "string" here, but if it is used here there are literally hundreds of other places in core where "string" should be used in the @param description. I would rather have everything documented consistently, rather than have this one and only one exception in core.

Note also there are a few (very few) instances in core that type a @param with "array" or "object". user.module uses "object" in two places, and I did not touch those because I felt that the use of "object" was consistent with the documentation standards.

shortcut.module contains four @param typed "object".
system.module contains two @param typed "object".
user.module contains two @param typed "object".

comment.module contains four @param typed "array".
system.module contains four @param typed "array".
trigger.module contains one @param typed "array".

I would prefer to address typing (or not) of all core @params in a separate issue, rather than promoting an inconsistent usage of typing in core as it stands now. Again, note there are NO other instances of using type "string" for a @param, and I don't think it's justified here. That's why I removed it.

Re: #8 b)

As I explained in #5, "Both "$user object" and "user object" were being used in user.module. I changed them all to be $user for consistency and because the Doxygen comments used this style for other objects like $account." Personally, I could go either way, but I think that $user emphasizes that we're talking about a specific variable. Plain old "user object" on the other hand is ambiguous, because it could mean any user-provided object. I expect people will complain either way, whether it is $user or "user", but there should be ONE convention that is adhered to, rather than using both interchangeably. Likewise, if $user is to become "user", $account should be "account", etc., throughout the doxygen comments. Perhaps we should make the distinction between the global $user variable and a local copy which should be referred to as $account, but this usage is highly inconsistent within the user.module and making that change would be more disruptive than I intended for this one simple patch.

Re: #9

could you please move them to the documentation component too, or at least tag them "needs documentation"

I will make a practice to tag these issues "needs documentation" in the future. In the past, I have been admonished for NOT categorizing (e.g.) user.module patches with the user.module component, for the similar reason that user.module maintainers won't see it otherwise ... Using the user.module component and the "needs documentation" tag seems the best way to deal with these two competing interests.

I will re-roll this patch after hearing back about the best way to proceed with the above, and after some of the other small user.module documentation comment patches (which break this patch) have been committed.

jhodgdon’s picture

Sorry, but I disagree on all of those points...

8 a) There is no reason that I can see to remove a clarification of a particular param, just because no one has clarified other params.

8 b) The only justification that I can see for using $abc in a narrative description is if it refers to a function parameter called $abc or a Drupal global variable $abc. Just changing 'user object' to '$user object' globally makes zero sense to me, as it refers to neither the global $user, nor a function parameter (we NEVER use $user as a function parameter, to avoid confusion with the global $user, at least I hope that is the case -- I think all of those have been weeded out). Instead, 'user' that you changed refers to 'a user object', as in, 'an object representing a user that I've just loaded from the database'.

9) We have had, over the past couple of years since I've been working on them, hundreds of API doc issues that have been in the 'documentation' component. They are generally never moved into the component of user.module, base system, etc. (i.e. the component corresponding to the particular function that is being documented), and I have never heard the maintainers of those components complain about this practice in the slightest. The only reason I would move something from documentation into, for instance, user.module, is if there is some question about what a function is intended to do that needs to be answered before we can document it properly, or if there is a question about whether something is a code bug or a documentation bug, and I need the maintainer to weigh in.

And, if I can make a slightly off-topic request... If you have the time and energy for work on documentation, I would greatly appreciate it if rather than spending your time and energy on mega-patches that try to bring a whole file's worth of functions up to our current documentation standards, you could instead look at the documentation component issue queue and work on issues there. The issues there are mostly places where someone has noticed and reported that the documentation is of a function is incorrect, misleading, incomplete, or unclear. Bringing doc up to standards is a good thing, but it's a lot more important (in my opinion) to have all of the functions documented correctly and clearly. Plus, as most of the doc issues deal with one or two functions, it's easier to get the patches reviewed and committed, and they don't conflict as often with other issues. And, I always try to get the particular functions up to standards at the same time. Just a thought...

TR’s picture

Status: Needs work » Needs review
FileSize
37.28 KB

@jhodgdon:
OK, I disagree with your disagreements, but in the spirit of moving this issue forward I've removed all the changes of "user" to "$user". Note that this leaves user.module with some Doxygen comments referring to "user" and some referring to "$user", but that's how it was before this patch. Specifically, for the example you cited in #8:

  * @return
- *   A fully-loaded user object if the user is found or FALSE if not found.
+ *   A fully-loaded $user object if the user is found or FALSE if not found.

After removing my changes there are still four functions that say "A fully-loaded $user object" and two functions that say "A fully-loaded user object". I'd like to deal with that in a separate issue, where we can discuss it without holding up the relatively trivial changes in this patch.

I also added the "string" type back into @param string $authname as you requested and found one other function with the same @param where I also added the "string" type.

I think that addresses all the issues with the patch that you have raised so far.

To summarize, this is what the patch changes:
1) Verb tenses. http://drupal.org/node/1354#files
2) Line wrapping. http://drupal.org/node/1354#functions
3) List format in @param and @return tags. http://drupal.org/node/1354#lists
4) Missing @param and @return tags were added to some functions.
5) Five (!) functions were missing Doxygen comment blocks.
6) Whitespace corrections.
7) Order of information in Doxygen comments. @param and @return were put after explanation of function.

Only comments are changed - no code is modified in any way.

Note that my intention in creating this patch was not to solve all the problems in the world with a "mega" patch, but to push forward a resolution to the documentation problem that was raised in this issue more than FOUR years ago. I view a lot of this patch as copy editing, and one of the functions of a copy editor is to ensure consistency of usage throughout the work. I don't see how that can be done properly on a piecemeal, function-by-function basis.

jhodgdon’s picture

Status: Needs review » Needs work

OK, I read through about half of the patch this time. There are still quite a lot of issues before I would advise committing this patch...

a) Another change you made in this patch: Took out a bunch of commas that I thought should have been left in. Such as this one:

- *   A fully-loaded user object upon successful user load, or FALSE if the user
+ *   A fully-loaded user object upon successful user load or FALSE if the user
  *   cannot be loaded.

I actually think it helps to have the comma, and isn't it correct to normally put commas before conjunctions joining large clauses? There were other places too...

b) Also, not all the tenses are fixed:

- * Save changes to a user account or add a new user.
+ * Saves changes to a user account or add a new user.

Needs add -> adds

Also here:

  * Menu access callback; limit access to account cancellation pages.

c)

+ *   A fully-loaded $user object upon successful save or FALSE if the save
+ *   failed.

This shouldn't be $user. It's not the global $user. There are still some other places too. This one is especially confusing:

 /**
- * Load either a specified or the current user account.
+ * Loads either a specified or the current user account.
  *
  * @param $uid
  *   An optional user ID of the user to load. If not provided, the current
  *   user's ID will be used.
+ *
  * @return
- *   A fully-loaded $user object upon successful user load, FALSE if user
+ *   A fully-loaded $user object upon successful user load or FALSE if user
  *   cannot be loaded.
  *
  * @see user_load()

In this case, it is the global $user if $uid is omitted, and it is not the global $user if $uid is provided and is different from the current user's ID. That's not made clear, and to me, putting $user in there makes it less clear than just saying "user object".

d) If you want to go for consistency in style:

+ * @return
+ *   Returns a string containing a message if name is invalid, NULL otherwise.

Normally we don't start the @return text with "Returns a".

e) The first sentence here needs to start with "a" or "the", probably "the":

+ * Random password will be composed of lower and upper case ASCII letters and
+ * numbers. Note that the number 0 and the letter 'O' have been removed to
+ * avoid confusion between the two. The same is true of 'I', 1, and 'l'.

f) http://drupal.org/node/1354#forms

 /**
- * Form validation handler for the current password on the user_account_form().
+ * Form validation for the current password on the user_account_form().
  *
  * @see user_account_form()
  */

Should say Form validation handler. Please don't remove the word "handler", as it is part of the standards.

g)

+ * Helper function to build login form used by user block.

Needs a "the" or two in there ... "build the login form used by the user block"

Same for:

+ * Menu access callback for user logout page.

and other similar places.

h) If you're bringing things up to standards, the theme_user_list doc should be changed to match
http://drupal.org/node/1354#themeable
@return isn't necessary for this, and the one-line summary should say "Returns HTML ..."

TR’s picture

Thank you for the review. I will address all the issues and post a new patch. However, I don't think your comments in c) are valid criticisms of this patch because the problems you point out are not things that this patch has changed.

+ *   A fully-loaded $user object upon successful save or FALSE if the save
+ *   failed.

This shouldn't be $user. It's not the global $user.

I DID NOT CHANGE THIS. That's the way it is in core right now. The only thing changed about that line is the line wrapping. This is one of the inconsistent uses of $user that currently exists in user.module. As I said in #12, I would prefer to handle the user/$user usage in a separate issue. I don't want this patch to be a catch-all for every documentation problem in the user.module, which is why I took all the changes to user/$user out of the patch.

There are still some other places too. This one is especially confusing:

/**
- * Load either a specified or the current user account.
+ * Loads either a specified or the current user account.
  *
  * @param $uid
  *   An optional user ID of the user to load. If not provided, the current
  *   user's ID will be used.
+ *
  * @return
- *   A fully-loaded $user object upon successful user load, FALSE if user
+ *   A fully-loaded $user object upon successful user load or FALSE if user
  *   cannot be loaded.
  *
  * @see user_load()

In this case, it is the global $user if $uid is omitted, and it is not the global $user if $uid is provided and is different from the current user's ID. That's not made clear, and to me, putting $user in there makes it less clear than just saying "user object".

Again, I did not change this. That "especially confusing" usage is what currently exists in core. And again, I would like to deal with the user vs. $user thing in a separate issue rather than hold back this patch. This patch makes no changes to the current core's usage of "user" and "$user".

Note, the "$user" problem is not confined to just "$user". For example, "Can either be a full user object or a $uid." I'd rather not expand the scope of this patch to deal with this.

jhodgdon’s picture

I'm not blaming you for the $user things. I realize you didn't create the problems. But you are the one who wanted everything to be consistent. I'm fine with leaving the $user things as they are now, and let's file a different issue to fix them.

spelcheck’s picture

Status: Needs work » Needs review

#12: 154221-reroll.patch queued for re-testing.

jhodgdon’s picture

Status: Needs review » Needs work

patch still needs work, independent of whether the tests fail...

jhedstrom’s picture

Issue summary: View changes
Issue tags: +Needs reroll

This needs a serious reroll, or can possibly just be moved back to 7.x since many of the above have either been removed, or already fixed elsewhere.

TR’s picture

Assigned: TR » Unassigned
madhavvyas’s picture

Version: 8.0.x-dev » 7.x-dev
Status: Needs work » Needs review
Issue tags: -Needs backport to D7, -Needs reroll
FileSize
33.19 KB

Comment #12 patch backport to drupal7