Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Part of meta-issue #1310084: [meta] API documentation cleanup sprint
Comment | File | Size | Author |
---|---|---|---|
#27 | d7-block.patch | 12.07 KB | xjm |
#27 | interdiff.txt | 9.75 KB | xjm |
#23 | block-clean-up-documentation-1323720-23.patch | 954 bytes | xenophyle |
#18 | block-clean-up-documentation-1323720-5.patch | 15.23 KB | xenophyle |
#15 | block-clean-up-documentation-1323720-4.patch | 15.78 KB | xenophyle |
Comments
Comment #1
xenophyle CreditAttribution: xenophyle commentedHere's my first pass at a patch.
Comment #2
jhodgdonLooks pretty good, thanks!
A few things to fix:
a)
See http://drupal.org/node/1354#forms for standards on how to document form submission headers. Also, farther down in the patch, this form generating function should use the standards:
b)
- Each parameter gets a separate @param
- Description ends in .
- We also have standards for how to document sorting callbacks like this -- see http://drupal.org/node/1354#functions (scroll down to "Callbacks" section). In particular, you probably don't need the @param/@return at all on this function; instead, include the "Callback for uasort in function_name_here()." line.
c) Right above that:
This could be wrapped better - should go out to as close to 80 characters as possible without going over.
d)
This should say "Access callback: only...". See http://drupal.org/node/1354#menu-callback -- also the next section in the patch should be "Theme callback: ...".
e)
There is an extra space at the end of the added line. You can probably set your editor to delete trailing whitespace, or at least to show trailing whitespace. I didn't necessarily look at the whole patch carefully for extra whitespace yet, so there may be other places.
f)
Whenever you use a function name, put () after it (this signals the API module to turn it into a link on api.drupal.org).
g)
This needs to be split into two paragraphs. The first line of any docblock should be one 80-character-or-less sentence.
Comment #3
xenophyle CreditAttribution: xenophyle commentedWow, thanks for the quick response! I'll get to work.
Comment #4
xenophyle CreditAttribution: xenophyle commentedTrying again...
Comment #5
jhodgdonThis is looking much better! A few minor things to fix:
a)
Path needs / in it: admin/structure/block/demo/%
b)
Having a . here seems a bit confusing to me and isn't in our standards for menu callbacks.
c)
- uasort should be uasort() to turn into a function link on api.drupal.org
- Substitute the actual function where this is used for function_name_here(). I guess my comment above was taken a bit too literally. Check out the standards at http://drupal.org/node/1354#functions (scroll down to the section called "Callback functions" within that section).
d)
spelling: paramter -> parameter
Comment #6
xenophyle CreditAttribution: xenophyle commentedOk, I truly do not know how that ended up in here; I have a definite memory of editing that line to actually make sense. :)
Comment #7
xenophyle CreditAttribution: xenophyle commentedComment #8
jhodgdon+ * Callback for usort in block_admin_display_prepare_blocks().
usort() still needs parens to turn into a function link. Fix that and I think it's good to go!
Comment #9
xenophyle CreditAttribution: xenophyle commentedHere's the patch with that change.
Thanks for all your help!
Comment #10
xenophyle CreditAttribution: xenophyle commentedComment #11
jhodgdonLooks good, thanks!
Comment #12
xenophyle CreditAttribution: xenophyle commentedI just noticed that there is an @ingroup hooks line in the sample hook definition. Is that something that should go in all modules?
Comment #13
jhodgdonYes, all hooks are supposed to have that, so that they appear on the Hooks topic page on api.drupal.org. Or else they should be within an @ingroup hooks @{ @} section. It wasn't specifically part of the cleanup... do you see some hooks in this module that are missing from the Hooks topic list?
Comment #14
xenophyle CreditAttribution: xenophyle commentedAh, I didn't realize the @ingroup hooks @{ @} section did the same thing. I guess that's all set, then.
Comment #15
xenophyle CreditAttribution: xenophyle commentedI just changed the way form constructors that are also used in page callbacks are documented as per #1325116: Clean up API docs for Aggregator module.
Comment #16
xjmHey @xenophyle. Looks like this patch is in pretty good shape! Here are a few minor things I noticed:
Since these comments aren't in the doxygen blocks, it might be better to correct them in a separate issue. Maybe it's okay, but adding other cleanups makes it more likely that this patch will collide with other patches. Reference: http://webchick.net/please-stop-eating-baby-kittens
Two suggestions here: 1. While we are adding this parameter documentation, do we want to make it use the normal pattern for optional parameters? 2. It seems like a bit of a run-on sentence.
How about this?
Edit: Or, we could split it into two sentences. What do you think?
I think this should have a comma after "user role" for clarity, and a hyphen in "user-specific." Minor nitpick, though. :)
Missing a period at the end.
Trailing whitespace left over from rewrapping here.
Totally a minor point/nitpick, but I'd use single quotes here, since that's more usual for denoting a literal value.
Comment #17
xjmNote that this patch will need to be rerolled, because the core directory structure for Drupal 8 has now changed. (For more information, see #22336: Move all core Drupal files under a /core folder to improve usability and upgrades). If you need help rerolling this patch, you can come to core office hours or ask in #drupal-gitsupport on IRC.
Comment #18
xenophyle CreditAttribution: xenophyle commentedRerolled patch following xjm's suggestions. Thanks for the 8.x restructuring alert.
Comment #19
xjmI've read through #18 and I think this is RTBC if it passes testbot. Thanks @xenophyle!
Comment #20
xjmComment #21
catchCommitted/pushed to 8.x, thanks!
Comment #23
xenophyle CreditAttribution: xenophyle commentedUnclassify form constructors as page callback as per #1337282: Lacking doc header standard for forms used as page callbacks
Comment #24
jhodgdonThanks!
Comment #25
catchYep. Committed/pushed to 8.x, thanks!
Comment #27
xjmWith the menu callback stuff removed. This one had just one hunk that didn't apply, but I couldn't find the function in D7.
Comment #28
jhodgdonLooks fine for d7, thanks!
Comment #29
webchickCommitted and pushed to 7.x. Thanks!