Clean up API docs for system module (excluding subdirectories)

Do you plan to patch this module? If so can you assign the issue to yourself and also add your name to the summary issue? Thanks!

Proposed text for the lease time parameter in SystemQueue::claimItem() and 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.

Here is a patch.

Given how humungous this patch is, I mostly focussed on fixing formal coding standards issues (i.e. I only tried to fix existing language to a very limited extent).

I'm with you on the minimal changes because of the patch size. My first question, though is, why is all the parameter documentation being removed from functions early in the path? They don't seem to fall into any of the categories where we omit documentation for expected parameters or return value docs (form handlers, menu callbacks, overridden methods...)

Sorry, I should have added a note about this:

These are all functions in image.gd.inc, which implement image toolkit operations for GD. I removed them because they duplicate the param/return value docs of the corresponding image_OP() function (which are in core/includes/image.inc). Technically, these are callbacks / hooks, I guess (well, REALLY these are implementations of an interface).

I decided to delete these after doing some research trying to figure out how they work exactly, as I was thinking of improving the param docs a little---and then realized that this should really be done in core/includes/image.inc . All of these have @see directives to the corresponding image_OP()-function, but perhaps we could make this a little more clear ... maybe by adding a line "See image_load() for the documentation of the parameters and return value." or something? We could also Prefix the summary with a "Image toolkit callback: "-prefix, though I am not sure this is terribly helpful. Finally, an @see system_image_toolkits() and/or @see hook_image_toolkits() might be helpful.

What do you think?

In other cases, such as hook implementations, where we are omitting all doc because the params and return values are documented elsewhere, the function where it's documented is in the first line, such as:
- Implements hook_whatever().
- Overrides ClassName::foo().

So I think if we were going to do that here, we should do something similar... like making these:

Image toolkit callback: implements image_resize() for the GD toolkit.

or something like that? And @see hook_image_toolkits() does seem helpful...

But take a look at this:
http://api.drupal.org/api/drupal/modules--system--system.api.php/functio...

It's specifically referring to the image_gd_* functions for documentation of how to create these callbacks. So it looks like we shouldn't remove their documentation at all?

Hm, I am still not a fan of the duplication. But I guess these functions really serve as an ersatz callback documentation.

Attached patch restores the param/return value docs in image.gd.inc. I decided to prefix the summaries with "Image toolkit callback:", but left the rest as it was (Instead of changing it to a "Implements image_resize() for the GD toolkit" format), as these are not really implementations of the image_* functions, but are called by them (though most share there signatures with the calling function). I retained the @see to the corresponding image_* functions and added one to hook_image_toolkit().

This seems like the right course of action for the moment. We should probably make an image.api.php file (or something with a similar name) to document the callbacks as "hooks" (or better yet, figure out a standard way to do callback function documentation), but at the moment this will do. I'll try to make time to review the whole patch later unless someone decides to beat me to it (please). :)

I could break up the patch by file, if that makes it easier?

Nope, one patch per issue. I am just very busy today, sorry!

It's a humungous patch, so I totally understand ...

We had a discussion on #1315992: No standard for documenting menu callbacks and we have (unfortunately) changed the standard for hook_menu() callbacks:
http://drupal.org/node/1354#menu-callback

So this patch will need an update.

The attached patch removes the Path(s):-lines.

I have not added @returns in most cases. Not sure if the current standard REQUIRES those, and in most cases, this would lead to:

/**
 * Page callback: Displays [description].
 *
 * @return
 *   A renderable array representing [same description].
 */

So this seems rather redundant. Let me know if I should introduce the returns nonetheless (I think cases like this could arguably be seen as "short forms").

Yes, the @returns are needed, to distinguish between some page functions that might be returning HTML (which is legal, if not advisable), vs. renderable arrays. Check the discussion on the standards issue for more discussion.

Here is a patch adding in the @returns.

Looking at this patch, I guess the @return do actually add some useful information. It does actually differ from callback to callback.

+++ b/core/modules/system/system.admin.incundefined
@@ -2133,7 +2259,12 @@ function system_add_date_format_type_form_submit($form, &$form_state) {
+ *   A json-formatted string representing the date.

I think JSON should be all in caps here.

Besides #21, I see the following that need addressing in this patch:

a) Boolean should be capitalized:

 + *   A boolean indicating whether the correct version of the GD toolkit is
(and much farther down in the patch)
+ *   If true, only returns a boolean indicating whether there are system status

b)

  * @return
- *   TRUE or FALSE, based on success.
+ *   TRUE or FALSE, based on success

The "." got removed here.

c)

+ * Right-to-left specific stylesheet for the System module administration pages.

Should be Right-to-left-specific

d)

+ * Page callback: Displays a list of administration tasks, organized by module.
+ *
+ *
+ * @return

Too many blank lines here.

e)

+ *   An associative array specifying which modules are considered incompatible.
+ *   The module's name is used as the key, with a value of TRUE. Modules that
+ *   depend (directly or indirectly) to one these modules, and which in turn
+ *   is required by the module represented by the $file parameter, will be
+ *   added to this array.

Grammar cleanup needed here. Should be:
Modules that depend ... *on* one *of* these... *are* required...

f)

+ *   array of 'incopatible' modules, or depends, possibly

spelling: incompatible -- also I think maybe it should be in double quotes "" instead of single quotes ''?

g) Also... I found the new documentation for this function _system_is_compatible to be kind of confusing in general, and I don't think it matches what the function does. But actually this function does not seem to be ever called anywhere in Drupal 8 (except by itself) and I think it should be removed. So maybe we should just leave the doc block alone and remove the function (perhaps in another issue)?

h)

+ * Callback for uasort() in system_admin_index(), system_themes_page() and
+ * system_modules().

Needs , before and.

i)

+ * Form submit handler for system_ip_blocking_delete().

submit -> submission

j)

 /**
- * Submit callback; clear system caches.
+ * Form submission handler for system_performance_settings().
  *
- * @ingroup forms
+ * This submission handler is attached to the 'Clear all caches' button.
  */
 function system_clear_cache_submit($form, &$form_state) {

Since there are several form submission handlers for different buttons here, I think the description of which button should go in the first lines of their functions?

k)

+ * Form constructor for the site image toolkit settings form.
+ *
  *
  * @ingroup forms

Too many blank lines here.

l)

 /**
- * Menu callback: displays the site status report. Can also be used as a pure check.
+ * Page callback: Displays the site status report.
  *
  * @param $check
- *   If true, only returns a boolean whether there are system status errors.
+ *   If true, only returns a boolean indicating whether there are system status
+ *   errors.
+ *
+ * @return
+ *   A string containing HTML markup.
+ *
+ * @see system_menu()
  */
 function system_status($check = FALSE) {

- TRUE needs to be all caps.
- I think the @return should also mention that it could just return TRUE/FALSE instead of HTML markup.
- Boolean needs to be capitalized.

m)

+ * Page callback: Return information about the current PHP interpreter.

Return -> Returns

n)

 /**
- * Delete a configured date format.
+ * Form submission handler for system_date_delete_format_form()
  */

Needs . at end of line. Same here:

+ * Form validation handler for system_configure_date_formats_form()

and here:

+ * Form submission handler for system_configure_date_formats_form()

