Attaching minor patch to apply doxygen to function "user_admin" in user.admin.inc, page callback to admin/user/user path.

Took me a couple minutes to figure out what this function does when I was stepping through Drupal's form process and saw this function. You can see there's no doxygen on this function and therefore the API page has no explanation: http://api.drupal.org/user_admin

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jzacsh’s picture

Attached for 6.x

jzacsh’s picture

Attached are d7 and d8 patches as well.

jzacsh’s picture

Actually, here's the same patches for d6, d7, d8 with @param used, as an attempt to explain the strangeness going on in this function's $op.

skottler’s picture

Looks good to me! Cleanly applied to all three versions.

Documentation patches rock!

mlncn’s picture

Status: Needs review » Reviewed & tested by the community
robbiethegeek’s picture

Looks good to me, applied cleanly to each version of Drupal.

jzacsh’s picture

Component: user.module » documentation

Fixing "component", per @jhodgdon

jhodgdon’s picture

Version: 6.x-dev » 8.x-dev
Component: documentation » user.module
Status: Reviewed & tested by the community » Needs work

Thanks for the patch!

This doesn't follow the documentation guidelines however, and in English we don't put a capital letter after a semicolon (;). So it needs to be fixed.

Please read
http://drupal.org/node/1354 for doc standards, and attach a patch for Drupal 8 only first, then backport to d7/d6. Thanks!

jhodgdon’s picture

Component: user.module » documentation
Issue tags: +Needs backport to D6, +Needs backport to D7

cross-post and adding tags

jhodgdon’s picture

Also, to the reviewers above - please do not mark patches RTBC unless you have verified that they follow the doc standards, and the test bot has come back green. Thanks!

jzacsh’s picture

Version: 8.x-dev » 6.x-dev
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs backport to D6, -Needs backport to D7
FileSize
895 bytes
895 bytes
895 bytes

Correcting name schema for patch files so test bot doesn't ignore them.

jhodgdon’s picture

Version: 6.x-dev » 8.x-dev
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs backport to D6, +Needs backport to D7

tagging and fixing cross-post again

jzacsh’s picture

Fixed doxygen style per #8 - http://drupal.org/node/1354 is very useful documentation!

jzacsh’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Status: Needs review » Needs work

Please read: http://drupal.org/node/1354#function
Function doc first line should start with a verb ending in s (third person).
Param should start with capital letter (after the optional).

Also, maybe it would be helpful to say what the flags are in the param? Normally flag means a constant... these are not constants are they?

jzacsh’s picture

FileSize
12.89 KB

Do we really want the "verb first"-rule applied in this case? Everywhere else in the codebase we have, "Menu callback; [verb]s" written. If everyone agrees that even menu callbacks should conform to this then that's a different story and maybe an issue should just be opened to change all of them in core in one shot.

As for this patch, I personally really like the current (possibly wrong) convention.
You can see a ton of our code still does it: $ grep -rin ' \* Menu callback' modules

@jhdogdon, thoughts?

jhodgdon’s picture

In your current patch, you have:

Menu callback; generate the appropriate user admin form.

It should at least be:

Menu callback: generates the appropriate user admininstration form.

We had an issue a while back to make standards for documenting menu callbacks, and it was never resolved. Guess we should find it and finish the discussion and get it resolved... Just because everything else in core is also bad is not a good reason to make a bad patch that conforms with the rest of the bad stuff.

jhodgdon’s picture

Title: patch to apply doxygen to function "user_admin" in user.admin.inc, page callback to admin/user/user path » user_admin function has no doc

more appropriate title

jzacsh’s picture

@jhodgdon Thanks for the tip. I'd be happy to tackle that issue you mentioned, if we find it.

Attached is the fix per comment #17.

jzacsh’s picture

Status: Needs work » Needs review

Whoops, setting to needs review.

jhodgdon’s picture

Status: Needs review » Needs work

The first line is OK I guess, since we don't have standards on this yet.

I just noticed something else though in the @param:

+ *   (optional) flag to indicate which form to build. Defaults to '', which
+ *   will trigger the user filter form.

a) Flag should be capitalized.

b) Is it really a flag? To me, "flag" indicates usually a PHP constant, but in this case it is a string?

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
597 bytes

Concerns in #2 addressed. Updated for D8.

jhodgdon’s picture

Status: Needs review » Needs work

Wow, that's a wacky function! It looks like it can build a multiple user delete confirm form, a user create form, or the default user admin form that lists users (with filtering). It also looks like $_POST['op'] overrides the argument $callback_arg, and I wouldn't say that if $callback_arg is '' that the user filter form is always what is displayed... Really only '' and 'create' are ever passed to $callback_arg, but $_POST['op'] can override it.

So, I think the @param needs some attention and this function needs a @return that says that one of those forms is returned depending on $callback_arg and $_POST values.

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
952 bytes

How's this?

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, that works!

jhodgdon’s picture

Status: Reviewed & tested by the community » Fixed

Thanks again -- patch in #24 is committed to 8.x, 7.x, and 6.x (I checked and the 6.x function works the same way as 7/8, and remarkably, the patch applies fine to all three).

Albert Volkman’s picture

Its a magical patch.

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