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
Comment | File | Size | Author |
---|---|---|---|
#24 | user_admin_docs-1247812-24.patch | 952 bytes | Albert Volkman |
#22 | user_admin_docs-1247812-22.patch | 597 bytes | Albert Volkman |
#19 | 1247812-user_admin-d8-doxygen.patch | 931 bytes | jzacsh |
#16 | found_Menu_callbackd8.txt | 12.89 KB | jzacsh |
#13 | 1247812-user_admin-d8-doxygen.patch | 921 bytes | jzacsh |
Comments
Comment #1
jzacsh CreditAttribution: jzacsh commentedAttached for 6.x
Comment #2
jzacsh CreditAttribution: jzacsh commentedAttached are d7 and d8 patches as well.
Comment #3
jzacsh CreditAttribution: jzacsh commentedActually, 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.
Comment #4
skottler CreditAttribution: skottler commentedLooks good to me! Cleanly applied to all three versions.
Documentation patches rock!
Comment #5
mlncn CreditAttribution: mlncn commentedComment #6
robbiethegeek CreditAttribution: robbiethegeek commentedLooks good to me, applied cleanly to each version of Drupal.
Comment #7
jzacsh CreditAttribution: jzacsh commentedFixing "component", per @jhodgdon
Comment #8
jhodgdonThanks 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!
Comment #9
jhodgdoncross-post and adding tags
Comment #10
jhodgdonAlso, 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!
Comment #11
jzacsh CreditAttribution: jzacsh commentedCorrecting name schema for patch files so test bot doesn't ignore them.
Comment #12
jhodgdontagging and fixing cross-post again
Comment #13
jzacsh CreditAttribution: jzacsh commentedFixed doxygen style per #8 - http://drupal.org/node/1354 is very useful documentation!
Comment #14
jzacsh CreditAttribution: jzacsh commentedComment #15
jhodgdonPlease 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?
Comment #16
jzacsh CreditAttribution: jzacsh commentedDo 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?
Comment #17
jhodgdonIn 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.
Comment #18
jhodgdonmore appropriate title
Comment #19
jzacsh CreditAttribution: jzacsh commented@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.
Comment #20
jzacsh CreditAttribution: jzacsh commentedWhoops, setting to needs review.
Comment #21
jhodgdonThe first line is OK I guess, since we don't have standards on this yet.
I just noticed something else though in the @param:
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?
Comment #22
Albert Volkman CreditAttribution: Albert Volkman commentedConcerns in #2 addressed. Updated for D8.
Comment #23
jhodgdonWow, 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.
Comment #24
Albert Volkman CreditAttribution: Albert Volkman commentedHow's this?
Comment #25
jhodgdonThanks, that works!
Comment #26
jhodgdonThanks 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).
Comment #27
Albert Volkman CreditAttribution: Albert Volkman commentedIts a magical patch.