o)

 /**
- * Perform necessary alterations to the JavaScript before it is presented on
+ * Perform necessary alterations to the JavaScript before it is rendered on
  * the page.

Needs to be shortened to one line.

I ran out of steam and stopped around this spot in the patch (beginning of system.api.php)... sorry!

Alright, I reviewed system.api.php (but not the files after). Few things I noticed:

1. +++ b/core/modules/system/system.api.phpundefined
   @@ -280,7 +279,7 @@ function hook_exit($destination = NULL) {
   - * Perform necessary alterations to the JavaScript before it is presented on
   + * Perform necessary alterations to the JavaScript before it is rendered on
     * the page.
   

   We should probably reword this to get it under 80 chars, correct?

2. +++ b/core/modules/system/system.api.phpundefined
   @@ -296,25 +295,24 @@ function hook_js_alter(&$javascript) {
   + * - title: The human readable name of the library.
   

   While we're changing this line, can we please hyphenate "human-readable"?

3. +++ b/core/modules/system/system.api.phpundefined
   @@ -422,7 +421,7 @@ function hook_css_alter(&$css) {
   -function hook_ajax_render_alter($commands) {
   +function hook_ajax_render_alter(&$commands) {
   

   This change seems out of scope for this issue, no? (I think there might be another issue about these somewhere.)

4. +++ b/core/modules/system/system.api.phpundefined
   @@ -789,12 +790,11 @@ function hook_menu_get_item_alter(&$router_item, $path, $original_map) {
   + * For a detailed usage example, see page_example.module. For comprehensive
   + * documentation on the menu system, see http://drupal.org/node/102338.
   

   Should this use @see?

5. +++ b/core/modules/system/system.api.phpundefined
   @@ -1846,28 +1838,26 @@ function hook_custom_theme() {
   + *     - boolean: 0 (false) or 1 (true).
   

   FALSE and TRUE should be capitalized here, I think.

6. +++ b/core/modules/system/system.api.phpundefined
   @@ -2631,31 +2623,30 @@ function hook_requirements($phase) {
   - * See the Schema API Handbook at http://drupal.org/node/146843 for
   - * details on schema definition structures.
   + * See the Schema API Handbook at http://drupal.org/node/146843 for details on
   + * schema definition structures.
   

   Should we use an @link here?

7. +++ b/core/modules/system/system.api.phpundefined
   @@ -2737,8 +2728,8 @@ function hook_schema_alter(&$schema) {
   + * Structured (aka dynamic) queries that have tags associated may be altered by
   + * any module before the query is executed.
   

   Let's add periods for a.k.a. ...Actually, we could probably just remove "aka" entirely.

8. +++ b/core/modules/system/system.api.phpundefined
   @@ -2821,8 +2812,8 @@ function hook_query_TAG_alter(QueryAlterableInterface $query) {
   + * Please be sure that anything added or modified in this function that can be
   + * removed during uninstall should be removed with hook_uninstall().
   

   We should remove the word "Please" here.

I mostly just scanned all the rewrapped lines of text, so it's possible I could have missed any typos in those.

And, the rest of the files. (Again, I just scanned the longer blocks of rewrapping, so we might want to proofread those again later.)

1. +++ b/core/modules/system/system.mail.incundefined
   @@ -10,7 +10,7 @@
   -   * Concatenate and wrap the e-mail body for plain-text mails.
   +   * Concatenates and wraps the e-mail body for plain-text mails.
   

   The word "mails" is a little weird.

2. +++ b/core/modules/system/system.mail.incundefined
   @@ -29,15 +29,16 @@ class DefaultMailSystem implements MailSystemInterface {
   +   * Sends an e-mail message, using Drupal variables and default settings.
   

   Should the comma here be removed?

3. +++ b/core/modules/system/system.moduleundefined
   @@ -1078,7 +1078,12 @@ function system_menu() {
   + * Theme callback: Return the them for the default batch page.
   

   Typo: "the them."

4. +++ b/core/modules/system/system.moduleundefined
   @@ -2796,7 +2810,7 @@ function system_settings_form($form) {
   - * Execute the system_settings_form.
   + * Form submission hanlder for system_settings_form().
   

   Typo: "hanlder."

5. +++ b/core/modules/system/system.moduleundefined
   @@ -3305,7 +3325,11 @@ function system_message_action(&$entity, $context = array()) {
   + * Form constructor for the system_goto action configuration form.
   

   I read this a few times... so it's a configuration form for the action named 'system_goto'? Should we put that in single quotes for clarity, or is there another name to use?

6. +++ b/core/modules/system/system.moduleundefined
   @@ -3355,10 +3382,14 @@ function system_block_ip_action() {
   + * Generates an array of time zones and their local time and date.
   

   I think "times and dates" should be pluralized, right? (Because they differ from timezone to timezone.)

7. +++ b/core/modules/system/system.moduleundefined
   @@ -3452,25 +3485,22 @@ function system_image_toolkits() {
   + *   (optional) Replace behavior when the destination file already exists:
     *   - FILE_EXISTS_REPLACE: Replace the existing file.
     *   - FILE_EXISTS_RENAME: Append _{incrementing number} until the filename is
     *     unique.
     *   - FILE_EXISTS_ERROR: Do nothing and return FALSE.
   + *   Defaults to FILE_EXISTS_RENAME.
   

   This confused me at first. Maybe we could change it to:
   ...when the destination file already exists. Allowed values:

   Would that be more clear?

8. +++ b/core/modules/system/system.moduleundefined
   @@ -3583,7 +3613,7 @@ function system_date_formats() {
   - * Gets the list of defined date formats and attributes.
   + * Retrievess the list of defined date formats and attributes.
   

   Typo: "Retrievess."

9. +++ b/core/modules/system/system.queue.incundefined
   @@ -106,10 +106,11 @@ interface DrupalQueueInterface {
   -   * Add a queue item and store it directly to the queue.
   +   * Adds a queue item and store it directly to the queue.
   

   Should be "stores it directly to the queue.

10. +++ b/core/modules/system/system.testundefined
    @@ -19,7 +19,7 @@ class ModuleTestCase extends DrupalWebTestCase {
    +   * Assert sthere are tables that begin with the specified base table name.
    

    Typo: "Assert sthere."

11. +++ b/core/modules/system/system.testundefined
    @@ -141,7 +142,7 @@ class EnableDisableTestCase extends ModuleTestCase {
    -   * Test that all core modules can be enabled, disabled and uninstalled.
    +   * Tests that all core modules can be enabled, disabled and uninstalled.
    

    There should be a comma after "disabled" here.

12. +++ b/core/modules/system/system.testundefined
    @@ -357,7 +358,7 @@ class ModuleDependencyTestCase extends ModuleTestCase {
    -   * Attempt to enable translation module without locale enabled.
    +   * Attempts to enable translation module without locale enabled.
    

    Maybe "Attempts to enable the Translation module"?

13. +++ b/core/modules/system/system.testundefined
    @@ -424,7 +425,7 @@ class ModuleDependencyTestCase extends ModuleTestCase {
    -   * Tests enabling a module that depends on a module with an incompatible core version.
    +   * Tests enabling modules depending on modules incompatible with core.
    

    Hmm, I can see that you're rewording this to get it under 80 characters, but "modules incompatible with core" is WTF-y. Maybe "Tests module dependency chain with an incompatible core version." or something? Not sure exactly. (The summaries for all three of these tests are a little awkward, but this one in particular seems incorrect.)

14. +++ b/core/modules/system/system.testundefined
    @@ -1202,7 +1188,7 @@ class DateTimeFunctionalTest extends DrupalWebTestCase {
    -   * Test if the date formats are stored properly.
    +   * Tests if the date formats are stored properly.
    

    I think it should be "Tests whether..."

15. +++ b/core/modules/system/system.testundefined
    @@ -1301,7 +1281,7 @@ class PageTitleFiltering extends DrupalWebTestCase {
    -   * Reset page title.
    +   * Resets page title.
    

    Probably should be "Resets the page title."

16. +++ b/core/modules/system/system.testundefined
    @@ -1311,7 +1291,7 @@ class PageTitleFiltering extends DrupalWebTestCase {
    -   * Tests the handling of HTML by drupal_set_title() and drupal_get_title()
    +   * Tests the handling of HTML by drupal_set_title() and drupal_get_title().
    

    Maybe "Tests HTML handling in..."?

17. +++ b/core/modules/system/system.testundefined
    @@ -1338,7 +1318,7 @@ class PageTitleFiltering extends DrupalWebTestCase {
    -   * Test if the title of the site is XSS proof.
    +   * Tests whether the title of the site is XSS proof.
    

    Maybe "XSS-proof" should be hyphenated? (I'd also quibble with the idea of something being XSS-"proof"... maybe it would be better to say "Tests XSS protection for the site title" or something.)

18. +++ b/core/modules/system/system.testundefined
    @@ -1453,7 +1433,7 @@ class SystemBlockTestCase extends DrupalWebTestCase {
    -   * Test displaying and hiding the powered-by and help blocks.
    +   * Tests displaying and hiding the powered-by and help blocks.
    

    I don't think "powered-by" should be hyphenated, and I think the names should probably be capitalized and/or quoted. Maybe:
    Tests displaying and hiding the "Powered by" and "Help" blocks.

19. +++ b/core/modules/system/system.testundefined
    @@ -1500,7 +1480,7 @@ class SystemBlockTestCase extends DrupalWebTestCase {
    - * Test main content rendering fallback provided by system module.
    + * Tests main content rendering fallback provided by system module.
    

    How about:
    Tests the main content rendering fallback provided by the System module.

20. +++ b/core/modules/system/system.testundefined
    @@ -1532,7 +1512,7 @@ class SystemMainContentFallback extends DrupalWebTestCase {
    -   * Test availability of main content.
    +   * Tests availability of main content.
    

    Should probably be "Tests the availability of the main content [something-or-other]."

21. +++ b/core/modules/system/system.testundefined
    @@ -2300,7 +2280,7 @@ class RetrieveFileTestCase extends DrupalWebTestCase {
    - * Functional tests shutdown functions.
    + * Functional tests for shutdown functions.
    

    Needs a verb.

Phew! This patch is a monster. Thanks @sven.lauer!

Oh, one other thing. I didn't see the parameter documentation from #2 in there--should we add that here, or open a separate issue? I don't want to force rerolls on this patch if we put it in a separate issue, so if we go that route, maybe we should postpone it.

I noticed some oddities with hook_element_info, and I wanted to add them in here, but I ended up rerolling and trying to incorporate #21-24. Hope you don't mind, sven.lauer!

#23 3) I think this change is fine for this issue. I couldn't find another open one referring to it either.
#23 4) Not sure.
#23 6) Not sure.
#24 18) powered-by is the block delta, and is used in a lot of places. Not sure about changing this here.

Part of the changes to this patch were moved to includes/theme.inc by #761608: Missing theme settings values because list_themes() has inconsistent theme object data, I kept them in here for now.

BOTIFY!

+++ b/core/modules/system/system.moduleundefined
@@ -3339,7 +3339,7 @@ function system_block_ip_action() {
- * Generates an array of time zones and their local time and date.
+ * Generates an array of time zones and their local times and dates.

Oops, I meant to say this above, but I think "timezones" is one word.

Other than that, all the changes in the interdiff look correct to me.

+++ b/core/modules/system/system.api.phpundefined
@@ -210,22 +209,25 @@ function hook_cron_queue_info_alter(&$queues) {
+ *   - #title_display: optional string indicating if and how #title should be
+ *     displayed, see theme_form_element() and theme_form_element_label().

Oops, one more thing. This should probably be:
- #title_display: (optional) String indicating...

The full patch in #26 probably should be reviewed one more time. Edit: or make that in #32!

Rerolled, also fixed some more s/boolean/Boolean.
Left in the ones that should become bool when we do the data types stuff.

Thanks, tim.plunkett for picking up my slack!

(Yes, it is one beast of a patch, but I still feel kind of bad for overlooking so many things ...)

#32: drupal-1326664-32.patch queued for re-testing.

#32: drupal-1326664-32.patch queued for re-testing.

Looks like this needs a reroll.

Conflicted with http://drupalcode.org/project/drupal.git/commit/c1c2ceab and http://drupalcode.org/project/drupal.git/commit/982087f2.

Well, I got through hook_mail_alter() on the re-review and didn't find anything wrong.

I started after hook_mail_alter() and found the following:

a) (system.api.php: hook_xmlrpc)

+ *     - int: An integer number (for example,  -12).

There are two spaces between , and -12

b) (system.api.php: hook_watchdog)

+ *     is normally the module name. Do not use 'debug', use severity
+ *     WATCHDOG_DEBUG instead.

This , should be a ; (comma splice).

c) (system.api.php: hook_watchdog)

+ *     - WATCHDOG_EMERGENCY: System is unusable
+ *     - WATCHDOG_ALERT: Action must be taken immediately
+ *     - WATCHDOG_CRITICAL: Critical conditions
+ *     - WATCHDOG_ERROR: Error conditions
+ *     - WATCHDOG_WARNING: Warning conditions
+ *     - WATCHDOG_NOTICE: Normal but significant condition
+ *     - WATCHDOG_INFO: Informational messages
+ *     - WATCHDOG_DEBUG: Debug-level messages

- All these list items should end in .
- It would be nice to be consistent and make them all say condition/message instead of conditions/messages.

d) (system.api.php: hook_mail)

 /**
- * Prepare a message based on parameters; called from drupal_mail().
+ * Prepare a message based on parameters. Called from drupal_mail().

This is two sentences instead of one in the first line description. Either move the second to a new paragraph or combine them into one sentence.

e) (same)

+ *   - id: An ID to identify the mail sent. Look at module source code or
+ *     drupal_mail() for possible id values.

id -> ID in the second line.

f) hook_stream_wrappers()

/**
- * Registers PHP stream wrapper implementations associated with a module.
+ * Register a PHP stream wrapper implementations.

a ... implementations -- is it registering one or many? I think it's many, and therefore "a" should be removed.

g) hook_requirements()

+ * During the 'install' phase, modules can for example assert that ibrary or
+ * server versions are available or sufficient. Note that the installation of a

Typo: ibrary -> library

h) hook_schema()

+ * tables and their related keys and indexes. A schema is defined by
  * hook_schema() which must live in your module's .install file.

needs comma before which.

i)

  * @link http://drupal.org/node/180528 http://drupal.org/node/180528 @endlink

This does not need to be a @link (no link text). Please remove the @link and just put the URL on the previous line.

j) hook_uninstall()

  * The information that the module should remove includes:
- * - variables that the module has set using variable_set() or system_settings_form()
+ * - variables that the module has set using variable_set() or
+ *   system_settings_form()
  * - modifications to existing tables

List items need to end in .

k)

+ * Batch API array, as appropriate. See below for morei nformation on the 'type'

morei nformation --> typo

l) hook_action_info()

  *
  * An action consists of two or three parts:
- * - an action definition (returned by this hook)
- * - a function which performs the action (which by convention is named
+ * - An action definition (returned by this hook)
+ * - A function which performs the action (which by convention is named
  *   MODULE_description-of-function_action)
- * - an optional form definition function that defines a configuration form
+ * - An optional form definition function that defines a configuration form
  *   (which has the name of the action function with '_form' appended to it.)

List items need to end in . [in the last one, the . should be outside the )]

m)

+ * files into a single possibly compressed file.  Common examples of such files

Two spaces after . instead of one.

n) system.js:

+ * When a field is filled out, this function applies its value to other fields
+ * that will likely
  * use the same value. In the installer this is used to populate the
  * administrator e-mail address with the same value as the site e-mail address.

Needs paragraph wrapping.

I stopped at the top of system.module in the patch.

I also noticed these things in the top part of the patch:
a) system.admin.inc

+ * Form constructor for the administration theme selction form.

selction (typo)

And continuing with the review, in system.module:

b)

 /**
- * Refresh bootstrap column in the system table.
+ * Refreshes bootstrap column in the system table.

Needs "the" as second word.

c)

+ *   Allowed values:
  *   - FILE_EXISTS_REPLACE: Replace the existing file.
  *   - FILE_EXISTS_RENAME: Append _{incrementing number} until the filename is
  *     unique.
  *   - FILE_EXISTS_ERROR: Do nothing and return FALSE.
+ *   Defaults to FILE_EXISTS_RENAME.

The default value could be indicated more compactly by putting (default) on the default item in the list.

d) system.queue.inc

  /**
-   * Release an item that the worker could not process, so another
-   * worker can come in and process it before the timeout expires.
+   * Releases an item that the worker could not process, so another worker can
+   * come in and process it before the timeout expires.

Needs one-line description.

e) system.updater.inc

  /**
-   * Return available database schema updates one a new version is installed.
+   * Returns available database schema updates one a new version is installed.

This makes no sense to me... "one a new version"??? Maybe it should be "when"?

f) theme.api.php

+ * includes the template and stores the output. Drupal's theme engines can
+ * provide alternate template engines, such as XTemplate, Smarty and PHPTal. The
+ * most common template engine is PHPTemplate (included with Drupal and
+ * implemented in phptemplate.engine, which uses Drupal's default template
+ * renderer.

Parenthesis opened but never closed.

Phew, I think that's it!

When #1471376: Convert updater.inc to PSR-0 was committed, most of the changes from this patch were incorporated. This only catches a few extraneous changes from there, which is why the patch is smaller.

Incorporated the changes from #41.

Cross post! Rerolled again.

Wow, so apparently I went into a trance there yesterday when I tried to review this. ;)

Rerolling after #1373142: Use the Testing profile. Speed up testbot by 50%.

Wait... This is supposed to be system.module. How come the first several hunks in the patch are not part of modules/system?

Those hunks were in system.module when this patch started. :)
In my rerolls, git moved them to where ever the code went.

It would be trivial to strip those out, but harder to track down if I stopped including them in rerolls.

The 4 no-longer-in-system-module hunks were moved out by these issues:

#761608: Missing theme settings values because list_themes() has inconsistent theme object data
#1468244: Convert DrupalQueue system to PSR-0
#1471368: Convert uuid.inc to PSR-0
#1468244: Convert DrupalQueue system to PSR-0

They can each be moved there as follow-ups, or kept in here.

Ah, OK, seems sensible to get them in here. If no one beats me to it, I'll give this a review tomorrow, and then hopefully commit it. But if we're over thresholds again, I'll probably have to hold off on committing it, as it is intersecting with a lot of patches (obviously).

Rerolled around #1471364: Move mail system classes from system.mail.inc to PSR-0 classes in Drupal\Core.

Top part of the patch nitpick:

+++ b/core/lib/Drupal/Core/Mail/PhpMail.php
@@ -13,7 +13,7 @@ namespace Drupal\Core\Mail;
   /**
-   * Concatenates and wrap the e-mail body for plain-text mails.
+   * Concatenates and wraps the e-mail body for plain-text mails.

Technically, "mail" is not a countable noun, so can't be pluralized. Should say "e-mail" not "mails".

I wouldn't have mentioned it, but then I went on and did a careful review of the system.admin.inc part of this patch. It needs a bit of work:

b)

I don't think we need @see to itself here. :) Maybe it was supposed to link to theme_system_admin_index()? But even that is probably silly, since there's a theme call below (and on the latest and greatest API module code, those will turn into links).

Those other @see to the theme functions can probably be removed too. I think in general that it's rather pointless to add @see to functions that are called within a function. It should be used for things that are less obvious. I also don't know that we need the back references (e.g. from theme_system_admin_index() back to system_admin_index()). In a few days at least, api.drupal.org will be making those links for us. Just need to deploy the new code... but those are less obvious and can probably stay.

c)
<code>
@@ -717,18 +755,26 @@ function system_theme_settings_submit($form, &$form_state) {

+ *   An associative array specifying which modules are considered incompatible.
+ *   The module's name is used as the key, with a value of TRUE. Modules that
+ *   depend (directly or indirectly) on one of these are modules, and which in
+ *   turn is required by the module represented by the $file parameter, will be
+ *   added to this array.

The last sentence here makes zero sense to me as it is written. There are several things that need rewording.

d)

@@ -1350,11 +1429,18 @@ function system_modules_uninstall_submit($form, &$form_state) {
 }
 
 /**
- * Menu callback. Display blocked IP addresses.
+ * Page callback: Displays blocked IP addresses.
+ *
+ * This page callback displays system_ip_blocking_form().
  *

Similar to (a), I don't think we need that line telling us that system_ip_blocking_form() is displayed. That kind of technical information is immediately obvious from the code.

e)

@@ -2990,16 +3179,10 @@ function system_actions_manage() {
 }
 
 /**
- * Define the form for the actions overview page.
- *
- * @param $form_state
- *   An associative array containing the current state of the form; not used.
- * @param $options
- *   An array of configurable actions.
- * @return
- *   Form definition.
+ * Form constructor for the actions overview page.
  *
  * @ingroup forms
+ * @see system_actions_manage()
  * @see system_actions_manage_form_submit()
  */
 function system_actions_manage_form($form, &$form_state, $options = array()) {

The $options parameter should not have been removed from this doc block.

f)

