Problem/Motivation

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

Proposed resolution

Correct the following in includes files beginning with N-Z:

  • 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

Create new patch following the "defacto standard" for line wrapping once core files are moved to the core directory.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

Status: Active » Needs work
FileSize
37.68 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 summary: View changes

Updated issue summary.

jhodgdon’s picture

Issue tags: -Needs backport to D7

Standard is there... :)
http://drupal.org/node/1354#menu-callback

And we should probably not try to backport these issues to d7, as webchick does not appear to be receptive to this type of cleanup.

xjm’s picture

Lars: Don't spend time reviewing this!

xjm’s picture

Status: Needs work » Postponed

Postponing for core directory change.

xjm’s picture

Issue summary: View changes

Updated issue summary.

jhodgdon’s picture

Status: Postponed » Active

There has also been a start to the select.inc cleanup on another issue:
#1255524: select.inc needs more doxygen
which I'm closing as a duplicate of this one.

And this shouldn't be postponed any more?

jhodgdon’s picture

Sorry, that is wrong -- that select.inc is in the database issue, not this one, whoops!

jhodgdon’s picture

Issue tags: +Novice, +Needs backport to D7

tagging.

Also xjm do you plan to do this one or should we unassign?

xjm’s picture

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

xjm’s picture

Issue summary: View changes

Updated issue summary.

jhodgdon’s picture

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!

mjonesdinero’s picture

Assigned: Unassigned » mjonesdinero

will work on this issue and hopefully attached the patch Maximum -- June 25..thanks

mjonesdinero’s picture

Status: Active » Needs review
FileSize
44.04 KB

not so long, since have time to do it now
will wait for feedback if accepted

Status: Needs review » Needs work

The last submitted patch, n-z_clean_up_api-1317628-11.patch, failed testing.

mjonesdinero’s picture

Status: Needs work » Needs review
FileSize
44.04 KB

another attempt, it is okie with my test on my localhost, hope the testbot is okie

Status: Needs review » Needs work

The last submitted patch, n-z-clean-up-1317628-13.patch, failed testing.

mjonesdinero’s picture

Status: Needs work » Needs review

i dont know why the test bot is making my patch error will work with this again..

in my local it is working

mjonesdinero’s picture

if someone could tell me why this is happening thanks in advance

disasm’s picture

It's failing to apply because it's searching for some text that doesn't exist anymore in theme.inc. Did you merge with latest 8.x before generating the patch?

Checking patch core/includes/theme.inc...
error: while searching for:
* executed (if they exist), in the following order (note that in the following
* list, HOOK indicates the theme hook name, MODULE indicates a module name,
* THEME indicates a theme name, and ENGINE indicates a theme engine name):
* - template_preprocess(&$variables, $hook): Creates a default set of variables
* for all theme hooks.
* - template_preprocess_HOOK(&$variables): Should be implemented by
* the module that registers the theme hook, to set up default variables.
* - MODULE_preprocess(&$variables, $hook): hook_preprocess() is invoked on all
* implementing modules.
* - MODULE_preprocess_HOOK(&$variables): hook_preprocess_HOOK() is invoked on
* all implementing modules, so that modules that didn't define the theme hook
* can alter the variables.
* - ENGINE_engine_preprocess(&$variables, $hook): Allows the theme engine to
* set necessary variables for all theme hooks.
* - ENGINE_engine_preprocess_HOOK(&$variables): Allows the theme engine to set

error: patch failed: core/includes/theme.inc:760
error: core/includes/theme.inc: patch does not apply

I'm attempting a re-roll (went all the way back to April, and it's still failing to apply). If I get this to re-roll, I'll post.

disasm’s picture

reroll

mjonesdinero’s picture

hi disasm

Thanks for the re-roll, yup i have notice it last nyt, but since you have do it for me thanks..

but i am still assigning this to me, for further feedbacks from @jhodgdon

mjonesdinero’s picture

Assigned: mjonesdinero » Unassigned
jhodgdon’s picture

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

The last submitted patch, n-z-clean-up-1317628-18.patch, failed testing.

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
41.73 KB

Re-roll.

jhodgdon’s picture

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

The last submitted patch, n-z-clean-up-1317628-23.patch, failed testing.

jhodgdon’s picture

Issue tags: +Needs reroll

Looks like this one needs a reroll again, sorry for the delay in reviewing! I'm trying to get through these...

Gaelan’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
38.35 KB

Reroll.

jhodgdon’s picture

This patch is going to have to wait for full review/commit until #1168246: Freedom For Fieldsets! Long Live The DETAILS. (a large patch tagged with 'avoid commit conflicts') is taken care of.

jhodgdon’s picture

#29: n-z-clean-up-1317628-29.patch queued for re-testing.

jhodgdon’s picture

(retesting because that patch went in)

