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; in drupal_html_to_text(). No inline comment. Yeah.
  • Node-specific code in menu.inc. :(
Files: 
CommentFileSizeAuthor
#48 h-m_complete-1317626-48.patch3.57 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 39,441 pass(es).
[ View ]
#48 interdiff.txt379 bytesAlbert Volkman
#46 h-m_complete-1317626-46.patch3.51 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 39,407 pass(es).
[ View ]
#44 h-m_complete-1317626-44.patch5.09 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 42,095 pass(es).
[ View ]
#41 h-m_complete-1317626-41.patch5.15 KBAlbert Volkman
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/includes/install.core.inc.
[ View ]
#41 interdiff.txt2.21 KBAlbert Volkman
#39 h-m_complete-1317626-39.patch4.39 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 42,087 pass(es).
[ View ]
#37 h-m_complete-1317626-37.patch44.75 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 39,423 pass(es).
[ View ]
#33 h-m_complete-1317626-33.patch44.76 KBAlbert Volkman
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch h-m_complete-1317626-33.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#31 h-m_complete-1317626-31.patch47.16 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 40,361 pass(es).
[ View ]
#31 h-m-complete-do-not-test.patch46.68 KBAlbert Volkman
#31 interdiff.txt2.46 KBAlbert Volkman
#19 h-m-complete.patch47.9 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 35,358 pass(es).
[ View ]
#19 interdiff-module.txt4.81 KBxjm
#18 h-m-menu.patch43.08 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 35,360 pass(es).
[ View ]
#18 interdiff.txt18.59 KBxjm
#17 h-m-mail.patch24.77 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 35,363 pass(es).
[ View ]
#17 interdiff-mail.txt3.21 KBxjm
#16 h-m-lock.patch22.89 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 35,352 pass(es).
[ View ]
#16 interdiff-lock.txt1.34 KBxjm
#15 h-m-language.patch21.56 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 35,345 pass(es).
[ View ]
#15 interdiff-language.txt841 bytesxjm
#14 h-m-install-core.patch16.43 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 35,369 pass(es).
[ View ]
#14 interdiff-install-core.txt10.14 KBxjm
#13 h-m-image.patch6.29 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 35,363 pass(es).
[ View ]
#13 interdiff-image.txt2.56 KBxjm
#12 h-m-blank-lines.patch3.73 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 35,349 pass(es).
[ View ]
#12 interdiff-blank-lines.txt2.75 KBxjm
#11 h-m-see.patch1.29 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 35,368 pass(es).
[ View ]
#11 interdiff-see.txt885 bytesxjm
#10 h-m-file-blocks.patch431 bytesxjm
PASSED: [[SimpleTest]]: [MySQL] 35,361 pass(es).
[ View ]
#1 1317626-1-includes-h-m.patch32.47 KBxjm
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1317626-1-includes-h-m.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

Title:Clean up API docs for includes directory, files starting with I-MClean up API docs for includes directory, files starting with H-M
Status:Active» Needs work
StatusFileSize
new32.47 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1317626-1-includes-h-m.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Cleanup 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.

Issue tags:-needs backport to D7

Untagging.

In reviewing this patch, I note that there are indentation problems with some functions. For instance, in drupal_install_mkdir()

  * @param $message
  *  (optional) Whether to output messages. Defaults to TRUE.
+ *
  * @return
  *  TRUE/FALSE whether or not the directory was successfully created.

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(),

+ *   Optional. By default retrieves values from the 'language_types' variable
+ *   to avoid unnecessary hook invocations.
+ *   If set to FALSE retrieves values from the actual language type
+ *   definitions. This allows to react to alterations performed on the
+ *   definitions by modules installed after the 'language_types' variable is
+ *   set.

the sting 'Optional.' shorld be '(optional)'. I also would add a comma after FALSE.

Again... 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. :)

Status:Needs work» Postponed

Postponing for core directory change.

Issue summary:View changes