@ -3035,22 +3216,21 @@ function system_actions_manage_form_submit($form, &$form_state) {
 }
 
 /**
- * Menu callback; Creates the form for configuration of a single action.
+ * Form constructor for the action configuration form.
  *
- * We provide the "Description" field. The rest of the form is provided by the
- * action. We then provide the Save button. Because we are combining unknown
- * form elements with the action configuration form, we use an 'actions_' prefix
- * on our elements.
+ * This constructor provides the "Description" field and the save button. The
+ * rest of the form is provided by the action in the ACTION_FUNCTION_form()
+ * callback (see hook_action_info() for details).
  *
  * @param $action
- *   Hash of an action ID or an integer. If it is a hash, we are
- *   creating a new instance. If it is an integer, we are editing an existing
- *   instance.
- * @return
- *   A form definition.
+ *   (optional) Hash of an action ID or an integer. If it is a hash, we are
+ *   creating a new instance of the given action. If it is an integer, we are
+ *   editing an existing instance.
  *
+ * @see system_menu()
  * @see system_actions_configure_validate()
  * @see system_actions_configure_submit()
+ * @ingroup forms
  */
 function system_actions_configure($form, &$form_state, $action = NULL) {

I would not really say here the $action is optional. If it's missing, this function just redirects back to another page instead of constructing the form. So let's leave out the (optional).

I ran out of steam here. The next section to review starts at system.api.php.

Continuing on... In system.api.php:
g)