Status: Needs review » Needs work

The last submitted patch, n-z-clean-up-1317628-29.patch, failed testing.

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
38.23 KB

Re-rolled to handle password.inc move.

jhodgdon’s picture

Status: Needs review » Needs work

Thanks for all the re-rolls! I finally gave this patch a thorough review. It looks mostly good; just needs a bit of further clean-up:

a) schema.inc

* @return array|bool
- *   If the module has updates, an array of available updates sorted by version.
+ *   If the module has updates, an array of available updates sorted by
+ *   version.
  *   Otherwise, FALSE.
  */
 function drupal_get_schema_versions($module) {

The next line should be moved up to the end of the rewrapped text.

b)

+++ b/core/includes/session.inc
@@ -2,7 +2,7 @@
 
 /**
  * @file
- * User session handling functions.
+ * Handles the users session functions.

This doesn't make any sense to me. How do you handle a function? I think these are functions for handling user sessions, so the previous @file was better. We don't require @file lines to start with a verb. Let's just leave this one as it was.

c) theme.inc not sure which function:

/**
- * Initialize the theme system given already loaded information. This
+ * Initializes the theme system given already loaded information. This
* function is useful to initialize a theme when no database is present.

Needs one-sentence summary. Move next sentence to new paragraph.

This one too:

/**
- * Set the callback that will be used by theme_get_registry() to fetch the registry.
+ * Sets the callback that will be used by theme_get_registry() to fetch the
+ * registry.

And this one a bit farther down:

/**
- * Force the system to rebuild the theme registry; this should be called
+ * Forces the system to rebuild the theme registry; this should be called
  * when modules are added to the system, or when a dynamic system needs
  * to add more theme hooks.

Maybe the rest of theme.inc should be checked for this? I only looked at the patch itself, so only lines that were already being changed. There were a couple more places like this in the patch, and there might be more in the file too?

theme.maintenance.inc has the same problem too, and unicode.inc, and update.inc.

d) theme.inc still

/**
- * Get the theme_registry cache; if it doesn't exist, build it.
+ * Gets the theme_registry cache; if it doesn't exist, build it.

probably that last "build" should be "builds"?

e) I think some of the re-wrapping that was done by this patch wasn't really necessary, but I guess it can be left in the patch. There were a bunch of lines of exactly 80 characters that were rewrapped to be less than 80... they could be left alone and the patch would be a *lot* smaller!

f)

+++ b/core/includes/theme.maintenance.inc
@@ -2,7 +2,7 @@
 
 /**
  * @file
- * Theming for maintenance pages.
+ * Themes for maintenance pages.

This @file change is also wrong. See (b). And this one:

+++ b/core/includes/update.inc
@@ -2,7 +2,7 @@
 
 /**
  * @file
- * Drupal database update API.
+ * Updates the API by Drupal Database.

g) token.inc

/**
- * Given a list of tokens, returns those that begin with a specific prefix.
+ * Gives a list of tokens and returns those begin with a specific prefix.

This change incorrectly altered the meaning of the documentation.

h) unicode.inc

/**
- * Complement to mime_header_encode
+ * Complements to mime_header_encode
  */
 function mime_header_decode($header) {

The original doc was saying that this is the complement function to mime_header_encode(). The new doc doesn't say that. Probably the function doc should actually instead say what the function does and have an @see pointing to the other function.

Similarly, this was not a good change:


 /**
- * Helper function to mime_header_decode
+ * Helps the function mime_header_decode
  */
 function _mime_header_decode($matches) {

And this one:

/**
- * Helper function for case conversion of Latin-1.
+ * Helps the function for case conversion of Latin-1.
  * Used for flipping U+C0-U+DE to U+E0-U+FD and back.
  */
 function _unicode_caseflip($matches) {

And this one in update.inc:

/**
- * Helper function to test compatibility of a module or theme.
+ * Helps the function that test the compatibility of a module or theme.
  */
 function update_check_incompatibility($name, $type = 'module') {

And there are several other "Helper" -> "Helps" changes that need attention.

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
14.53 KB
43.76 KB

This should be all of the issues except for the helper items.

jhodgdon’s picture

On the "helper" functions, the function doc should say what the function does. Then probably have an @see to the main function, or a separate line saying "Helper function for xyz()."

Status: Needs review » Needs work

The last submitted patch, n-z-clean-up-1317628-36.patch, failed testing.

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
1.47 KB
43.82 KB

Christmas vacation patchin' :)

jhodgdon’s picture

Status: Needs review » Needs work

Thanks for the new patch!

I started at the top and gave this a thorough review. It all looks good, except:

a)

--- a/core/includes/session.inc
+++ b/core/includes/session.inc
@@ -270,7 +270,7 @@ function drupal_session_initialize() {
 }
 
 /**
- * Forcefully starts a session, preserving already set session data.
+ * Starts a session, preserving already set session data.
  *
  * @ingroup php_wrappers
  */

I realize we have a standard that we should start with a verb, but this change has lost information that I think was useful. Either revert this change or put "forcefully" back into the line somewhere.

b) There are a couple of functions at the end of theme.inc that don't remotely have one-line verb-starting descriptions (this patch touches them but doesn't fix that problem): template_preprocess_maintenance_page(), template_process_maintenance_page
and the next one, template_preprocess_region(), still has the wrong verb tense.

c) I don't think this change is at all necessary:

