Problem/Motivation

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

Proposed resolution

Correct the following in includes files beginning with A-C:

  • 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

Patch in #43 includes the requested cleanup in all files in includes/ beginning with A through C. The patch incorporates numerous corrections but needs to be reviewed.

Followup tasks for other patches

  1. WTFs @ format_date() WRT _format_date_callback() usage.
  2. Check list indentations.
  3. Check for double spaces after periods.

Followup issues

Related issues

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

Version: 7.x-dev » 8.x-dev

Durr.

xjm’s picture

Issue summary: View changes

Adding working notes to 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

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

.

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

.

jhodgdon’s picture

Saw your summary...

Regarding callbacks, I just noticed that also and filed:
#1315992: No standard for documenting menu callbacks
so we could create a standard.

Regarding classes/interfaces, we do have standards:
http://drupal.org/node/1354#classes

And I think you can clean out extra lines of whitespace at tops of files in this patch -- that doesn't add in any significant way to the review burden.

jhodgdon’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

jhodgdon’s picture

Regarding helper/shutdown functions: I say leave them as-is for now... I'm not too concerned about 1-line docs that start with:

Something blah: verbs in the right format.

jhodgdon’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

Issue summary: View changes

updated summary

xjm’s picture

Issue summary: View changes

.

xjm’s picture

Status: Active » Needs work
FileSize
53.83 KB

Work so far, through bootstrap.inc, minus the unfinished tasks listed above.

xjm’s picture