@@ -1427,21 +1430,22 @@ function hook_init() {
...
+ *   - available: A Boolean value to indicate that the toolkit is operating
+ *     properly, e.g. all required libraries exist.

This should not be e.g.. E.g. means "for example". If you want to change it to either "i.e." or "that is", the punctuation is wrong, should be:
....properly; that is, all required...

h)

@@ -1461,45 +1465,37 @@ function hook_image_toolkits() {
...

+ *  - id: The drupal_mail() id of the message. Look at module source code or
+ *    drupal_mail() for possible id values.

id is a psychological term. ID means identifier. Two misuses here.

i)

@@ -1817,15 +1812,15 @@ function hook_theme_registry_alter(&$theme_registry) {
...

+ * Since only one theme can be used at a time, the last (i.e., highest weighted)
+ * module which returns a valid theme name from this hook will
  * prevail.

Wrapping needs attention here.

j)

@@ -1847,28 +1842,26 @@ function hook_custom_theme() {

...
+ *   - The method signature is an array of XML-RPC types. The first element of
+ *     this array is the type of return value and then you should write a list
+ *     of the types of the parameters. XML-RPC types are the following (See the
+ *     types at http://www.xmlrpc.com/spec):

Can we clean this up a bit?
- "and then you should write" -- eek! How about, "The first element is...; remaining elements are the types of ..."
- (See... => see should not be capitalized... actually how about just (see http://whatever) without "the types at"?

k)

@@ -1919,30 +1912,35 @@ function hook_xmlrpc_alter(&$methods) {
 }
 
 /**
- * Log an event message
+ * Respond the logging of an event message.

Respond the --> Respond to the

l)

@@ -2541,25 +2540,23 @@ function hook_file_url_alter(&$uri) {

...
+ * server versions are available or sufficient. Note that the installation of a
+ * module can happen during installation of Drupal itself (by install.php) with
+ * an installation profile or later by hand. As a consequence, install-time
...
+ * it is not available during install.php. For localization you should for
+ * example use $t = get_t() to retrieve the appropriate localization function
...
+ * severity levels have no effect on the installation. Module dependencies do
+ * not belong to these installation requirements, but should be defined in the

- or needs a comma before it [first section]
- for example needs to have commas before and after it [second section]
- not belong to -> not belong in [third section

What remains to be reviewed again starts with system.archiver.inc

Moving on... in system.js:

m)

@@ -47,11 +50,10 @@ Drupal.behaviors.cleanURLsSettingsCheck = {
 };
 
 /**
- * Internal function to check using Ajax if clean URLs can be enabled on the
- * install page.
+ * Check if clean URLs can be enabled on the install page, using Ajax.

Should start with Checks.

n)

+++ b/core/modules/system/system.maintenance.css
@@ -1,5 +1,11 @@
 
 /**
+ * @file
+ * Stylesheet for maintenance mode.
+ */
+
+

I don't think we need so many blank lines here?

Phew! I got through the file finally... Review is done.. Anyone for a fix-up job? :)

Perhaps it's not the best time to re-roll this, as we are over multiple issue thresholds, but I had the time, so I quickly did this. Attached patch is a re-roll of #51, addressing the comments in #52 to #54.
The patch no longer contains the changes for the Archiver classes, as these were fixed in the process as moving these classes to core/lib as part of the PSR move.

Comments:

b) I don't think we need @see to itself here. :) Maybe it was supposed to link to theme_system_admin_index()?

Duh! That is probably what was intended, yes.

But even that is probably silly, since there's a theme call below (and on the latest and greatest API module code, those will turn into links).

Those other @see to the theme functions can probably be removed too. I think in general that it's rather pointless to add @see to functions that are called within a function.

I removed the @sees to the theme functions where theme() is called in the function body (I had left/put those in originally because I did not know that auto-linking theme functions was on the horizon. I have not touched the back-references (from theme function to invoking function) for now.

c)
[...]
The last sentence here makes zero sense to me as it is written. There are several things that need rewording.

Arg. I wrote this after spending an extended time trying to figure out what the function does with its parameters, and why. I must have been quite exhausted. Looking at the doc of this function globally, I realized that the @return actually is quite clear and concise about what happens to the $incompatible parameter, except for what happens to it on recursive calls. So I added a comment trying to explain this, and removed the confusing sentence from the param doc.

(Really, the name of this function should be changed---the function does not do anything related to incompatibility, per se: It determines whether a given module depends, directly or indirectly on one of a set of given other modules.)

g)