+++ b/core/includes/theme.maintenance.inc
@@ -2,7 +2,7 @@
 
 /**
  * @file
- * Theming for maintenance pages.
+ * Theming for the maintenance pages.
  */

d) typo (arrayof is one word in last line):

+++ b/core/includes/token.inc
@@ -61,10 +61,11 @@
  *   replacement process. Supported options are:
  *   - langcode: A language code to be used when generating locale-sensitive
  *     tokens.
- *   - callback: A callback function that will be used to post-process the array
- *     of token replacements after they are generated. For example, a module
- *     using tokens in a text-only email might provide a callback to strip HTML
- *     entities from token values before they are inserted into the final text.
+ *   - callback: A callback function that will be used to post-process the
+ *     arrayof token replacements after they are generated. For example, a

e) same file, typo (being vs. begin):

/**
- * Given a list of tokens, returns those that begin with a specific prefix.
+ * Returns a list of tokens that being with a specific prefix.

f) Unicode should be capitalized:

+++ b/core/includes/unicode.inc
@@ -1,10 +1,13 @@
 <?php
 
 /**
+ * @file
+ * Provides unicode-related conversions and operations.
+ */
+

g) unicode.inc

/**
- * Complement to mime_header_encode
+ * Decodes MIME/HTTP encoded header values.
+ *
+ * @param $header
+ *   The header to encode.
+ *
+ * @return string
+ *   The mime-decoded header.
+ *
+ * @see mime_header_encode()
  */
 function mime_header_decode($header) {

The param says it is encoding instead of decoding.

h) still unicode.inc

/**
- * Helper function to mime_header_decode
+ * Decodes non-encoded header data passed from mime_header_decode().
+ *
+ * @param $matches
+ *   The array of matches from preg_replace_callback().
+ *
+ * @return string
+ *   The mime-decoded header.
+ *
+ * @see mime_header_decode()
  */
 function _mime_header_decode($matches) {

This description doesn't seem right. Why would you need to decode data that wasn't encoded?!? Also, the return value is not an entire header, just a piece of data, right? I also think this should follow our standards for callbacks:
http://drupal.org/node/1354#callbacks

i) still unicode.inc

+ * @return array
+ *   Flips character to Latin-1 version.
+ *
+ * @see drupal_strtolower()
  */
 function _unicode_caseflip($matches) {

That @return description doesn't describe the return value.

j) unicode.inc

+ * @param $length
+ *   The amount of characters to read.

Characters don't have "amounts", they have "numbers" as they are countable. This terminology appears several times in unicode.inc.

k) update.inc

/**
- * Helper function to install a new module in Drupal 8 via hook_update_N().
+ * Helps the function to install a new module in Drupal 8 via hook_update_N().
  */
 function update_module_enable(array $modules) {

That is not a good description. Should probably be "Installs a new ...".

l) update.inc

/**
- * Perform one update and store the results for display on finished page.
+ * Performs one update and store the results for display on finished page.

- store -> stores
- And what is "finished page" anyway? Maybe needs to say 'the "finished" page" or 'the results page'?

m) update.inc

/**
- * Finish the update process and store results for eventual display.
+ * Finishes the update process and store results for eventual display.

store -> stores

n) update.inc

/**
  * Updates config with values set on Drupal 7.x
  *
- * Provide a generalised method to migrate variables from Drupal 7 to Drupal 8's
- * configuration management system.
+ * Provide a generalised method to migrate variables from Drupal 7 to
+ * Drupal 8's configuration management system.

- First line needs to end in .
- In the next paragraph, either provide a subject or keep the same verb tense as first line.

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
7.01 KB
44.78 KB

Got 'em.

jhodgdon’s picture

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

Looks good -- I'll get it committed -- thanks!

jhodgdon’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Whew! Committed to 8.x. Let's see how much of this we can get ported to 7.x. Thanks all!

jhodgdon’s picture

Assigned: jhodgdon » Unassigned
Albert Volkman’s picture

Status: Patch (to be ported) » Needs review
FileSize
35.57 KB

First pass at a D7 backport.

jhodgdon’s picture

Status: Needs review » Fixed

Looks great! Committed to 7.x -- another one of these issues is DONE!

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.