Final of the series (sorry it was so prolific- I should have done all the modules as one patch). This includes all the minimal changes needed for the stuff I haven't already done.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

Status: Needs review » Reviewed & tested by the community

Igen.

catch’s picture

Status: Reviewed & tested by the community » Needs work

No longer applies.

aspilicious’s picture

Let me help cleanup some old issues...

aspilicious’s picture

Status: Needs work » Needs review
aspilicious’s picture

Component: other » documentation
jhodgdon’s picture

Status: Needs review » Needs work

Mostly OK.

A few things:
- Doc headers for functions need to start with 3rd person verb, like:

- * Return the complete URL that should be redirected to during an installation
- * request.
+ * Return the complete URL redirected to during an installation request.

Should be "Returns".

Also verify -> verifies in the next one.

Also, this one needs a one-line description at the beginning:

/**
  * Global flag to identify update.php run, and so avoid various unwanted
  * operations, such as hook_init() and hook_exit() invokes, css/js preprocessing
- * and translation, and solve some theming issues. This flag is checked on several
- * places in Drupal code (not just update.php).
+ * and translation, and solve some theming issues. This flag is checked on
+ * several places in Drupal code (not just update.php).
  */
 define('MAINTENANCE_MODE', 'update');
aspilicious’s picture

  * Global flag to identify update.php run
  * 
  * This flag avoids various unwanted operations, such as hook_init() and 
  * hook_exit() invokes, css/js preprocessing and translation, and solve some 
  * theming issues. This flag is checked on several places in Drupal code (not 
  * just update.php).

something like this?

jhodgdon’s picture

Yeah, something like that would be good.... How about:

 * Global flag indicating that update.php is being run.
  *
  * When this flag is set, various operations do not take place, such
  * as invoking hook_init() and hook_exit(), css/js preprocessing, and translation.
  * The existence of this flag is checked in function_foo(), function_bar(), and ...
aspilicious’s picture

Nice like it, my english skills aren't good enough to write such a text, luckily we have jhodgdon...

I'll fix those asap

aspilicious’s picture

I have the feeling that this one is getting big again...

aspilicious’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, general_commentstandardsv3.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review

#10: general_commentstandardsv3.patch queued for re-testing.

jhodgdon’s picture

Status: Needs review » Needs work

Current patch:

a)

 /**
- * Verify existing settings.php
+ * Verify existing settings.php.

Verify -> Verifies... Actually, what does this function do? Probably should say "Verifies the existence of settings.php.", if that is what the function does? The current doc header doesn't make any sense to me.

b)

- * Check a database connection and return any errors.
+ * Checks a database connection and return any errors.

return -> returns

This also happens later in the file:

 /**
- * Check installation requirements and report any errors.
+ * Checks installation requirements and report any errors.
  */

report -> reports

c)

 /**
- * Batch callback for batch installation of modules.
+ * Batches callback for batch installation of modules.
  */
 function _install_module_batch($module, $module_name, &$context) {

Good try, but I don't think "Batch callback" is a verb. :) I would just leave this as "Batch callback" for now at least.

d)

+ * Global flag indicating that update.php is being run.
+ *
+ * When this flag is set, various operations do not take place, such as invoking
+ * hook_init() and hook_exit(), css/js preprocessing, and translation. The
+ * existence of this flag is checked in function_foo(), function_bar(), and ...
  */
 define('MAINTENANCE_MODE', 'update');

I didn't mean the ... or function_foo() function_bar() etc. literally! Either remove that sentence or track down where that flag is checked for. :)

aspilicious’s picture

d) I thought it was a coding standard for telling a lot of places.

In general: my english sucks

aspilicious’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Almost! There is still one more English grammar spot. Fix this and it's RTBC as far as I am concerned:

 /**
- * Display themed installer output and end the page request.
+ * Displays themed installer output and end the page request.

end -> ends

jhodgdon’s picture

Status: Needs review » Needs work
aspilicious’s picture

RTBC version :)

aspilicious’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

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