+ * - available: A Boolean value to indicate that the toolkit is operating
+ * properly, e.g. all required libraries exist.

This should not be e.g.. E.g. means "for example". If you want to change it to either "i.e." or "that is", the punctuation is wrong, should be:
....properly; that is, all required...

I don't think that "i.e." / "that is" is what was intended here---rather, what the function returns in this array element is a boolean indicating whether the toolkit is operational, and an example of what an implementation might check is whether all required libraries exist, but there presumably could be others. I split this into two sentences and reworded things to make this more clear.

P. S. The interdiff is missing the fix for i), which is in the patch, as the interdiff was made against an intermediate re-roll of #51 just to make it apply, and I had fixed the wrapping issue while making the patch apply.

I reviewed the interdiff and all the changes there look good. The new text for _system_is_incompatible() is much clearer. I did notice this one thing:

+++ b/core/modules/system/system.api.php
@@ -1477,8 +1478,8 @@ function hook_image_toolkits() {
+ *  - ID: The drupal_mail() id of the message. Look at module source code or
+ *    drupal_mail() for possible ID values.

Looks like we missed an "id" here.

I haven't read the full patch yet.

Alright, I reviewed again from system.api.php through the end. Note that some of my notes about rewrapping below might be mistaken; I was eyeballing it, so check before rewrapping something. :)