Updated issue summary.

Agreed - 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.

Status:Postponed» Active

Next!

Tagging. Also, xjm do you still plan to do this one?

Yep, just waiting on #1317620: Clean up API docs for includes directory, files starting with D-G and then I will do this one.

StatusFileSize
new431 bytes
PASSED: [[SimpleTest]]: [MySQL] 35,361 pass(es).
[ View ]

We're (nearly) under thresholds now so here goes.

The only missing @file docblock.

Status:Active» Needs work
StatusFileSize
new885 bytes
new1.29 KB
PASSED: [[SimpleTest]]: [MySQL] 35,368 pass(es).
[ View ]

The only misplaced @see.

Issue summary:View changes

Updated issue summary.

StatusFileSize
new2.75 KB
new3.73 KB
PASSED: [[SimpleTest]]: [MySQL] 35,349 pass(es).
[ View ]

Blank 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.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

StatusFileSize
new2.56 KB
new6.29 KB
PASSED: [[SimpleTest]]: [MySQL] 35,363 pass(es).
[ View ]

Function summaries and rewrapping in image.inc.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

StatusFileSize
new10.14 KB
new16.43 KB
PASSED: [[SimpleTest]]: [MySQL] 35,369 pass(es).
[ View ]

Lots of form constructor fun and followups needed for install.core.inc.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

StatusFileSize
new841 bytes
new21.56 KB
PASSED: [[SimpleTest]]: [MySQL] 35,345 pass(es).
[ View ]

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

StatusFileSize
new1.34 KB
new22.89 KB
PASSED: [[SimpleTest]]: [MySQL] 35,352 pass(es).
[ View ]

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

.

Issue summary:View changes

Updated issue summary.

StatusFileSize
new3.21 KB
new24.77 KB
PASSED: [[SimpleTest]]: [MySQL] 35,363 pass(es).
[ View ]

And this is enough for today. menu.inc and module.inc remaining.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

StatusFileSize
new18.59 KB
new43.08 KB
PASSED: [[SimpleTest]]: [MySQL] 35,360 pass(es).
[ View ]

Not so bad.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Status:Needs work» Needs review
StatusFileSize
new4.81 KB
new47.9 KB
PASSED: [[SimpleTest]]: [MySQL] 35,358 pass(es).
[ View ]

Smaller than the last two, for sure.

Issue summary:View changes

.

To help review this patch:

  1. Read the scope of the issue as defined in the issue summary above.
  2. Pick an interdiff above.
  3. Review only the changed lines (beginning with a + or -, not the additional lines that are provided for context.
  4. Ensure that the text in the added lines is clear, grammatically correct English, with capitalization, periods, etc. Keep an eye out for typos.
  5. Ensure that the updated lines meet the specific standards applicable in each case:
  6. Apply the whole patch from #19 to your local repository. Check the relevant file(s) for your interdiff and ensure that there are no other cleanups needed in that category. (For #10 - #12, check in all includes with names beginning with D-G. For #13 - #19, also ensure that no lines in any docblock are over 80 characters unless they are within @code/@endcode or @link/@endlink.)
  7. Document what you reviewed and what you found specifically in your review comment.

Just 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?

interdiff-image.txt from #13 looks good to except minor issues mentioned in #21

regarding 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 :)

+ * 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."

@kbasarab reviewed interdiff-module.txt.

+ * 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"

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.

#10 Looks good. Not much in that one.
#11 Looks good.

#12:

