Problem/Motivation

Part of meta-issue #1310084: [meta] API documentation cleanup sprint.

Proposed resolution

Correct the following in includes files beginning with D-G:

  • 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

Followups

@return formatting

  • errors.inc

Optional parameters not documented properly

  • file.inc

List formatting

  • file.inc

Other followups

  • Batch API functions probably do not belong in form.inc anymore--FAPI dependencies should be removed.
  • Add defgroup for stream wrapper registry?
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

Status: Active » Needs work
FileSize
55.1 KB

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.

xjm’s picture

Issue tags: -Needs backport to D7

Untagging.

Lars Toomre’s picture

There is an indentation issue here in the corrected documentation for form_error():

+ *   special logic other than adding child elements: for example, for the 
+ *  'fieldset' element type, one of the functions in this array is form
+ *  _process_fieldset(), which adds the attributes and JavaScript needed to
+ *   make the fieldset collapsible if the #collapsible property is set. The
xjm’s picture

This patch is still marked needs work, meaning it's not finished. Also, this and the other two through Z are actually going to be discarded and re-done, so don't waste time on 'em. :)

Lars Toomre’s picture

Just saw your comment now about how this and other two are going to be redone. I will hold off on further comments until then.

xjm’s picture

Status: Needs work » Postponed

Postponing for core directory change.

xjm’s picture

Status: Postponed » Active

Going to roll this in steps with interdiffs so that it's easier to review.

xjm’s picture

Status: Active » Needs work
FileSize
5.12 KB

First, just moving the @see to the end.

xjm’s picture

FileSize
6.54 KB
11.37 KB

Adding blank lines above @return.

xjm’s picture

Rewrapping lines over 80 chars + \n for non-summaries.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

FileSize
962 bytes
15.25 KB

Cleaning up @file blocks. Next comes the fun part.

xjm’s picture

Summary corrections through errors.inc.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

FileSize
12.04 KB
27.91 KB

Summary corrections through file.inc.

I also removed a floating docblock with nothing attached to it--at first it looked like it might be a defgroup, but it's actually in the middle of another defgroup. So I added that to the followup list.

aspilicious’s picture

Status: Needs work » Needs review
+++ b/core/includes/file.incundefined
@@ -1598,6 +1591,7 @@ function file_save_upload($source, $validators = array(), $destination = FALSE,
  * @see move_uploaded_file()
+ * @see http://drupal.org/node/515192
  * @ingroup php_wrappers
  */

After multiple @see there should be a newline. *I think*

+++ b/core/includes/file.incundefined
@@ -2228,10 +2227,11 @@ function drupal_unlink($uri, $context = NULL) {
  * @see DrupalStreamWrapperInterface::realpath()
  * @see http://php.net/manual/function.realpath.php
  * @ingroup php_wrappers

Same in a few more places

-8 days to next Drupal core point release.

xjm’s picture

Status: Needs review » Needs work

This patch isn't done yet.

xjm’s picture

@aspilicious and I checked and his comment in #14 is actually not accurate, at least in the current standard. See, for example: http://drupal.org/node/1354#forms So leaving those for now.

Still two more files to go. Hopefully the interdiffs help, although this one is turning out to be a lot smaller than I feared.

xjm’s picture

FileSize
15.41 KB
42.37 KB

Through form.inc. Not nearly as bad as I thought.

xjm’s picture

FileSize
5.79 KB
48.16 KB

I added a return value to one function here because it was already provided in the multi-line summary that I fixed.

xjm’s picture

Status: Needs work » Needs review
FileSize
642 bytes
48.79 KB

And, done.

Theoretically one can just read the interdiffs for a first pass at a review, or to do a partial review. Or, if preferred, one can just read the final patch, which actually didn't turn out to be so terribly big.

aspilicious’s picture

+++ b/core/includes/form.incundefined
@@ -2901,7 +2921,9 @@ function date_validate($element) {
+ * Renders a month name for display.
+ *
+ * Callback for drupal_map_assoc() within form_process_date().

Not sure about this one. But why don't we have to use: "Render callback: ..."

Thats everything :)

-9 days to next Drupal core point release.

jhodgdon’s picture

That particular doc looks right to me. It isn't a render callback. It's a callback for drupal_map_assoc() and it is documented according to our standards for callbacks.

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

Than this is rtbc.

aspilicious’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

FileSize
539 bytes
48.79 KB
catch’s picture

#23: d-g-23.patch queued for re-testing.

jhodgdon’s picture

Thanks for the interdiff! I don't see any other Path items that need removing so yes this is still RTBC.

Dries’s picture

Status: Reviewed & tested by the community » Patch (to be ported)

Committed to 8.x. Moving to 7.x.

xjm’s picture

+++ b/core/includes/file.incundefined
@@ -1950,7 +1946,7 @@ function file_transfer($uri, $headers) {
 /**
- * Menu handler for private file transfers.
+ * Page callback: Handles private file transfers.
  *
  * Call modules that implement hook_file_download() to find out if a file is
  * accessible and what headers it should be transferred with. If one or more
@@ -1960,6 +1956,7 @@ function file_transfer($uri, $headers) {

@@ -1960,6 +1956,7 @@ function file_transfer($uri, $headers) {
  * If the file does not exist drupal_not_found() will be returned.
  *
  * @see hook_file_download()
+ * @see system_menu()
  */

+++ b/core/includes/form.incundefined
@@ -2901,7 +2921,9 @@ function date_validate($element) {
 /**
- * Helper function for usage with drupal_map_assoc to display month names.
+ * Renders a month name for display.
+ *
+ * Callback for drupal_map_assoc() within form_process_date().
  */

Looks like these are the only changes that need to be removed from the backport.

xjm’s picture

I don't see this commit in D8 yet.

Albert Volkman’s picture

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

I see this in D8, do you @xjm? And, D7 backport.

jhodgdon’s picture

I spot checked a few things from the D8 patch and agree it seems to be there. Thanks for checking and for the d7 backport; needs a review.

xjm’s picture

Assigned: xjm » ksenzee
Status: Needs review » Reviewed & tested by the community

Backport looks keen.

xjm’s picture

Assigned: ksenzee » Unassigned

Whoops. Sorry @ksenzee.

jhodgdon’s picture

Status: Reviewed & tested by the community » Fixed

One thing needs fixing (errors.inc):

 /**
- * Map PHP error constants to watchdog severity levels.
+ * Maps PHP error constants to watchdog severity levels.
  * The error constants are documented at
  * http://php.net/manual/en/errorfunc.constants.php

Needs a blank line after the first-line description.

But the rest looked good, so I just patched, fixed this one line, and committed. Thanks!!

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.