1. +++ b/core/modules/system/system.api.php
   @@ -600,28 +602,28 @@ function hook_menu_get_item_alter(&$router_item, $path, $original_map) {
   + * mymodule_abc_load() will be invoked with the argument '123', and should load
   + * and return an "abc" object with internal id 123:
   

   Another id.

2. +++ b/core/modules/system/system.api.php
   @@ -1427,21 +1430,23 @@ function hook_init() {
   + * The toolkit's functions must be named image_toolkitname_OPERATION().
   + * where OPERATION may be:
   

   I think the period should be replaced with a comma and the paragraph should be rewrapped.

3. +++ b/core/modules/system/system.api.php
   @@ -1461,45 +1466,37 @@ function hook_image_toolkits() {
   + * hook. All core modules use drupal_mail() for messaging, it is best practice
   + * but not mandatory in contributed modules.
   

   There should be a conjunction or semicolon after "messaging".

4. +++ b/core/modules/system/system.api.php
   @@ -1655,19 +1652,19 @@ function hook_permission() {
   + *   - theme_engine: A theme engine is being checked for the actual theme
     *     being used.
   

   I think this could be rewrapped.

5. +++ b/core/modules/system/system.api.php
   @@ -1730,15 +1726,15 @@ function hook_permission() {
   + *   - override preprocess functions: Set to TRUE when a theme does NOT want the
   + *     standard preprocess functions to run. This can be used to give a theme
   + *     FULL control over how variables are set. For example, if a theme wants
   + *     total control over how certain variables in the page.tpl.php are set,
   + *     this can be set to true. Please keep in mind that when this is used by a
   + *     theme, that theme becomes responsible for making sure necessary variables
   + *     are set.
   

   I think we can remove the opening "Set to..." and te word "please" here.

6. +++ b/core/modules/system/system.api.php
   @@ -1851,28 +1846,26 @@ function hook_custom_theme() {
   + *     - boolean: 0 (false) or 1 (true).
   

   I think FALSE and TRUE should be capitalized here.

7. +++ b/core/modules/system/system.api.php
   @@ -1923,30 +1916,35 @@ function hook_xmlrpc_alter(&$methods) {
   + *   - severity: One of the following values as defined in RFC 3164
   + *     (http://www.faqs.org/rfcs/rfc3164.html):
   

   We could use @link here.

8. +++ b/core/modules/system/system.api.php
   @@ -2638,31 +2635,30 @@ function hook_requirements($phase) {
   + * See the Schema API Handbook at http://drupal.org/node/146843 for details on
   + * schema definition structures.
   

   We could link the words Schema API Handbook here using @link.

9. +++ b/core/modules/system/system.api.php
   @@ -2721,10 +2717,10 @@ function hook_schema() {
   + * When a module modifies the database structure of another module (by changing,
   + * adding or removing fields, keys or indexes), it should implement
   
   @@ -3524,21 +3511,21 @@ function hook_archiver_info_alter(&$info) {
   + * recommended that each date type starts with the module name. A date type can
   + * consist of letters, numbers and underscores.
   

   I think we prefer the final serial commas here (after "adding", "keys", "numbers", etc.).

10. +++ b/core/modules/system/system.api.php
    @@ -2897,24 +2893,24 @@ function hook_install() {
    + * See the batch operations page for more information on how to use the batch
    + * API: http://drupal.org/node/180528
    

    Or, we could use the @link on the text "batch operations page"?

11. +++ b/core/modules/system/system.api.php
    @@ -3197,53 +3190,48 @@ function hook_registry_files_alter(&$files, $modules) {
    + *     - form: This indicates that the task function will return a standard
    + *       Form API definition (and separately define validation and submit
    ...
    + *     - INSTALL_TASK_RUN_IF_NOT_COMPLETED: This indicates that the task will
      *       run once during the installation of the profile. This is the default
    ...
    - *       - INSTALL_TASK_SKIP: This indicates that the task will not run during
    + *     - INSTALL_TASK_SKIP: This indicates that the task will not run during
    ...
    - *       - INSTALL_TASK_RUN_IF_REACHED: This indicates that the task will run
    + *     - INSTALL_TASK_RUN_IF_REACHED: This indicates that the task will run
    

    I think all these paragraphs need rewrapping?

12. +++ b/core/modules/system/system.api.php
    @@ -3408,20 +3395,20 @@ function hook_file_mimetype_mapping_alter(&$mapping) {
    + *   - behavior: (optional) A machine-readable array of behaviors of this
      *     action, used to signal additionally required actions that may need to be
    

    Needs rewrapping I think?

13. +++ b/core/modules/system/system.api.php
    @@ -3596,28 +3585,27 @@ function hook_date_format_types_alter(&$types) {
    + *   - locales: (optional) An array of 2 and 5 character locale codes, defining
    

    Really minor, but I think this should be "2- and 5-character locale codes"? Also, the comma before "defining" seems wrong to me.

14. +++ b/core/modules/system/system.api.php
    @@ -4052,17 +4040,17 @@ function hook_batch_alter(&$batch) {
      *   An associative array of information about the updater(s) being provided.
    ...
      *   - name: Human-readable name of this updater.
      *   - weight: Controls what order the Updater classes are consulted to decide
    ...
    + *     decide if each Updater wants to handle the task. In general, this doesn't
    + *     matter, but if you need to override an existing Updater, make sure your
    + *     Updater has a lighter weight so that it comes first.
    
    @@ -4090,8 +4078,8 @@ function hook_updater_info() {
    + *   Associative array of updaters as defined through hook_updater_info(). Alter
    

    We could probably spin this off into a followup issue since we're not already changing these lines, but the capitalization for "Updater" is really inconsistent. If it weren't for "Updater classes" I'd say it should all just be lowercase, but I'm not sure.

15. +++ b/core/modules/system/system.api.php
    @@ -4131,8 +4119,8 @@ function hook_countries_alter(&$countries) {
    + *   Contains the system path that is going to be loaded. This is read only, use
    + *   hook_url_inbound_alter() to change the path.
    

    "Read-only" should be hyphenated; and the comma should be replaced with either a semicolon or a period + new sentence.

16. +++ b/core/modules/system/system.api.php
    @@ -4146,29 +4134,29 @@ function hook_menu_site_status_alter(&$menu_site_status, $path) {
      * connection types will be available to site administrators using the Update
    - * manager when they are redirected to the authorize.php script to authorize
    - * the file operations.
    + * manager when they are redirected to the authorize.php script to authorize the
    + * file operations.
    

    I think per the new capitalization policy "Update Manager" should be Proper Noun Case? Reference: http://drupal.org/node/604342#capitalization

17. +++ b/core/modules/system/system.api.php
    @@ -4146,29 +4134,29 @@ function hook_menu_site_status_alter(&$menu_site_status, $path) {
    + *   FileTransfer type (not human-readable, used for form elements and variable
    + *   names, etc), and the values are subarrays that define properties of that
    

    "etc." needs a period.

18. +++ b/core/modules/system/system.api.php
    @@ -4146,29 +4134,29 @@ function hook_menu_site_status_alter(&$menu_site_status, $path) {
    + *   - file path: (optional) The directory (relative to the Drupal root)
      *     where the include file lives. If not defined, defaults to the base
    ...
    + *   - weight: (optional) Integer weight used for sorting connection types on
      *     the authorize.php form.
    

    I think the wrapping is not quite right here.

19. +++ b/core/modules/system/system.js
    @@ -1,8 +1,12 @@
    - * Show/hide the 'Email site administrator when updates are available' checkbox
    - * on the install page.
    + * Shows/hides the 'Email site administrator ...' checkbox on the install page.
    
    @@ -92,7 +97,7 @@ Drupal.behaviors.copyFieldValue = {
    - * Show/hide custom format sections on the regional settings page.
    + * Shows/hides custom format sections on the regional settings page.
    

    I think "Shows and hides" would be better?

20. +++ b/core/modules/system/system.js
    @@ -69,9 +71,12 @@ Drupal.cleanURLsInstallCheck = function () {
    + * Copies the value of a field to other fields, after it is filled out.
    

    The comma here seems incorrect to me.

21. +++ b/core/modules/system/system.js
    @@ -117,8 +122,10 @@ Drupal.behaviors.dateTime = {
    - * Show/hide settings for page caching depending on whether page caching is
    - * enabled or not.
    + * Shows/hides settings for page caching.
    + *
    + * Toggles visibility of page caching settings, depending on whether page
    + * caching is enabled or not.
    

    This is under 80 characters:

     * Shows or hides settings for page caching depending on whether it is enabled.
    

    What do you think? Otherwise, I'd at least recommend replacing the slash with "and" and updating the wrapping for the second paragraph.

22. +++ b/core/modules/system/system.module
    @@ -1747,27 +1752,27 @@ function _system_themes_access($theme) {
    + * instantiate the appropriate FileTransfer object, and then invoke the desired
    + * operation passing in that object. The authorize.php script can act as a Batch
    + * API processing page, if the operation requires a batch.
    

    Is the operation passing in the object? Otherwise there should be a comma after "operation."

23. +++ b/core/modules/system/system.module
    @@ -2023,7 +2027,10 @@ function system_user_login(&$edit, $account) {
    - * Add the time zone field to the user edit and register forms.
    + * Adds the time zone field to the user edit and register forms.
    

    I think "timezone" needs to be one word here as well.

24. +++ b/core/modules/system/system.module
    @@ -2518,7 +2528,7 @@ function _system_update_bootstrap_status() {
    - * Helper function to scan and collect theme .info data and their engines.
    + * Scans and collects theme .info data and their engines.
    

    The word "their" is a little weird here--perhaps:
    Scans and collects theme .info data and engine information.

25. +++ b/core/modules/system/system.test
    @@ -129,7 +130,7 @@ class ModuleTestCase extends DrupalWebTestCase {
    - * Test module enabling/disabling functionality.
    + * Tests module enabling/disabling functionality.
    

    This is kinda awkward. "Tests functionality for enabling and disabling modules," perhaps?

26. +++ b/core/modules/system/system.test
    @@ -371,7 +372,7 @@ class ModuleDependencyTestCase extends ModuleTestCase {
    -   * Attempt to enable translation module without locale enabled.
    +   * Attempts to enable the Translation module without locale enabled.
    

    I think "Locale" needs to be capitalized here as well.

27. +++ b/core/modules/system/system.test
    @@ -2380,7 +2366,7 @@ class RetrieveFileTestCase extends DrupalWebTestCase {
    + * Tests for the shutdown functions.
    

    "Tests" here is being used as a noun, I think, unless we are testing for the presence or absence of shutdown functions, which I don't think is the case.

28. +++ b/core/modules/system/theme.api.php
    @@ -5,46 +5,45 @@
      * Functions and templates for the user interface to be implemented by themes.
    

    The docs in this group are quite redundant and there are a few style errors as well. I'd like to open a followup issue to clean this up.

I'll do the top of the patch after supper; after that, an updated patch with interdiff would be great. At that point I'll just review the interdiff again, and if everything in it checks out I'll be comfortable RTBCing this bad boy.

Alright, reviewed the rest and found just a few more things:

+++ b/core/includes/theme.inc
@@ -793,9 +793,10 @@ function list_themes($refresh = FALSE) {
- * Themes can inherit templates and function implementations from earlier themes.
+ * Themes can inherit templates and function implementations from earlier
+ * themes.

Hrm, why is this being rewrapped? Looks to me like the original line is already under 80 chars.

1. +++ b/core/modules/system/image.gd.inc
   @@ -222,14 +248,16 @@ function image_gd_desaturate(stdClass $image) {
   + * Image toolkit callback: Creates a (GD) image resource from a file.
   

   Does GD need to be in parens here?

2. +++ b/core/modules/system/system.admin.inc
   @@ -1278,13 +1347,17 @@ function system_modules_uninstall($form, $form_state = NULL) {
   - * Confirm uninstall of selected modules.
   + * Builds a form for confirming that the selected modules should be uninstalled.
   ...
   - * @ingroup forms
   ...
     * @return
     *   A form array representing modules to confirm.
   ...
    function system_modules_uninstall_confirm_form($storage) {
   

   It sounds like this is actually a form constructor? Or am I mistaken?

3. +++ b/core/modules/system/system.admin.inc
   @@ -1380,9 +1463,14 @@ function system_ip_blocking($default_ip = '') {
   + *   IP address to be used as the default value of the IP address form field.
   + *   Pass the empty string if you want no default value.
   

   Minor thing, but I think this should be "an empty string" rather than "the empty string"?

4. +++ b/core/modules/system/system.admin.inc
   @@ -1455,10 +1559,12 @@ function system_ip_blocking_delete_submit($form, &$form_state) {
   + * @see system_settings_form(()
   

   Typo here (double opening paren).

5. +++ b/core/modules/system/system.admin.inc
   @@ -2087,11 +2202,12 @@ function theme_system_date_time_settings($variables) {
   + * Form constructor for the add new date format type form.
   
   @@ -2753,7 +2900,14 @@ function theme_system_themes_page($variables) {
   - * Menu callback; present a form for deleting a date format.
   + * Form constructor for the delete date format form.
   
   @@ -2786,7 +2940,14 @@ function system_date_delete_format_form_submit($form, &$form_state) {
   - * Menu callback; present a form for deleting a date type.
   + * Form constructor for the date format type delete form.
   

   These are kind of confusing; for the first, maybe:
   Form constructor for the 'Add new date format type' form.
   or
   Form constructor for a form to add new date format types.

   Similarly for the second and third constructors.

Rerolling addressing the issues in #59 and #61, except:

6. in #59: "false" and "true" are not PHP constants here, but the English words, so I think we should not capitalize them.

14. in #59: Let's postpone this for another issue, esp. since I am also not sure what the right thing to do is (with "Updater class" in particular.

#61, unnumbered: No, the period takes us over 80 chars.

I agree with xjm about #59/6 (caps for FALSE/TRUE).

RE #59/14 Capitalization... I don't think we need a separate issue. There is a class called Updater, and hook_updater_info() in system.api.php allows modules to tell Drupal about their own updater classes.... So, to be consistent with English rules and how we're doing other things in core, I think the capitalization should be:
- If referring to the specific Drupal Core class called "Updater", capitalize it.
- Otherwise, don't capitalize. This is consistent with the other hook_*_info() hooks in core, which do not capitalize their subjects ("block", "filter", "hook", etc.). So we also shouldn't have caps for "the updater classes", "decide if each updater wants to...", "override an existing updater", etc.

Updating for #1299424: Allow one module per directory and move system tests to core/modules/system.

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!

Ouch, this one was fun. Patch #62 didn't apply cleanly, so I reverted the code to revision e6e4bc1 and applied patch #55. After merging and handling conflicts, I then went back through @xjm's updates and re-applied. This will unfortunately probably need a complete re-review as I can't create an interdiff.

Yeah, sorry, the approach we were taking in the past on these patches wasn't working all that well... So yes, if you can make a patch that doesn't introduce new errors, I will commit it. I'm not really willing to commit patches that introduce new problems (such as substituting one bad piece of documentation for another bad piece of documentation), but for instance if you make a patch that just fixes the "make sure there are spaces between @param and @return", I would commit that, and then we could go back and do the next bit of cleanup.

Type hinting -- PLEASE NO! That makes the patch really hard to review, and is *specifically* not included in this effort.

Please remove the type hinting to a separate issue. The cleanup stuff that *is* part of this particular clean-up initiative requires almost no thought to review, and the review can be done without even looking at the code. Whereas the type hinting requires a careful read of the code to make sure it is correct. I can't give this patch a quick look-over to see if it's all fine and commit it with that type hinting in there, and I do not have Drupal volunteer time that I want to spend any time soon reading the code carefully just to commit a clean-up patch (there are several other clean-up patches waiting in the queue, and API module improvements to do, etc.). Sorry, that's just how it is.... Type hinting is a separate effort, and it is that way for a reason.

I'm sorry that this is frustrating for you. It's also frustrating for me -- I've been through this discussion about type hinting for this particular initiative so many times that I don't have much patience for explaining again why they aren't part of this particular initiative and why I'd appreciate it if they weren't added to patches on this initiative. Would you consider re-rolling the patch without the type hints so that we can move forward?

Please... Do I have to say this again really? Yes, type hinting is great. No, it doesn't belong on this issue. Please move the type hinting part of this patch to the initiative you have already started for that, and get the patch reviewed there (there are plenty of people who feel strongly about it as you do), and it will be considered for committing. Don't put it here. And please don't keep discussing this on this issue, which is part of an initiative which does not include type hinting and which I am the only reviewer/commiter for. Please. Please. Please.

Looks pretty good! This needs a quick update for two small fixes, and then I think it can go in:

a) image.gd.inc

+ * The form element returned by this function are integrated into the form
+ * built by system_image_toolkit_settings().
+ *
+ * @return
+ *   An array of Form API elements to be added to form.
+ *
+ * @see hook_image_toolkits()
  */
 function image_gd_settings() {

- That first paragraph here has a grammatical error "element... are integrated". Should be "elements".
- The return needs "the" added: ... to be added to *the* form.

b) In image_gd_rotate() docs:

+ *   (optional) An hexadecimal integer specifying the background color to use
+ *   for the uncovered area of the image after the rotation. E.g. 0x000000 for

A hexadecimal... not An hexadecimal...

Thanks! I committed all of this patch except the system-rtl stylesheet change (it should have replaced that line, not added to the documentation header):

--- a/core/modules/system/system.admin-rtl.css
+++ b/core/modules/system/system.admin-rtl.css
@@ -1,6 +1,7 @@
 /**
  * @file
  * RTL styles for administration pages.
+ * Right-to-left-specific stylesheet for System module administration pages.
  */

I missed that in the last review, sorry!

So... There is more to be done on this issue... There were files in the patch from #68 that didn't make it into the patches starting in #74 that led to this patch, so let's go back and get the rest (or another chunk) in the next patch. Thanks!

Thanks for the patch! I reviewed about half of it, and found quite a few things that need attention... presumably the second half has many of the same problems, since most of them were repeated several times in the patch. Here's what I found:

a) The first-line changes in language.api.php are wrong:
http://drupal.org/node/1354#hooks

b) system.admin.inc

/**
- * Menu callback; Provide the administration overview page.
+ * Menu callback; Provides the administration overview page.

Should be : not ;

c) Some first-line descriptions and other added documentation could use a/an/the added, such as:
- Menu callback: Creates form to display theme configuration. ==> *a* form to display *the* theme configuration
- This form constructor creates a theme configuration form that can be used both for entire site and for individual themes. ==> for *the* entire site
- Menu callback: Provides module enable/disable interface. ==> Provides *a* module...
There are others... reading through the patch would be helpful.

d) On the other hand, this "the" should be removed:

/**
- * Validator for the system_theme_settings() form.
+ * Form validation handler for the system_theme_settings().
+ *
+ * @see system_theme_settings_submit()
  */
 function system_theme_settings_validate($form, &$form_state) {

e) system_modules() should be documented as a form constructor (and leave out @param for form, form_state). Same for system_performance_settings(), and possibly other form constructors too. Some submission handlers are also not brought up to standards.

f)

/**
- * Array sorting callback; sorts modules or themes by their name.
+ * Callback for uasort() within system_themes_page().
  */
 function system_sort_themes($a, $b) {

See http://drupal.org/node/1354#callbacks

g) First line for system_modules_uninstall() needs a verb... or needs to be documented as a form constructor (after Page callback:). One or the other. Oh, and it should say "Page callback" not "Menu callback".

/**
- * Builds a form of currently disabled modules.
+ * Menu callback: Form for a listing of currently disabled modules.

There are several other functions like this too. The standard is:

Page callback: Form constructor for...

Nothing should ever say "Menu callback:". See
http://drupal.org/node/1354#menu-callback

h) In several cases, information was lost when updating the form submission handler docs to standards. For instance:

 /**
- * Submit callback; clear system caches.
- *
- * @ingroup forms
+ * Form submission handler for system_performance_settings().
  */
 function system_clear_cache_submit($form, &$form_state) {
   drupal_flush_all_caches();
@@ -1701,19 +1787,19 @@ function system_clear_cache_submit($form, &$form_state) {
 }
 
 /**
- * Submit callback; clear the page cache.
- *
- * @ingroup forms
+ * Form submission handler for system_performance_settings().
  */
 function system_clear_page_cache_submit($form, &$form_state) {

Probably both of these should say "Form submission handler for the ... button for system_performance_settings()" so that the information is not lost. There are other examples like this in the patch as well.

@Lars Is system_sort_themes() a callback for uasort()? It looks like you just copied the example in the documentation.

The line about "Callback for..." is not supposed to be the first line of the function -- the first line should still say something like "Sorts the..." or whatever.

Um. http://drupal.org/node/1354#callbacks starts with:

"For callbacks, add a blank line after the summary, followed by a line that identifies the callback's purpose. Use the following format:"

Can you suggest clearer wording? I think this says to leave the summary as normal and have an additional line with that format?

good idea, how's this? http://drupal.org/node/1354#callbacks

Looks pretty good! A few things to fix up:

a) This is a bit more verbose than our standard wording for callbacks (see links above):

+ * Helps to sort a list of modules or themes by their name.
+ *
+ * This function os a callback for uasort() within system_themes_page().
  */
 function system_sort_themes($a, $b) {

Also, "os" is misspelled in that line.

b)

+++ b/core/modules/system/language.api.php
@@ -11,7 +11,7 @@
  */
 
 /**
- * Allows modules to act after language initialization has been performed.
+ * Allow modules to act after language initialization has been performed.

Normally in hook definitions, we wouldn't need the "Allow modules to...". So this could just say "Act after language...". (That would also be more consistent with the other hooks in this file).

c)

+ *
+ * @return array
+ *   A form array.
+ *
  * @ingroup forms
  */
 function system_modules_confirm_form($modules, $storage) {

Omit @return for form constructors. (Applies to several other functions in this patch too.)

d)

+ * Form submission handler for system_cron_settings().
  */
 function system_cron_settings_submit($form, &$form_state) {
   config('system.cron')
@@ -1537,9 +1619,7 @@ function system_cron_settings_submit($form, &$form_state) {
 }
 
 /**
- * Submit callback; run cron.
- *
- * @ingroup forms
+ * Form submission handler for system_cron_settings().
  */
 function system_run_cron_submit($form, &$form_state) {

These two functions do two different things and your docs have lost that information. One should say "for the run cron button on..." or something.

Similarly:

/**
- * Submit callback; clear system caches.
+ * Form submission handler for system_performance_settings().
  *
- * @ingroup forms
+ * This function performs the action of clearing the system caches.
  */
 function system_clear_cache_submit($form, &$form_state) {
   drupal_flush_all_caches();
@@ -1701,19 +1783,21 @@ function system_clear_cache_submit($form, &$form_state) {
 }
 
 /**
- * Submit callback; clear the page cache.
+ * Form submission handler for system_performance_settings().
  *
- * @ingroup forms
+ * This function performs the action of clearng the page cache.
  */
 function system_clear_page_cache_submit($form, &$form_state) {

It would be good if the information was in the first line about which submit handler does what (also "clarng" is misspelled).

e)

+ * @param bool $check
+ *   (optional) If true, only returns a Boolean whether there are system status
+ *   errors. Defaults to FALSE.
+ *
+ * @return string
+ *   An HTML-formatted string.
  */
 function system_status($check = FALSE) {

I'm not too fond of how that @param doc is worded.

f) Looks like this patch isn't finished:

+ * @return mixed
+ *   ???

g) in system.module:

/**
- * Page callback; Execute cron tasks.
+ * Page callback; Executes cron tasks.

Should be : not ; here

h)

/**
- * Menu item access callback - only enabled themes can be accessed.
+ * Menu item access callback: Only enabled themes can be accessed.
+ *
+ * @param string|object $theme
+ *   Either the name of a theme or a full theme object.
+ *
+ * @return bool
+ *   TRUE if user has permission to both 'administer themes' and to access the
+ *   enabled theme; otherwise, FALSE.
  */
 function _system_themes_access($theme) {

- First line: Should say "Access callback: " and then start with a verb.
- Doesn't need @return for an access callback.

i)

/**
- * Use authorize.php to run batch_process().
+ * Usse authorize.php to run batch_process().
  *
  * @see batch_process()
+ *
+ * @ingroup batch
  */
 function system_authorized_batch_process() {

Typo in first line.

j)

/**
- * Helper function to sort requirements.
+ * Callback for uasort() to sort items by weight.
  */
 function _system_sort_requirements($a, $b) {

See above comments about callback function standards.

k)

/**
- * Generates a form array for a confirmation form.
+ * Generatess a form array for a confirmation form.
...
+ *
+ * @ingroup forms
  */
 function confirm_form($form, $question, $path, $description = NULL, $yes = NULL, $no = NULL, $name = 'confirm') {

This should not be part of @ingroup forms (that is for form-generating functions, not the form API core fucntions, right?).
- Typo in the first line also.

l)

+ *
+ * @ingroup system_menu()
  */
 function system_admin_compact_page($mode = 'off') {

I think that was intended as @see?

m)

/**
- * Menu callback; Retrieve a JSON object containing a suggested time zone name.
+ * Menu callback; Retrieves a JSON object containing a suggested time zone name.
...
+ * @param int $offset
+ *   (optional) Integer offset from GMT in seconds. Defaults to negative one,
..
+ * @param int $is_daylight_savings_time
+ *   (optional) Daylight savings time indicator. A value of -1 means that
...
function system_timezone($abbreviation = '', $offset = -1, $is_daylight_saving_time = NULL) {

- First line: ; ==> :
- It seems really silly to me to write out "negative 1" in code docs. Just write "-1".
- The type for @param $is_daylight can't be right since the default is NULL... and the documentation doesn't say anything about what NULL means/does.

n)

/**
  * Returns HTML for the Powered by Drupal text.
  *
+ * @return string
+ *   An HTML-formatted string.
+ *
  * @ingroup themeable
  */

Revert this change. See http://drupal.org/node/1354#themeable -- same for the other theme_xyz() functions in this patch.

Added related issue link to #1533400: Make system module (except tests subdirectory) pass Coder Review.

These issues are a lot of work with very little tangible payoff, so I'm closing the rest of them as "won't fix". Your efforts on working on this issue were appreciated... it was just my fault for starting a task that was very difficult to get right.

Let's instead put our effort into fixing and reviewing documentation that is really unclear and/or wrong, and I hope that the people who worked on these issues are not afraid to jump into a more reasonable issue!