@@ -815,6 +827,7 @@ function drupal_requirements_severity(&$requirements) {
  *
  * @param $module
  *   Machine name of module to check.
+ *
  * @return
  *   TRUE/FALSE depending on the requirements are in place.
  */

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.

Actually, I believe it should read:

TRUE or FALSE, depending on whether the requirements are met.

Thanks @kbasarab!

#1464944: Installation of configuration system fails in some cases added another couple incorrect docblocks here.

Status:Needs review» Needs work

It 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?

Assigned:xjm» Unassigned

I'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!

Status:Needs work» Needs review
StatusFileSize
new2.46 KB
new46.68 KB
new47.16 KB
PASSED: [[SimpleTest]]: [MySQL] 40,361 pass(es).
[ View ]

Re-worked @xjm's patch from #19 to apply cleanly, then implemented the above suggestions. New patch, revised #21 patch, and interdiff attached.

Version:8.x-dev» 7.x-dev
Status:Needs review» Patch (to be ported)

This looks great! Committed to 8.x.

Status:Patch (to be ported)» Needs review
StatusFileSize
new44.76 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch h-m_complete-1317626-33.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

First pass at a D7 backport.

Status:Needs review» Needs work
Issue tags:+Novice, +needs backport to D7, +docs-cleanup-2011

The last submitted patch, h-m_complete-1317626-33.patch, failed testing.

Issue tags:+Needs reroll

Looks 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!

Status:Needs work» Needs review
StatusFileSize
new44.75 KB
PASSED: [[SimpleTest]]: [MySQL] 39,423 pass(es).
[ View ]

No worries, re-rolled!

Version:7.x-dev» 8.x-dev
Status:Needs review» Active

Everything 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

Status:Active» Needs review
StatusFileSize
new4.39 KB
PASSED: [[SimpleTest]]: [MySQL] 42,087 pass(es).
[ View ]

Resolves issues found by @jhodgdon.

Status:Needs review» Needs work

Thanks! That's mostly great... A couple of corrections/additions:

a) (install.core.inc)

+ *
+ * @param $files
+ *   An array of .po files.
+ *
+ * @see install_find_translations()
+ * @ingroup forms
  */
function install_select_language_form($form, &$form_state, $files) {

Looking at how $files is used in the function, I see:
  foreach ($files as $file) {
    if (isset($standard_languages[$file->langcode])) {
...

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?

Status:Needs work» Needs review
StatusFileSize
new2.21 KB
new5.15 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/includes/install.core.inc.
[ View ]

This is my attempt to resolve these issues. I _think_ I understood what you were saying in (a), but wasn't entirely sure.

Status:Needs review» Needs work

The last submitted patch, h-m_complete-1317626-41.patch, failed testing.

Here's the syntax error in the patch:

+ */
+function install_find_translation_files($langcode = NULL) {
  */
function install_select_language_form($form, &$form_state, $files) {

I think the new wording is good. :)

Status:Needs work» Needs review
StatusFileSize
new5.09 KB
PASSED: [[SimpleTest]]: [MySQL] 42,095 pass(es).
[ View ]

Ah, got in a hurry. I was trying to get this patch together before my work day started :)

Version:8.x-dev» 7.x-dev
Status:Needs review» Patch (to be ported)

Great, thanks! Committed to 8.x.

I think we can just port this to D7 and then call this issue fixed.

Status:Patch (to be ported)» Needs review
StatusFileSize
new3.51 KB
PASSED: [[SimpleTest]]: [MySQL] 39,407 pass(es).
[ View ]

Backported.

Status:Needs review» Needs work

It looks like in D7 we also need to fix this first line to conform with our usual Form standards:

/**
  * Form API array definition for language selection.
+ *
+ * @ingroup forms
  */
function install_select_locale_form($form, &$form_state, $locales, $profilename) {

Other than that, the patch looks good to me, thanks!

Status:Needs work» Needs review
StatusFileSize
new379 bytes
new3.57 KB
PASSED: [[SimpleTest]]: [MySQL] 39,441 pass(es).
[ View ]

Done!

Assigned:Unassigned» jhodgdon
Status:Needs review» Reviewed & tested by the community

Thanks! I'll get this committed (probably tomorrow).

Status:Reviewed & tested by the community» Fixed

Whew! committed this one too.

Assigned:jhodgdon» Unassigned

Glad 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.

It 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.

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

Issue summary:View changes

.