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.
Problem/Motivation
Part of meta-issue #1310084: [meta] API documentation cleanup sprint.
Proposed resolution
Correct the following in includes
files beginning with H-M:
- Ensure that each file has an
@file
docblock. - Ensure that all
@return
lines are preceded by a blank line. - Ensure that
@see
and@ingroup
lines 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
@param and @return formatting
- install.inc (some indentation is incorrect).
- menu.inc (missing punctuation; @return that are actually updates to variables passed by reference).
Optional parameter documentation
- image.inc
- install.core.inc
- install.inc
- lock.inc
- mail.inc
- menu.inc
- module.inc
List formatting
- image.inc
Missing @param
- install.core.inc
- mail.inc
- menu.inc
- module.inc
Missing @return
- install.core.inc
- install.inc
- lock.inc
- mail.inc
- menu.inc
- module.inc
Miscellaneous
- install_configure_form() does a lot more than construct a FAPI array.
$tag = !$tag;
indrupal_html_to_text()
. No inline comment. Yeah.- Node-specific code in
menu.inc
. :(
Comment | File | Size | Author |
---|---|---|---|
#48 | h-m_complete-1317626-48.patch | 3.57 KB | Albert Volkman |
#48 | interdiff.txt | 379 bytes | Albert Volkman |
#46 | h-m_complete-1317626-46.patch | 3.51 KB | Albert Volkman |
#44 | h-m_complete-1317626-44.patch | 5.09 KB | Albert Volkman |
#41 | h-m_complete-1317626-41.patch | 5.15 KB | Albert Volkman |
Comments
Comment #1
xjmCleanup for character limit, lines above
@return
, and@see
/@ingroup
positioning. No fixes to summaries or file docblocks yet. Waiting on #1315992: No standard for documenting menu callbacks and perhaps #1317826: Provide general documentation standard for callbacks.Comment #2
xjmUntagging.
Comment #3
Lars Toomre CreditAttribution: Lars Toomre commentedIn reviewing this patch, I note that there are indentation problems with some functions. For instance, in drupal_install_mkdir()
each of the explanations needs to be indented one additional space. Same goes for st() function.
I am aware that type hinting of parameters and return values is not required (but recommended). However, I think the documentation for both of these functions would be more clear if type hinting was included (e.g. '@param bool $message' and '@return bool').
In language_negotiation_get(),
the sting 'Optional.' shorld be '(optional)'. I also would add a comma after FALSE.
Comment #4
xjmAgain... the patch is still marked "Needs Work..." and we are not fixing everything about the docblocks, just the targeted things. And the previous patch is getting thrown out. :)
Comment #5
xjmPostponing for core directory change.
Comment #5.0
xjmUpdated issue summary.
Comment #6
jhodgdonAgreed - we're not generally putting types into these patches -- it makes the patch *much* harder to review, since you need to go read the function and verify the types are correct.
Comment #7
xjmNext!
Comment #8
jhodgdonTagging. Also, xjm do you still plan to do this one?
Comment #9
xjmYep, just waiting on #1317620: Clean up API docs for includes directory, files starting with D-G and then I will do this one.
Comment #10
xjmWe're (nearly) under thresholds now so here goes.
The only missing
@file
docblock.Comment #11
xjmThe only misplaced
@see
.Comment #11.0
xjmUpdated issue summary.
Comment #12
xjmBlank lines above
@return
.Note that a lot of
install.inc
has bad indentation for@param/@return
(one space rather than two). Added that to the summary as a followup.Comment #12.0
xjmUpdated issue summary.
Comment #12.1
xjmUpdated issue summary.
Comment #13
xjmFunction summaries and rewrapping in
image.inc
.Comment #13.0
xjmUpdated issue summary.
Comment #13.1
xjmUpdated issue summary.
Comment #13.2
xjmUpdated issue summary.
Comment #13.3
xjmUpdated issue summary.
Comment #13.4
xjmUpdated issue summary.
Comment #14
xjmLots of form constructor fun and followups needed for
install.core.inc
.Comment #14.0
xjmUpdated issue summary.
Comment #14.1
xjmUpdated issue summary.
Comment #15
xjmComment #15.0
xjmUpdated issue summary.
Comment #15.1
xjmUpdated issue summary.
Comment #16
xjmComment #16.0
xjmUpdated issue summary.
Comment #16.1
xjmUpdated issue summary.
Comment #16.2
xjm.
Comment #16.3
xjmUpdated issue summary.
Comment #17
xjmAnd this is enough for today.
menu.inc
andmodule.inc
remaining.Comment #17.0
xjmUpdated issue summary.
Comment #17.1
xjmUpdated issue summary.
Comment #17.2
xjmUpdated issue summary.
Comment #17.3
xjmUpdated issue summary.
Comment #18
xjmNot so bad.
Comment #18.0
xjmUpdated issue summary.
Comment #18.1
xjmUpdated issue summary.
Comment #19
xjmSmaller than the last two, for sure.
Comment #19.0
xjm.
Comment #20
xjmTo help review this patch:
+
or-
, not the additional lines that are provided for context.@param
and@return
sections, and also between those and surrounding sections in the docblock. See: http://drupal.org/node/1354#functions.@code/@endcode
or@link/@endlink
.)Comment #21
sphism CreditAttribution: sphism commentedJust had a look through #19 and here are some notes:
image.inc
In the function headings sometimes use 'Returns...', sometimes use 'Gets...' and sometimes use 'Retrieves...'
Since all 3 of those functions have _get in them i suggest we use 'Gets...'
2 new lines after:
function image_desaturate(stdClass $image) {
return image_toolkit_invoke('desaturate', $image);
}
Crops an image to the rectangle specified by the given rectangle.
-- sounds odd, perhaps:
Crops an image to a rectangle specified by the given dimensions.
no comments > 80 chars
can't see any other issues with doc blocks, except all missing param and return types, but that's for another sprint right?
Comment #22
sphism CreditAttribution: sphism commentedinterdiff-image.txt from #13 looks good to except minor issues mentioned in #21
Comment #23
sphism CreditAttribution: sphism commentedregarding interdiff-module.txt:
Determines which modules require and are required by each module.
--- sounds odd, is an improvement on "Find dependencies any level deep and fill in required by information too." but still not ideal
Other than that the interdiff looks good.
-----
Checking the applied patch:
No comments are over 80 chars
2 line breaks after function module_load_all
This seems to be written differently to all the others:
/**
* Array of modules required by core.
*/
function drupal_required_modules() {
--- Wouldn't it be "Returns an array of modules required by core." or something?
Yep all looks good, that's some nicely formatted and commented code right there :)
Comment #24
kbasarab CreditAttribution: kbasarab commented+ * Determines which modules require and are required by each module.
Should read differently. Doesn't quite sound right. Maybe:
"Determines which modules are required by each module"
In the full patch:
+ * Returns the router path, or the path for of a default local task's parent.
Should possibly read:
"* Returns the router path, or the path for a default local task's parent."
Comment #25
xjm@kbasarab reviewed
interdiff-module.txt
.I disagree with this particular point. The sentence is grammatically correct, and the revised suggestion does not cover the full functionality. The other change looks correct, though. Thanks @kbasarab and @sphism!
Remaining interdiffs to review: #10, #11, #12, #14, #15, #16, #17, and #18.
Comment #26
kbasarab CreditAttribution: kbasarab commented#10 Looks good. Not much in that one.
#11 Looks good.
#12:
Should TRUE/FALSE statement read:
"TRUE/FALSE depending on if the requirements are in place."
#14, #15, #16, #17 and #18 look good to me. Another set of eyes probably wouldn't be bad though.
Comment #27
xjmActually, I believe it should read:
Thanks @kbasarab!
Comment #28
xjm#1464944: Installation of configuration system fails in some cases added another couple incorrect docblocks here.
Comment #29
jhodgdonIt looks like a lot of this patch has been reviewed, and I think there were some review comments that need to be addressed. Can we get those fixed, and then I would be happy (or at least willing) to give this another review?
Comment #30
jhodgdonI'm going through these issues and un-assigning any without activity for several weeks. If you still want to work on this issue, please feel free to assign it back to yourself. Thanks!
Comment #31
Albert Volkman CreditAttribution: Albert Volkman commentedRe-worked @xjm's patch from #19 to apply cleanly, then implemented the above suggestions. New patch, revised #21 patch, and interdiff attached.
Comment #32
jhodgdonThis looks great! Committed to 8.x.
Comment #33
Albert Volkman CreditAttribution: Albert Volkman commentedFirst pass at a D7 backport.
Comment #34
jhodgdon#33: h-m_complete-1317626-33.patch queued for re-testing.
Comment #36
jhodgdonLooks like this one needs a reroll... and I'm *trying* to get through these patches to get them reviewed/committed... sorry for delays, I had a busy summer!
Comment #37
Albert Volkman CreditAttribution: Albert Volkman commentedNo worries, re-rolled!
Comment #38
jhodgdonEverything in this patch looks like an improvement, thanks! Committed to 7.x.
So... I took a look at the H-M include files, and I think we could go back to 8.x (and 7.x port) and fix a few more issues:
- menu.inc - _menu_check_access() - first line doesn't end in "."
- menu.inc - template_preprocess_menu_tree() - first line isn't documented the way we normally do these now: Implements template_preprocess_HOOK() for theme_menu_tree()
- menu.inc - theme_menu_local_tasks() doesn't have $variable documentation
- mail.inc - drupal_wrap_mail() has no @return
- install.inc - install_verify_config_directory() has a two-line start instead of one-line, and a wrapping issue in the next paragraph
- install.core.inc - install_select_language_form() needs @param $files docs
- None of the form constructors in install.core.inc has @ingroup forms in docs
- install.core.inc - _install_configure_form() should be documented as a form constructor
Comment #39
Albert Volkman CreditAttribution: Albert Volkman commentedResolves issues found by @jhodgdon.
Comment #40
jhodgdonThanks! That's mostly great... A couple of corrections/additions:
a) (install.core.inc)
Looking at how $files is used in the function, I see:
and it's called from install_select_languages(), which passes in the output of install_find_translations()...
b) Related: It looks like we need a @return for install_find_translations() that explains what the return value is... which can be taken from install_find_translation_files(). (install.core.inc)
c) _install_configure_form() still needs @param $install_state (install.core.inc)
d) Looking at the code in theme_menu_local_tasks() in menu.inc, it looks like both of the variable elements are optional, so maybe that should be mentioned in the documentation?
Comment #41
Albert Volkman CreditAttribution: Albert Volkman commentedThis is my attempt to resolve these issues. I _think_ I understood what you were saying in (a), but wasn't entirely sure.
Comment #43
jhodgdonHere's the syntax error in the patch:
I think the new wording is good. :)
Comment #44
Albert Volkman CreditAttribution: Albert Volkman commentedAh, got in a hurry. I was trying to get this patch together before my work day started :)
Comment #45
jhodgdonGreat, thanks! Committed to 8.x.
I think we can just port this to D7 and then call this issue fixed.
Comment #46
Albert Volkman CreditAttribution: Albert Volkman commentedBackported.
Comment #47
jhodgdonIt looks like in D7 we also need to fix this first line to conform with our usual Form standards:
Other than that, the patch looks good to me, thanks!
Comment #48
Albert Volkman CreditAttribution: Albert Volkman commentedDone!
Comment #49
jhodgdonThanks! I'll get this committed (probably tomorrow).
Comment #50
jhodgdonWhew! committed this one too.
Comment #51
jhodgdonComment #52
Lars Toomre CreditAttribution: Lars Toomre commentedGlad to see this one committed. I realise that @jhodgon has said in the past that there is no need for an empty line before a @ingroup directive. However, for the sake of of consistency, I have been adding blank lines between @return, @throws, @see, @ingroup and @todo.
I am unclear why @see and @ingroup directives can/should be grouped together whereas the others are supposed to be separated by a blank line. Can you elaborate @jhodgdon so I do not create more patches that violate what was committed here? Thanks in advance.
Comment #53
jhodgdonIt is never a mistake to add the blank lines, but our standards do not insist upon it.
For instance if there is one @see and one @ingroup, it doesn't really add a lot of readability to have a blank line in there. If there are 5 @see lines and 3 @ingroup lines, then it's probably a good idea. There should always be a blank line after @return if there are @see or @ingroup though... Anyway, I guess we don't have a hard-and-fast rule for this one -- use your own judgment. I won't reject patches that have the blank lines, or that don't.
Comment #54.0
(not verified) CreditAttribution: commented.
Comment #55
Andrew Answer CreditAttribution: Andrew Answer as a volunteer commented