+++ b/includes/bootstrap.incundefined
@@ -2062,20 +2084,23 @@ function drupal_anonymous_user() {
+ * ¶

Whitespace.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

+++ b/includes/bootstrap.incundefined
@@ -485,8 +490,10 @@ function timer_stop($name) {
+ * configuration file found will be used and the remaining ones will be
+ * ignored.
+ * If no configuration file is found, return a default value
+ * '$confdir/default'.

Didn't finish fixing the wrapping here.

+++ b/includes/bootstrap.incundefined
@@ -1103,8 +1110,10 @@ function bootstrap_invoke_all($hook) {
+ * This function prevents including a theme, engine, module, etc., more than
+ * once.

Do we want the word "function" here?

+++ b/includes/bootstrap.incundefined
@@ -1206,8 +1218,10 @@ function _drupal_set_preferred_header_name($name = NULL) {
+ * if they have been replaced or unset using
  * drupal_add_http_header().

Looks like this needs wrapping fixed.

+++ b/includes/bootstrap.incundefined
@@ -2854,6 +2882,7 @@ function drupal_get_schema($table = NULL, $rebuild = FALSE) {
+  // Constructs a SchemaCache object.

Should be a docblock.

how i can computer? why is blue? views? modules? wtf?

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

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Attached includes the above cleanups and is complete through bootstrap.inc, except for the points mentioned in the summary (#1315992: No standard for documenting menu callbacks for the ajax menu callbacks and the point about overridden methods). 1563 lines already. :)

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

.

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

.

xjm’s picture

Through common.inc. Haven't proofread it yet. Outstanding issue with JS_LIBRARY docblock.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

That patch is pretty big already and there are still 16 files to go. I am wondering if it might be better to split off these files and do the rest in a second patch.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Title: Clean up API docs for includes directory, files starting with A-M » Clean up API docs for includes directory, files starting with A-C
  • So, there are 44,000-some total lines in the includes directory. I dumped the wc in a spreadsheet to roughly divide it up alphabetically. Based on that, I propose splitting it into 4 groups for sprints like these:
  • A-C (~14k lines)
  • D-G (~9.5k lines)
  • I-M (~10.5k lines)
  • N-Z (~10k lines)

I'll update the meta issue summary for this.

xjm’s picture

+++ b/includes/ajax.incundefined
@@ -348,11 +351,14 @@ function ajax_get_form() {
+ * Paths:
+ *   - system/ajax

Only one path, so one line.

+++ b/includes/ajax.incundefined
@@ -381,21 +389,29 @@ function ajax_form_callback() {
+ * Theme callback: returns the correct theme for an Ajax request.

Capitalize.

+++ b/includes/batch.incundefined
@@ -370,7 +370,7 @@ function _batch_process() {
+ * Returns the formatted percentage for _batch_process().

Add an @see instead and change to "Formats the percent completion for a batch set."

+++ b/includes/batch.incundefined
@@ -521,7 +522,7 @@ function _batch_finished() {
- * Shutdown function; store the current batch data for the next request.
+ * Shutdown function: stores the current batch data for the next request.
  */

Capitalize and add @see.

+++ b/includes/cache.incundefined
@@ -175,13 +181,14 @@ function cache_clear_all($cid = NULL, $bin = NULL, $wildcard = FALSE) {
 /**
- * Check if a cache bin is empty.
+ * Checks if a cache bin is empty.
  *
- * A cache bin is considered empty if it does not contain any valid data for any
- * cache ID.
+ * A cache bin is considered empty if it does not contain any valid data for
+ * any cache ID.
  *
  * @param $bin
  *   The cache bin to check.
+ ¶
  * @return
  *   TRUE if the cache bin specified is empty.

Whitespace.

+++ b/includes/cache.incundefined
@@ -330,13 +342,13 @@ interface DrupalCacheInterface {
-   * @todo: this method is deprecated, as it's functionality is covered by
+   * @todo: This method is deprecated, as its functionality is covered by

I vaguely remember something about not using @todo; check docs.

+++ b/includes/common.incundefined
@@ -86,8 +86,7 @@ define('JS_DEFAULT', 0);
 /**
- * Error code indicating that the request made by drupal_http_request() exceeded
- * the specified timeout.
+ * Error code indicating that the request exceeded the specified timeout.
  */

Add an @see to drupal_http_request().

+++ b/includes/common.incundefined
@@ -146,12 +150,12 @@ define('DRUPAL_CACHE_PER_USER', 0x0002);
 /**
- * The block or element is the same for every user on every page where it is visible.
+ * The element is the same for every user on every page where it is visible.

Not sure if dropping the word block is advised since that would make it different from all the other constants in the group. Try something else to get under 80 char.

+++ b/includes/common.incundefined
@@ -1024,6 +1029,14 @@ function drupal_http_request($url, array $options = array()) {
+ * Strips slashes from a string or array of strings.

Add "Callback: " prefix?

+++ b/includes/common.incundefined
@@ -1034,11 +1047,13 @@ function _fix_gpc_magic(&$item) {
- * Helper function to strip slashes from $_FILES skipping over the tmp_name keys
- * since PHP generates single backslashes for file paths on Windows systems.
+ * Strips slashes from $_FILES items.
+ * ¶
+ * The tmp_name key is skipped keys since PHP generates single backslashes for
+ * file paths on Windows systems.

1. Add "Callback: "?
2. Whitespace

+++ b/includes/common.incundefined
@@ -3142,7 +3173,7 @@ function drupal_aggregate_css(&$css_groups) {
- * #pre_render callback to add the elements needed for CSS tags to be rendered.
+ * Preprocesses CSS elements for rendering.

Change to "Pre-render callback: Adds the elements needed..."

+++ b/includes/common.incundefined
@@ -5083,7 +5123,7 @@ function drupal_cron_run() {
+ * Shutdown function: performs cron cleanup.

Capitalize.

+++ b/includes/common.incundefined
@@ -5232,7 +5274,7 @@ function drupal_set_page_content($content = NULL) {
- * #pre_render callback to render #browsers into #prefix and #suffix.
+ * Preprocess a render array to convert #browsers into #prefix and #suffix.

Pre-render callback: Renders #browsers...

+++ b/includes/common.incundefined
@@ -5308,7 +5350,7 @@ function drupal_pre_render_conditional_comments($elements) {
- * #pre_render callback to render a link into #markup.
+ * Preprocesses a render array link element to convert it into #markup.

Pre-render callback: Renders a link...

+++ b/includes/common.incundefined
@@ -5362,7 +5404,7 @@ function drupal_pre_render_link($element) {
- * #pre_render callback that collects child links into a single array.
+ * Preprocesses a render array to collect child links into a single array.

Pre-render callback: Collects...

+++ b/includes/common.incundefined
@@ -5453,20 +5495,20 @@ function drupal_pre_render_links($element) {
- * #pre_render callback to append contents in #markup to #children.
+ * Preprocesses a render array to move the contents of #markup to #children.

Pre-render callback: Appends...

+++ b/includes/common.incundefined
@@ -6028,7 +6077,7 @@ function drupal_render_cid_create($elements) {
- * Function used by uasort to sort structured arrays by weight.
+ * Sorts a structured array by weight.

Sorting callback: Sorts...

+++ b/includes/common.incundefined
@@ -6040,7 +6089,7 @@ function element_sort($a, $b) {
- * Array sorting callback; sorts elements by title.
+ * Sorts a structured array by title.

Sorting callback: Sorts...

+++ b/includes/common.incundefined
@@ -6090,7 +6139,7 @@ function element_info_property($type, $property_name, $default = NULL) {
- * Function used by uasort to sort structured arrays by weight, without the property weight prefix.
+ * Sorts a structured array by weight, without the property weight prefix.

Sorting callback: Sorts...

+++ b/includes/common.incundefined
@@ -6102,7 +6151,7 @@ function drupal_sort_weight($a, $b) {
- * Array sorting callback; sorts elements by 'title' key.
+ * Sorts a structured array by 'title' key.

Sorting callback: Sorts...

Also drupal_sort_css_js().

how i can computer? why is blue? views? modules? wtf?

xjm’s picture

I'll wait on these corrections until we RTBC #1315992: No standard for documenting menu callbacks.

jhodgdon’s picture

You reviewed your own patch? :)

xjm’s picture

It's still marked NW. :P

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue tags: -Needs backport to D7

Untagging.

xjm’s picture

FileSize
170.25 KB
xjm’s picture

+++ b/includes/batch.queue.incundefined
@@ -12,13 +11,24 @@
+  /**
+   * Claims an item in the batch queue for processing.
+   *
+   * @param $lease_time
+   *   (optional) How long the processing is expected to take in seconds. After
+   *   this lease expires, the item will be reset and another consumer can
+   *   claim the item.  Defaults to 0.
+   *
+   * @return
+   *   A batch item object, or FALSE if no item could be claimed.

@@ -44,10 +54,21 @@ class BatchQueue extends SystemQueue {
+  /**
+   * Claims an item in the non-progressive batch queue for processing.
+   *
+   * @param $lease_time
+   *   (optional) How long the processing is expected to take in seconds. After
+   *   this lease expires, the item will be reset and another consumer can
+   *   claim the item.  Defaults to 0.
+   *
+   * @return

See if "short form" documentation should be used. (For the second for sure.)

+++ b/includes/cache-install.incundefined
@@ -15,17 +15,30 @@
+  /**
+   * Overrides DrupalDatabaseCache::delete().
+   */
   function get($cid) {

Fail.

+++ b/includes/common.incundefined
@@ -732,7 +740,8 @@ function drupal_access_denied() {
+ *   - headers: An array containing request headers to send as name/value
+       pairs.

Missing *.

+++ b/includes/common.incundefined
@@ -2963,7 +3001,7 @@ function drupal_get_css($css = NULL, $skip_alter = FALSE) {
- * Function used by uasort to sort the array structures returned by drupal_add_css() and drupal_add_js().
+ * Sorts the array structures returned by drupal_add_css() and drupal_add_js().

@@ -3003,7 +3041,7 @@ function drupal_sort_css_js($a, $b) {
- * Default callback to group CSS items.
+ * Groups CSS items of the styles element by their types, media, and browsers.

@@ -3100,7 +3138,7 @@ function drupal_group_css($css) {
- * Default callback to aggregate CSS files and inline content.
+ * Aggregates CSS files and inline content.

Switch to callback pattern.

-23 days to next Drupal core point release.

xjm’s picture

FileSize
171.19 KB
xjm’s picture

Status: Needs work » Needs review

Alright, I think this is ready for another pair of eyes. :)

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Status: Needs review » Needs work

Just noticed a missing period in #18 -- reroll incoming.

xjm’s picture

Status: Needs work » Needs review
FileSize
171.19 KB

Missing period for drupal_sort_css_js() summary fixed. Also rerolled against current 8.x.

xjm’s picture

Assigned: xjm » Unassigned
jhodgdon’s picture

Assigned: Unassigned » xjm
Status: Needs review » Needs work

Looks pretty close! A couple of things I noted:

a)

+ *   ENTER key within a textfield triggers the 'click' event of the form's
+ *   first *submit button. Triggering Ajax in this situation leads to

The rewrapping here inserted a * in the line. Oops!

b)

+  /**
+   * Overrides SystemQueue::claimItem().
+   *
+   * @param $lease_time
+   *   (optional) How long the processing is expected to take in seconds. After
+   *   this lease expires, the item will be reset and another consumer can
+   *   claim the item.  Defaults to 0.
+   *
+   * @return
+   *   A batch item object, or FALSE if no item could be claimed.
+   */
   public function claimItem($lease_time = 0) {

A one-liner should be enough here. The param/return docs should go on SystemQueue::claimItem() (and yes, I'm aware that function doesn't currently have any doc, but hopefully by the end of this process it will).

c) Same applies to other overrides in this file.

d)

@@ -319,10 +319,16 @@ abstract class DrupalCacheArray implements ArrayAccess {
     }
   }
 
+  /**
+   * Overrides ArrayAccess::offsetExists().
+   */
   public function offsetExists($offset) {

I think this should be implements rather than overrides? This is a class that is implementing an interface, not a class that is extending a base class and overriding some of its methods. Same applies to several other methods in the patch.

e)

+ * Initializes PHP environment.

Might need a "the" in there? Other "the" spots:
+ * Sets up script environment and loads settings.php.
+ * Attempts to serve a page from cache.
+ * Initializes database system and registers autoload functions.
etc.

f) Maybe not the right issue to clean this up, but this is not accurate:

+ * The bootstrap phase is a string describing a phase of Drupal to load. Each
+ * phase adds to the previous one, so invoking a later phase automatically

The bootstrap phases are integer constants, not strings. Separate issue?

g)

- * Wrapper around parse_url() to parse a system URL string into an associative array, suitable for url().
+ *  Parses a system URL string into an associative array suitable for url().

Extra space between * and Parses.

h)

+ * Callback: Strips slashes from $_FILES items.

Just a reminder that we're working on the standards for this on #1317826: Provide general documentation standard for callbacks (xjm knows this :) ).

i)

+ *   (Deprecated) Whether to decode entities in the $string. Set to FALSE if

Our standard for optional is lower-case, so deprecated should be too?

j)

+ *   First item for comparison.  The compared items should be associative

One space after ".".

k)

 /**
- * Prepare a string for use as a valid HTML ID and guarantee uniqueness.
+ * Prepares a string for use as a valid HTML ID and guarantee uniqueness.

guarantee -> guarantees

l) Some of the rewrapping in the patch is good, but some seems to be a bit over-agressive. Example:

- * Faster than actions_actions_map() when you only need the function name or ID.
+ * Faster than actions_actions_map() when you only need the function name or
+ * ID.

By my editor's count, the original line (in the file, not in the patch file) was 80 characters, so this didn't need to be rewrapped. There are quite a few others in the patch that I think also didn't need to be rewrapped -- you might want to check your editor settings. Note that patch files have an extra character at the beginning of the line, so don't count that.

Phew! Big patch!!

jhodgdon’s picture

The review above was for the patch in #18.

xjm: when you un-assign the issue after uploading a patch, that indicates to me that you plan to not work on it any more if it needs work... is that the case? Leaving the cross-posted assign in hopes that is not the case. :)

xjm’s picture

Er, sorry, I just meant to indicate I wasn't actively working on it at that time. :)

For the claimItem() methods, they override the default value that the base class (edit: Er, interface? damn OO) method provides. So I think the @param is needed there.

Re: The line wrapping -- The script I ran listed all the lines were greater than 80 characters. In my editor, there were a lot of these same lines that wrapped exactly one character in an 80-column terminal. I wonder if the script (and my editor) are counting the linebreak character in the total, and if that is desirable or not?

xjm’s picture

Hmm, I will try to roll a patch without the char-limit hunks for now 'til I can figure out the deal there.

xjm’s picture

FileSize
103.63 KB

Uploading for safekeeping a patch that just backs out all rewrapping for now (except for obvious things that were way over). #23 is not addressed yet (except those points relevant to the backed out hunks, naturally). Thank you, git gui. And, hey, it's 1/3 smaller this way!

xjm’s picture

FileSize
73.43 KB

Should really do this as well. I'll double check this tomorrow.

xjm’s picture

Or now.

+++ b/includes/actions.incundefined
@@ -228,14 +228,12 @@ function actions_actions_map($actions) {
- *

Yeah, add that back.

+++ b/includes/common.incundefined
@@ -1570,12 +1566,10 @@ function _filter_xss_attributes($attr) {
- *

This one too.

+++ b/includes/common.incundefined
@@ -4750,7 +4738,6 @@ function drupal_get_library($module, $name = NULL) {
- *

And him.

xjm’s picture

Status: Needs review » Needs work

Here's a printout of the lines I see as over 80 chars (the top key indicates line length). @jhodgdon, could you confirm whether the lines listed as 81 appear to be the correct (maximum) length to you?

Lines over 80 chars:
Array
(
    [81] => Array
        (
            [0] => actions.inc line 231
            [1] => ajax.inc line 49
            [2] => ajax.inc line 129
            [3] => ajax.inc line 150
            [4] => ajax.inc line 152
            [5] => ajax.inc line 158
[truncated because I am sick of scrolling past this]
xjm’s picture

Alright, addressing #23.

a: Reverted in line wrap revert.
b, c: The default value for $lease_time is different from that in SystemQueue::claimItem(), so I think the @param lines need to be added there.
d: I'm a bit OO-challenged and consequently foggy on the terminology here so bear with me. Can you confirm if this is correct:

  • If the code says: class Foo extends Bar
    Then: The short-form docblocks for methods can begin with "Overrides"
  • If the code says: class Foo implements Bar
    Then: The short-form docblock for methods can begin with "Implements."
  • If the code says: class Foo extends Bar implements Baz
    Then: If the method is only found in Baz, "Implements." If the method is found in Bar, "Overrides."

e: I guess "the" seems optional to me in those, but I will fix them with your suggestions.
f: I hate that docblock and I am already changing it, so I will kill that kitten.
g: Ah, thanks, will fix that.
h: Will go through and fix these to what we settled on.
i: I was deferring cleanup of the optional parameter stuff to a separate issue since this patch is so long.
j: Thanks. When I was 11 and I took typing, the teacher taught me to put two spaces after periods and I do it without thinking. I am mad at that teacher. :)
k: Thanks, I'll fix this.
l: Covered in #25 and #30.

jhodgdon’s picture

RE #30 line length - the lines you have marked as 81 characters do not wrap in my editor. Do they in yours? Maybe we need to get a few opinions on this. In Emacs, when I set the fill-column to 80 characters and auto-wrap those lines, they stay as-is. (ctrl-u 80 esc-x set-fill-column, esc-q)

On the other hand, for instance in actions.inc, on line 80 or so, there is a @param with a paragraph that could be wrapped longer. Is your script doing that too, or just catching the over-length lines?

RE #31 -
b/c - ok
d - yes you are correct
i - all I meant is that if (deprecated) is there, the word "deprecated" should be lower-case.
j - Me too, two spaces is a hard habit to break.

jhodgdon’s picture

Issue summary: View changes

.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

FileSize
32.91 KB
121.37 KB

Attached addresses the above stuff, including re-wrapping for paragraphs with lines over 80 when the linebreak is excluded. Haven't proofread yet.

Edit: We dropped 50k by not rewrapping the 81 char lines. :)

xjm’s picture

+++ b/includes/common.incundefined
@@ -5606,18 +5622,18 @@ function drupal_render_page($page) {
+ *    - 'keys': An array of one or more keys that identify the element. If ¶

Whitespace.

+++ b/includes/common.incundefined
@@ -6161,12 +6177,17 @@ function element_info_property($type, $property_name, $default = NULL) {
+ * Callbackfor uasort() within:
+ * - _field_ui_reduce_order()
+ * - update_resolve_dependencies()
+ * - archiver_get_info()
+ * - drupal_get_updaters()
+ * - drupal_get_filetransfer_info()
  *

Typo (missing space in "Callbackfor").

Also, I'm on the fence as to whether it's worthwhile to include a list when there's this many.

+++ b/includes/common.incundefined
@@ -7477,7 +7498,7 @@ function archiver_get_extensions() {
- *   The full path of the archive file.  Note that stream wrapper
+ *   The full path of the archive file. Note that stream wrapper

Fix wrapping here while we're at it.

xjm’s picture

+++ b/includes/actions.incundefined
@@ -176,10 +178,10 @@ function actions_list($reset = FALSE) {
+ * called, which happens when admin/config/system/actions is visited.
+ * Configurable actions are not added to the database until they are
+ * configured in the user interface, in which case a database row is created
+ * for each configuration of each action.

Check wrapping here; that looks like a big gap.

+++ b/includes/actions.incundefined
@@ -223,7 +226,7 @@ function actions_actions_map($actions) {
- * Given a hash of an action array key, returns the key (function or ID).
+ * Returns an action array key (function or ID) from its hash.
  *

Change to "given its hash."

+++ b/includes/ajax.incundefined
@@ -188,11 +189,12 @@
+ * Each command item is an associative array which will be converted to a
+ * command object on the JavaScript side. $command_item['command'] is the
+ * type of command, e.g. 'alert' or 'replace', and will correspond to a
+ * method in the Drupal.ajax[command] space. The command array may contain
+ * any other data that the command needs to process, e.g. 'method', 'selector',

Check wrapping.

+++ b/includes/authorize.incundefined
@@ -186,7 +189,7 @@ function _authorize_filetransfer_connection_settings_set_defaults(&$element, $ke
+ * Form validation handler for the authorize_filetransfer_form().

The word "the" is not needed here.

+++ b/includes/batch.queue.incundefined
@@ -12,13 +11,24 @@
+   *
+   * @return
+   *   A batch item object, or FALSE if no item could be claimed.

@@ -44,10 +54,21 @@ class BatchQueue extends SystemQueue {
+   *
+   * @return
+   *   A batch item object, or FALSE if no item could be claimed.

The parameter default is overriden, but not the return value, so remove @return.

+++ b/includes/bootstrap.incundefined
@@ -2062,20 +2092,23 @@ function drupal_anonymous_user() {
+ * automatically runs the earlier phases as well. The most important usage is
+ * that if you want to access the Drupal database from a script without
+ * loading anything else, you can include bootstrap.inc, and call

With further kitten peril, this sentence could be simplified to "To access the Drupal database from a script without loading anything else, include bootstrap.inc and call..."

+++ b/includes/bootstrap.incundefined
@@ -2752,16 +2785,17 @@ function request_path() {
+ * Avoid use of this function where possible, as resulting code is hard to
+ * read. In menu callback functions, attempt to use named arguments. See the
+ * explanation in menu.inc for how to construct callbacks that take
+ * arguments. When attempting to use this function to load an element from
+ * the current path, e.g. loading the node on a node page, please use
+ *  menu_get_object() instead.

Check wrapping.

+++ b/includes/cache-install.incundefined
@@ -6,7 +6,7 @@
+ * Provides a stub cache implementation to be used during installation.

Should this be "defines"?

+++ b/includes/cache.incundefined
@@ -236,31 +243,34 @@ interface DrupalCacheInterface {
+   * Returns data from the persistent cache.
+   *
+   * Data may be stored as either plain text or as serialized data.
+   * cache_get() will automatically return unserialized objects and arrays.

Check wrapping.

+++ b/includes/cache.incundefined
@@ -304,23 +314,25 @@ interface DrupalCacheInterface {
-   * Expire temporary items from cache.
+   * Expires temporary items from cache.

From THE cache, to be consistent with other methods?

+++ b/includes/cache.incundefined
@@ -330,13 +342,13 @@ interface DrupalCacheInterface {
-   * @todo: this method is deprecated, as it's functionality is covered by
+   * @todo: This method is deprecated, as its functionality is covered by
    * more targeted methods in the interface.

Adjust wrapping?

+++ b/includes/cache.incundefined
@@ -360,46 +372,86 @@ interface DrupalCacheInterface {
- * cached data. Each cache bin corresponds to a database table by the same name.
+ * cached data. Each cache bin corresponds to a database table by the same
+ * name.

Unnecessary wrapping?

+++ b/includes/common.incundefined
@@ -110,7 +112,9 @@ define('HTTP_REQUEST_TIMEOUT', -1);
+ * This setting should be used:
  * - for simple blocks (notably those that do not perform any db query),
  * where querying the db cache would be more expensive than directly generating
  * the content.

Looks like list indentation might be incorrect; maybe we can fix that.

+++ b/includes/common.incundefined
@@ -119,22 +123,25 @@ define('HTTP_REQUEST_TIMEOUT', -1);
+ * This is the default setting for blocks, used when the block does not
+ * specify anything.

Check wrapping.

+++ b/includes/common.incundefined
@@ -265,7 +271,7 @@ function drupal_get_rdf_namespaces() {
- * Add output to the head tag of the HTML page.
+ * Adds output to the head tag of the HTML page.

Capitalize HEAD tag for consistency.

+++ b/includes/common.incundefined
@@ -711,19 +717,20 @@ function drupal_not_found() {
+ * Delivers a "access denied" error to the browser.

AN access denied error.

+++ b/includes/common.incundefined
@@ -1034,11 +1049,14 @@ function _fix_gpc_magic(&$item) {
- * Helper function to strip slashes from $_FILES skipping over the tmp_name keys
- * since PHP generates single backslashes for file paths on Windows systems.
+ * Strips slashes from $_FILES items.
  *
- * tmp_name does not have backslashes added see
- * http://php.net/manual/en/features.file-upload.php#42280
+ * Callback for array_walk() within fix_gpc_magic().
+ *
+ * The tmp_name key is skipped keys since PHP generates single backslashes for
+ * file paths on Windows systems.
+ *
+ * @see: http://php.net/manual/en/features.file-upload.php#42280
  */

Missing @param. Also, no colon after @see.

+++ b/includes/common.incundefined
@@ -1943,7 +1975,9 @@ function date_iso8601($date) {
 /**
- * Callback function for preg_replace_callback().
+ * Translates a formatted date string.
+ *
+ * Callback for preg_replace_callback() within format_date().

Missing @param and @return.

+++ b/includes/common.incundefined
@@ -2677,10 +2712,10 @@ function drupal_get_path($type, $name) {
+ * base_path() prefixes and suffixes a "/" onto the returned path if the path

Might want to change this to "adds a '/' to the beginning and end of the returned path..." It's a bit confusing as written.

+++ b/includes/common.incundefined
@@ -2963,11 +2998,24 @@ function drupal_get_css($css = NULL, $skip_alter = FALSE) {
+ *   arrays of member items from drupal_add_css() or drupal_add_js(). ¶

Whitespace.

+++ b/includes/common.incundefined
@@ -3377,7 +3427,7 @@ function drupal_pre_render_styles($elements) {
- * variable is emptied to force a rebuild of the cache.  Second, the cache
+ * variable is emptied to force a rebuild of the cache. Second, the cache
  * file is generated if it is missing on disk. Old cache files are not deleted

Fix rewrapping.

+++ b/includes/common.incundefined
@@ -5557,18 +5622,18 @@ function drupal_render_page($page) {
+ *    - 'keys': An array of one or more keys that identify the element. If ¶

Whitespace.

+++ b/includes/common.incundefined
@@ -5557,18 +5622,18 @@ function drupal_render_page($page) {
+ *       See drupal_render_cid_create().

Should this be in an @see at the end?

+++ b/includes/common.incundefined
@@ -5809,16 +5875,17 @@ function show(&$element) {
+ * Gets the rendered output of a renderable element from cache.

From THE cache.

+++ b/includes/common.incundefined
@@ -5918,9 +5985,10 @@ function drupal_render_collect_attached($elements, $return = FALSE) {
+ * Prepares an element for caching based on a query.
+ *
+ * This smart caching strategy saves Drupal from querying and rendering to
+ * HTML when the underlying query is unchanged.

Fix wrapping.

+++ b/includes/common.incundefined
@@ -6745,7 +6856,9 @@ function _drupal_schema_initialize(&$schema, $module, $remove_descriptions = TRU
+ * The returned list is suitable for use in a SQL query.

AN SQL query. Edit: I guess this depends whether or not one reads it as "Sequel."

+++ b/includes/common.incundefined
@@ -7378,11 +7495,12 @@ function archiver_get_extensions() {
  * @param $file
- *   The full path of the archive file.  Note that stream wrapper
+ *   The full path of the archive file. Note that stream wrapper
  *   paths are supported, but not remote ones.

Fix wrapping.

xjm’s picture

Oops, one last bit:

+++ b/includes/common.incundefined
@@ -7434,10 +7552,10 @@ function drupal_get_updaters() {
- * Drupal FileTransfer registry.
+ * Assembles Drupal FileTransfer registry.

Assembles THE Drupal FileTransfer registry.

So it looks like I missed reverting a few of the 81 char rewraps before, but now I think that should be all of them.

xjm’s picture

Status: Needs work » Needs review
FileSize
14.03 KB
121.2 KB

Somebody killed a different kitten so attached tries to resolve a merge conflict. Sending to testbot to make sure it isn't brokened.

xjm’s picture

Status: Needs review » Needs work

Back to NW now that testbot is queued.

xjm’s picture

+++ b/includes/ajax.incundefined
@@ -381,10 +387,17 @@ function ajax_form_callback() {
  * Many different pages can invoke an Ajax request to system/ajax or another
  * generic Ajax path. It is almost always desired for an Ajax response to be
+ * @see system_menu()
+ * @see file_menu()
  * rendered using the same theme as the base page, because most themes are built
  * with the assumption that they control the entire page, so if the CSS for two

Something weird happened here.

+++ b/includes/bootstrap.incundefined
@@ -2882,11 +2917,23 @@ function drupal_get_schema($table = NULL, $rebuild = FALSE) {
+  /**
+   * Resolves a cache miss.
+   *
+   * @param $offset
+   *   The offset that was requested.
+   *
+   * @return
+   *   The value of the offset, or NULL if no value was found.

Just use "Overrides DrupalCacheArray::resolveCacheMiss()."

+++ b/includes/common.incundefined
@@ -2188,7 +2227,7 @@ function url_is_external($path) {
+ * Formats an attribute string for a HTTP header.

An HTTP header.

+++ b/includes/common.incundefined
@@ -4993,10 +5056,10 @@ function drupal_page_set_cache() {
+ *   Returns TRUE if cron ran successfully.

Can drop the word "Returns."

-26 days to next Drupal core point release.

xjm’s picture

Status: Needs work » Needs review
FileSize
2.31 KB
121.23 KB

Alright, your turn again. :)

xjm’s picture

FileSize
449 bytes
121.23 KB
+++ b/includes/bootstrap.incundefined
@@ -2926,13 +2926,7 @@ class SchemaCache extends DrupalCacheArray {
+   * Overrides DrupalCacheArray::resolveCacheMiss(),

Argh. Should have been a period. Fix attached.

Lars Toomre’s picture

Issue tags: +Needs backport to D7

Thanks for working on this issue xjm. Cleaning up the documentation in the a-c includes files is producing a big patch that takes a while to review. In reviewing this patch, I noticed that there are inconsistencies in how list items are formatted.

For instance, drupal_add_js() uses the pattern "- key:", while drupal_render_page() uses the pattern "- 'key':" (single quotes around the key). I tend to use the latter pattern in my own custom code. However, reviewing Doxygen and comment formatting conventions for list formatting just now, I see that "- key:" is the Drupal documentation standard. Hence, this patch needs work to correct the key lists in at least the following functions:

format_plural()
url()
drupal_render_page()
drupal_parse_dependency()

I did not check the source files themselves. Hence, there may also be other functions that need to be corrected for improper list item formats that are not included in the #41 patch file.

xjm’s picture

Status: Needs work » Needs review
Issue tags: -Needs backport to D7
FileSize
95 KB
12.93 KB
125.17 KB

Noticed a few things I'd originally fixed in #18 that were backed out with the wrapping in #27. Attached:

  • Reformats a few lists with excessive indentation.
  • Removes the word "please" in docblocks that were already being altered. For consistency with #679890: Using "Please" in the interface, it seems our API text should be similarly less polite, since after all the API is the programmer's UI. ;)
  • Corrects "incidences" to "instances".
  • Fixes double spaces after periods.

I've also attached an interdiff against the patch that was previously reviewed in #18.

xjm’s picture

For instance, drupal_add_js() uses the pattern "- key:", while drupal_render_page() uses the pattern "- 'key':" (single quotes around the key). I tend to use the latter pattern in my own custom code. However, reviewing Doxygen and comment formatting conventions for list formatting just now, I see that "- key:" is the Drupal documentation standard. Hence, this patch needs work to correct the key lists in at least the following functions:

format_plural()
url()
drupal_render_page()
drupal_parse_dependency()

Thanks for reviewing! However, things like this are being deferred to followup issues. I'll add it to the issue summary.

Edit: Also, we are not backporting the cleanups to D7, per webchick and jhodgdon, because they are too disruptive.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Just to reiterate: I personally am not going to work on cleaning up everything in all the doxygen in the includes directory in a single patch. Otherwise, this beast will be stuck in reroll hell forever. :) Please open followup issues for additional fixes that aren't part of the sprint once these patches land.

Also, this will need to be rerolled after Tuesday. I'll keep fixing this one and try to merge it, but I will start afresh after Tuesday for the rest of the includes directory, based on what we've worked out over the course of this issue.

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

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.

Lars Toomre’s picture

Regarding #44 about list formatting, I have proactively created a follow-up issue #1326574: Correct list stuff in docs for includes/database with an attached patch. That patch contains a correction for the malformed list in the common.inc as well as approximately 35 other core files where list formatting does not conform to core documentation standards.

Depending upon which patch gets committed first, this patch may need to be re-rolled. I am happy to re-roll mine to keep up as the documentation sprint patches are committed.

Lars Toomre’s picture

Issue summary: View changes

Updated issue summary.

jhodgdon’s picture

Let's keep the list formatting in a separate issue. Thanks Lars!

xjm’s picture

FileSize
125.37 KB

Rerolled for core/ directory.

xjm’s picture

Issue summary: View changes

Updated issue summary.

jhodgdon’s picture

Status: Needs review » Needs work

Looks pretty good... A few notes/questions/todos:

a)

 /**
- * Given a hash of an action array key, returns the key (function or ID).
+ * Returns an action array key (function or ID) given its hash.

Does this need a comma before "given"?

b)

 /**
- * Form element process callback to handle #ajax.
+ * Form element processing handler to handle the #ajax form property.

"handler to handle" seems a bit weird. How about "Form element processing handler for the #ajax..."?

c) function authorize_filetransfer_form() -- needs @see for its validate/submit handler functions. Validate and submit should also point to each other with @see.

d)

+   *   claim the item.  Defaults to 0.

Two spaces after the period - should only be one. Please check the rest of the file for this if it's easy in your editor. This exact line with two spaces appears at least twice.

Whew, long patch! That's all I see that needs help...

xjm’s picture

+ * claim the item. Defaults to 0.

Two spaces after the period - should only be one. Please check the rest of the file for this if it's easy in your editor. This exact line with two spaces appears at least twice.

Okay, I actually wrote a shell script last week to parse and check for this, so I don't know how it got by me! Edit: Maybe I only checked common.inc.... hmm.

I'll reroll with these fixes shortly.

xjm’s picture

FileSize
6.07 KB
127.4 KB

Corrected the above and found a couple more superfluous post-period spaces in files other than common.inc.

xjm’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: -docs-cleanup-2011

The last submitted patch, a-c-51.patch, failed testing.

jhodgdon’s picture

Status: Needs work » Needs review
Issue tags: +docs-cleanup-2011

#51: a-c-51.patch queued for re-testing.

Lars Toomre’s picture

Hi @xjm and @jhodgdon. The power is still out for much of Connecticut including me. Hence, I only am getting very intermittent Internet access like now as I pick-up dinner.

I have been working off-line (via a car generator) on the list items documentation issue (and its associated (optional)/(required) descriptions) #1326574: Correct list stuff in docs for includes/database. That issue has now expanded to almost 70 affected core files and will be a very big patch if it all goes in at once.

My current thinking is that #1326574: Correct list stuff in docs for includes/database probably will need to become a meta issue with several sub-issues of its own to get it all committed. (I need to roll a new "big" patch for that issue now that I have just now pulled down the current Drupal version including the movement to /core.)

Three of the files from that issue (archiver.inc, bootstrap.inc and common.inc) overlap with this issue. Hence, I am now thinking that I will try to add your patch (as is if too had been committed) and then overlay my changes to those three files on top to see what the results look like. If I can get git to behave sufficiently, I will then try to roll a smaller patch with only the three overlapping files for further discussion starting tomorrow (hoprfully!!!).

This make sense? I will try to check back later in the evening for any updates or thoughts. More to come ...

Status: Needs review » Needs work

The last submitted patch, a-c-51.patch, failed testing.

xjm’s picture

Status: Needs work » Needs review

Okay, finally testbot doesn't hate me anymore.

Lars Toomre’s picture

Re-testing patch in #51. I have tried to apply it in order to then roll lists changes patch, but I am getting 'patch failed: core/includes/common.inc:70'. Let's see the test bots think ...

Lars Toomre’s picture

Issue tags: -docs-cleanup-2011

#51: a-c-51.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +docs-cleanup-2011

The last submitted patch, a-c-51.patch, failed testing.

xjm’s picture

Just needs to be rerolled again. You don't need to run testbot to guess that. ;)

Lars Toomre’s picture

@xjm: Given my minimal Git skills, I was presuming that I had done something wrong while setting up branches, etc.

xjm’s picture

Status: Needs work » Needs review
FileSize
1007 bytes
127.32 KB

Here's the needed reroll. I've also attached a text file showing the changes made to resolve the merge conflict (which are the only differences betweeen this and 51).

Lars Toomre’s picture

The 'easy lists' patch to wrap around this one when it is committed is now largely done. That patch can be found at #1333534-1: Further cleanup for documentation in core/includes files starting with A-G . It may need to be re-rolled for this issue and/or #1317620: Clean up API docs for includes directory, files starting with D-G get committed.

However, I am glad to have much of the heavy lifting from my mega patch separation now done. I can re-roll the pieces whenever needed so it all gets committed. Let me know how I can further help you @xjm.

Cheers!

xjm’s picture

This patch needs to make it to RTBC before anything else. :)

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me, thanks!

chx’s picture

Unbelievable work.

xjm’s picture

catch’s picture

Status: Reviewed & tested by the community » Needs work

OK overall this looks great, except I got to here which looks a bit strange:

+  /**
+   * Overrides SystemQueue::claimItem().
+   *
+   * @param $lease_time
+   *   (optional) How long the processing is expected to take in seconds. After
+   *   this lease expires, the item will be reset and another consumer can
+   *   claim the item. Defaults to 0.
+   */
   public function claimItem($lease_time = 0) {
     $item = db_query_range('SELECT data, item_id FROM {queue} q WHERE name = :name ORDER BY item_id ASC', 0, 1, array(':name' => $this->name))->fetchObject();
     if ($item) {
@@ -29,7 +36,7 @@ class BatchQueue extends SystemQueue {
   }
 
   /**
-   * Retrieve all remaining items in the queue.
+   * Retrieves all remaining items in the queue.
    *
    * This is specific to Batch API and is not part of the DrupalQueueInterface,
    */
@@ -44,10 +51,18 @@ class BatchQueue extends SystemQueue {
 }
 
 /**
- * Batch queue implementation used for non-progressive batches.
+ * Defines a batch queue for non-progressive batches.
  */
 class BatchMemoryQueue extends MemoryQueue {
 
+  /**
+   * Overrides MemoryQueue::claimItem().
+   *
+   * @param $lease_time
+   *   (optional) How long the processing is expected to take in seconds. After
+   *   this lease expires, the item will be reset and another consumer can
+   *   claim the item. Defaults to 0.
+   */

Two things:

- the explanation of the param is identical.

- xjm explained in irc that the default value of the paramater is changed from 30 in the base class to 0 here, however while it mentioned that the default is 0, it doesn't mention that this is what has been changed.

Otherwise looks great! Didn't get all the way to the end of the patch yet though.

xjm’s picture

Note that these two classes are not actually extending the same class. One is extending SystemQueue and the other is extending MemoryQueue. I don't believe the method exists in the base queue... thinger.

What I'll do is:

  1. Suggest the parameter description I wrote above in #1326664: Clean up API docs for system module (excluding subdirectories)
  2. Change the docblock to remove the parameter description and just specify the overridden default. I couldn't find a good example of this so I'm not quite sure the best way to do this. Bikeshed with me:
    /**
      * Overrides MemoryQueue::claimItem().
      *
      * @param $lease_time
      *   (optional) Defaults to 0.  This overrides the default value of 30 provided
      *   by MemoryQueue::claimItem().
      */
catch’s picture

Would be good to explain why we're overriding as well. This is in the @file description so could maybe move from there?

 * - let an item be repeatedly claimed until it is actually deleted (no notion
 *   of lease time or 'expire' date), to allow multipass operations.
jhodgdon’s picture

Is the only difference between the overridden method and the base method the different default for the parameter? If so, I suggest:

---
Overrides MemoryQueue::claimItem().

The only difference between this function and MemoryQueue::claimItem() is that the $lease_time parameter defaults to 0 instead of 30.
---

I think we don't need to include @param here, since the use of the parameter is documented on the base class.

If there are other differences, they can be documented in that sentence instead of "the only difference".

Regarding #72:
I think the bit about what the class as a whole does belongs in the class's docblock, not the claimItem() docblock, if it isn't there already.

jhodgdon’s picture

Here are the standards for class/method docs:
http://drupal.org/node/1354#classes

It doesn't specifically say what to do in this case...

xjm’s picture

And here's the original issue (which @jhodgdon found for me in IRC): #718596: Lacking standards for documenting classes, interfaces, methods

xjm’s picture

Hm, thought about it a bit and I think it's misleading to say "the only difference"--the methods differ functionally. The word "unlike" seems promising, though.

I do think catch has a point about documenting what the differing default means for the use of the method. I'll see what I can come up with.

xjm’s picture

Status: Needs work » Needs review
FileSize
2.34 KB
127.8 KB

Let's start with this.

xjm’s picture

FileSize
478 bytes
127.8 KB
+++ b/core/includes/common.incundefined
@@ -1256,7 +1281,7 @@ function drupal_strip_dangerous_protocols($uri) {
- * Strips dangerous protocols (e.g. 'javascript:') from a URI and encodes it for output to an HTML attribute value.
+ * Strips dangerous protocols from a URI and encodes them for output to HTML.

Just noticed that this should be "encodes it" rather than "encodes them." We're encoding the URI, not dangerous protocols. ;)

xjm’s picture

FileSize
1.57 KB
15.9 KB
126.87 KB

Needs a reroll for #1227942: Rename format_username() now that it's in user.module. format_username() got moved into the user module, so the changes to that function didn't apply anymore. They were in hunks by themselves, so just rerolled without them. (For real this time.) :)

xjm’s picture

FileSize
10.33 KB
126.87 KB
xjm’s picture

The interdiff in #77 has the changes that currently need to be reviewed.

jhodgdon’s picture

Sorry for the delay on reviewing this (large) patch...

I reviewed this patch through bootstrap.inc. Found the following:

a)

+   * time of 0 (no expiration) instead of 30.  This allows the item to be

Two spaces after . Occurs a few times in this file.

Actually that's it... If someone wants to start reviewing at
diff --git a/core/includes/cache-install.inc b/core/includes/cache-install.inc
that would be cool! Or I'll get back to it when I can...

xjm’s picture

Status: Needs review » Needs work

Re: #82: Ah yes, in the sentences I added, visible in the interdiff in #77. :(

The patch after batch.queue.inc is the same as #68, aside from the changes in #78-#79 .

xjm’s picture

FileSize
18.26 KB
126.94 KB

Attached fixes the double spaces after periods in batch.queue.inc, plus one other in archiver.inc. I've also attached a manual diff against the previously RTBC patch in #68 to illustrate what has changed since then.

xjm’s picture

Status: Needs work » Needs review

As the diff above shows, these are the only differences between the patches after batch.queue.inc. The first is a correction, and the other removes changes to a moved function.

2321c2330
< + * Strips dangerous protocols from a URI and encodes them for output to HTML.
---
> + * Strips dangerous protocols from a URI and encodes it for output to HTML.
2516,2541c2525
< @@ -1966,14 +2001,12 @@ function _format_date_callback(array $matches = NULL, $new_langcode = NULL) {
<  }
<  
<  /**
< - * Format a username.
< + * Formats a username.
<   *
<   * By default, the passed-in object's 'name' property is used if it exists, or
<   * else, the site-defined value for the 'anonymous' variable. However, a module
<   * may override this by implementing hook_username_alter(&$name, $account).
<   *
< - * @see hook_username_alter()
< - *
<   * @param $account
<   *   The account object for the user whose name is to be formatted.
<   *
< @@ -1981,6 +2014,8 @@ function _format_date_callback(array $matches = NULL, $new_langcode = NULL) {
<   *   An unsanitized string with the username to display. The code receiving
<   *   this result must ensure that check_plain() is called on it before it is
<   *   printed to the page.
< + *
< + * @see hook_username_alter()
<   */
<  function format_username($account) {
<    $name = !empty($account->name) ? $account->name : variable_get('anonymous', t('Anonymous'));
< @@ -2047,8 +2082,8 @@ function format_username($account) {
jhodgdon’s picture

Status: Needs review » Needs work

I reviewed the rest of this patch, starting with cache-interface.inc. Found a few things to fix:

a)

  *   expire are deleted.
  *
  * @param $bin
- *   If set, the bin $bin to delete from. Mandatory
- *   argument if $cid is set.
+ *   If set, the bin $bin to delete from. Mandatory argument if $cid is set.
  *
  * @param $wildcard
  *   If $wildcard is TRUE, cache IDs starting with $cid are deleted in
- *   addition to the exact cache ID specified by $cid.  If $wildcard is
+ *   addition to the exact cache ID specified by $cid. If $wildcard is
  *   TRUE and $cid is '*' then the entire bin $bin is emptied.

There should not be a blank line between the params (just above $bin is another @param too)... And while you're at it, "bin $bin" should be "cache bin" (without the redundant, in text, $bin). Also the $wildcard one could use a bit of fixing, like it could just say "If TRUE, cache ..." rather than "If $wildcard is TRUE...".

b) @param for $key is missing -- maybe that's OK, but since $item is there, it seems a bit odd:

+ * The tmp_name key is skipped keys since PHP generates single backslashes for
+ * file paths on Windows systems.
+ *
+ * @param $item
+ *   An item from $_FILES.
+ *
+ * @see http://php.net/manual/en/features.file-upload.php#42280
  */
 function _fix_gpc_magic_files(&$item, $key) {

c) A bit of a nitpick, but the verb tenses in the list items could be made parallel:

+ *   Based on the first character of the key, the value is escaped and/or
+ *   themed:
+ *   - !variable: Inserted as is.
+ *   - @variable: Escape plain text to HTML (check_plain).
+ *   - %variable: Escape text and theme as a placeholder for user-submitted
+ *     content (check_plain + drupal_placeholder).

Insert/Escape or Inserted/Escaped... probably the latter, to match the tense of the sentence before the list?

d) List formatting (missing :):

+ *   - 'langcode' (default to the current language) The language code to
+ *     translate to a language other than what is used to display the page.
+ *   - 'context' (default to the empty context) The context the source string
+ *     belongs to.

e)

+ * into a table. The table must have an id attribute set. If using

id -> ID (sorry, not your fault, just a pet peeve of mine...)

f)

+ *   One or more cache granularity constants, e.g. DRUPAL_CACHE_PER_USER to
+ *   cache for each user separately or
+ *   DRUPAL_CACHE_PER_PAGE | DRUPAL_CACHE_PER_ROLE to cache separately for
+ *   each page and role.

Line wrapping is a bit odd here? I don't think the | needs to be all on one line?

Also, punctuation: "constants; e.g., DRUP..."

g) One sentence for one-line function summary:

 /**
- * Get properties of a structured array element. Properties begin with '#'.
+ * Gets properties of a structured array element. Properties begin with '#'.

If you make a new patch, and interdiff between the new patch and #84 would be helpful. Thanks!

xjm’s picture

Hmm, I thought we'd decided earlier that we weren't going to do any list reformatting or other cleanups for rewrapped lines in this patch because it was so big already (and there was another issue targeting the list reformatting).

It will need to be rerolled anyway because of the const/define patch, though. I was hoping to get this in before that one since it could be automatically generated and this can't. Oh well...

Edit:

Line wrapping is a bit odd here? I don't think the | needs to be all on one line?

Ah, I thought it did, because when I wrapped it in the middle of the expression it wasn't clear that it was a bitwise or. Maybe it should be inside @code?

jhodgdon’s picture

RE #87...

List formatting -- I think if there is a lot of list formatting, that would be bad to include, but in these cases you have already done some cleanup on the list formatting (like fixing indenting). It just didn't quite get cleaned up all the way (like adding colons). I would avoid large swaths of list formatting, which would greatly add to the size of the patch, but if you are cleaning up a few lines that is OK.

OK on the odd wrapping. Putting it in @code does make sense.

xjm’s picture

Alright, thankfully rerolling for the const change only required a simple rebase, thanks to git. Adding the fixes from #86 now.

xjm’s picture

Status: Needs work » Needs review
FileSize
4.88 KB
127.31 KB
  • For (b), I added documentation of the $key parameter for both array_walk() callbacks I found.
  • For (c) and (d), I actually just replaced these lines with references to format_string() and t(), rather than duplicating those functions' parameter descriptions. I also added @see to format_string() and t() at the end since those functions also help explain how format_plural() works.
  • For (e) I also corrected another example of this in the same docblock, because having one identifier and one psychoanalytical construct in the same paragraph was even worse than two of the latter. ;) I also fixed the wording of the second sentence a bit, since "as such" didn't really make sense in context.
  • For (f) I reworded the whole parameter description.
Lars Toomre’s picture

I have not re-rolled the list formatting patch based on @xjm's suggestion to wait until A-C and D-G patches are committed. Whatever list changes you correct I will simply take out of the follow up list patch whenever this get ins.

xjm’s picture

Excellent, thanks @Lars Toomre. :)

jhodgdon’s picture

Status: Needs review » Needs work

From the interdiff:

--- a/core/includes/common.inc
+++ b/core/includes/common.inc
@@ -1025,6 +1025,8 @@ function drupal_http_request($url, array $options = array()) {
  *
  * @param $item
  *   An individual string or array of strings from superglobals.
+ * @param $key
+ *   The key for the item within a superglobals array.
  */
 function _fix_gpc_magic(&$item) {

I think the @param $key got added to the wrong function here?

The rest looks good...

xjm’s picture

Status: Needs work » Needs review
FileSize
467 bytes
127.24 KB

Hum, guess I got confused and thought both those functions took the key. Fixed here.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Great! I think we're good to go now.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Thanks! Committed/pushed to 8.x.

Status: Fixed » Closed (fixed)

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

xjm’s picture

Version: 8.x-dev » 7.x-dev
Status: Closed (fixed) » Patch (to be ported)
Issue tags: +Needs backport to D7

Resurrecting for as much D7 backport as is appropriate.

xjm’s picture

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

Here's a backport with the new formatting for menu callbacks, etc. removed. There were also a few methods in cache.inc that don't exist in D7. I diffed the patches manually to show the differences.

xjm’s picture

FileSize
53.47 KB

Err, ignore that last interdiff. That was against the modified patch where I'd already removed the menu callbacks. ;) This is against the original.

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

Ok done with this huge patch...

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 7.x. Thanks!

Status: Fixed » Closed (fixed)

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

xjm’s picture

Assigned: xjm » Unassigned
xjm’s picture

Issue summary: View changes

.