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

Files:

  • core/authorize.php
  • core/install.php
  • core/update.php
Files: 
CommentFileSizeAuthor
#40 1606946-40.patch449 bytesDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 39,589 pass(es).
[ View ]
#37 clean_up_api_docs-1606946-37.patch589 bytesAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es).
[ View ]
#35 clean_up_api_docs-1606946-35.patch408 bytesAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 39,595 pass(es).
[ View ]
#27 clean_up_api_docs-1606946-27-d8.patch395 bytesAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 48,952 pass(es).
[ View ]
#27 clean_up_api_docs-1606946-27-d7-do-not-test.patch408 bytesAlbert Volkman
#25 clean_up_api_docs-1606946-25.patch6.85 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 39,553 pass(es).
[ View ]
#23 clean_up_api_docs-1606946-23.patch938 bytesAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 48,787 pass(es).
[ View ]
#19 clean-up-api-docs-1606946-19.patch693 bytesudaksh
PASSED: [[SimpleTest]]: [MySQL] 47,862 pass(es).
[ View ]
#13 clean-up-api-docs-1606946-13.patch6.85 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 41,307 pass(es).
[ View ]
#13 interdiff.txt7.3 KBAlbert Volkman
#11 clean-up-api-docs-1606946-11.patch11.58 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 41,080 pass(es).
[ View ]
#11 interdiff.txt1.17 KBAlbert Volkman
#10 clean-up-api-docs-1606946-10.patch11.54 KBbunthorne
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch clean-up-api-docs-1606946-10.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#8 1606946-8.patch0 bytesbunthorne
PASSED: [[SimpleTest]]: [MySQL] 41,080 pass(es).
[ View ]
#6 1606946-6.patch1.58 KBbunthorne
PASSED: [[SimpleTest]]: [MySQL] 41,084 pass(es).
[ View ]

Comments

Component:other» documentation

fix component

tag

Assigned:Unassigned» markgifford

I'll look into this issue.

core/xmlrpc.php is not in the directory; xmlrpc was moved into its own module - see #1627496: Switch XML-RPC callback from "xmlrpc.php" to "xmlrpc" and #1033818: Move xmlrpc includes into xmlrpc.module - so I'll try and tackle the three remaining files:

core/authorize.php
core/install.php
core/update.php

Ah. I guess we need a new issue for the XML-RPC module. I'll add that. Thanks!

Assigned:markgifford» Unassigned

Sorry, unassigning and putting back in the queue - I'm in the middle of a really busy couple of weeks so can't give this the time it needs. I hope to help out with the documentation in the near future once things die down

StatusFileSize
new1.58 KB
PASSED: [[SimpleTest]]: [MySQL] 41,084 pass(es).
[ View ]

Novices welcome? Here is a novice patch for authorize.php. More work will be needed, no doubt.

Status:Active» Needs work

StatusFileSize
new0 bytes
PASSED: [[SimpleTest]]: [MySQL] 41,080 pass(es).
[ View ]

Patch for install.php attached. Keeping status as needs work.

Thanks for starting on this!

The patch in #8 is an empty file (0 bytes). I took a quick look at the patch in #6 though, and it seems like you're mostly on the right track. A few notes:

a) One-line documentation summaries need to end in ".", such as:

+ * Global flag to identify update.php and authorize.php runs

b) You might need a few "the" words added here and there (and don't remove ones that were in the documentation before!), such as in:

+ * Defines root directory of Drupal installation.
  */
define('DRUPAL_ROOT', getcwd());

(Defines *the* root directory of *the* Drupal installation.)

c) Except in the case of terribly unclear writing, just fix up the /** */ documentation blocks, not in-code // comments.

d) Always wrap paragraphs of documentation as close to 80 characters in a line as possible without going over 80 characters. So:

+ * operations such as hook_init() and hook_exit() invokes,
+ * css/js preprocessing and translation, and solves some theming issues.

--> css/js should be moved up to the previous line.

e) There are trailing blank spaces at the end of some lines. You should be able to set your code editor to remove or at least show the blank spaces.

f) We actually don't require that variables/constants documentation start with a verb:
http://drupal.org/node/1354#constants

StatusFileSize
new11.54 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch clean-up-api-docs-1606946-10.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

The update.php file has 3 places where functions had no definition; I did not feel qualified to write it. I entered NEEDS FUNCTION DEFINITION as placeholders.
Patch includes changes for all 3 files (authorize.php, install.php, update.php).
Setting to needs work because of the 3 functions needing definition.

Status:Needs work» Needs review
StatusFileSize
new1.17 KB
new11.58 KB
PASSED: [[SimpleTest]]: [MySQL] 41,080 pass(es).
[ View ]

First stab at it.

Status:Needs review» Needs work

Thanks, looks mostly good! A couple of things to address:

a)

-// Change the directory to the Drupal root.
+// Changes the directory to the Drupal root.
chdir('..');

I'm not fond of this change. We don't normally use verbs in 3rd person in in-code comments. Same here:
-// Exit early if running an incompatible PHP version to avoid fatal errors.
+// Exits early if running an incompatible PHP version to avoid fatal errors.

and elsewhere in the patch.

Generally, I would actually not make any changes to in-code comments. Just change the documentation block comments (/** */).

b)

- *   TRUE if the current user can run authorize.php, otherwise FALSE.
+ *   TRUE if current user can run authorize.php, and FALSE if it cannot.

It seems to me that both the original and new lines here have some grammatical issues. How about:

TRUE if the current user can run authorize.php, and FALSE if not.

(I am not sure I would want to call a user "it" :) )

c)

-// Change the directory to the Drupal root.
+// Changes directory to Drupal root.

I would not take "the" out here.

d)

/**
- * Global flag to indicate that site is in installation mode.
+ * Sets a global flag to indicate the site is in installation mode.
  *
- * This constant is defined using define() instead of const so that PHP
- * versions older than 5.3 can display the proper PHP requirements instead of
- * causing a fatal error.
+ * The constant is defined using define() instead of const so that PHP
+ * versions prior to 5.3 can display proper PHP requirements instead of causing
+ * a fatal error.
  */
define('MAINTENANCE_MODE', 'install');

I wouldn't say that a define "sets" a global flag. It is probably better in this case not to start it with a verb. A constant doesn't really do anything -- it just sits there.

e) We need some more "the" or "a" words, such as in:

+/**
+ * Renders form with a list of available database updates.
+ */

and
+/**
+ * Provides links to homepage and administration pages.
+ */
function update_helpful_links() {

and elsewhere

f) I wouldn't remove the blank line here:

}
-
+/**
+ * Form constructor for the list of available database module updates.
+ */
function update_script_selection_form($form, &$form_state) {

Normally we do want a blank line between functions.

g) Don't change code lines, just documentation, such as here:

-  $output .= "<li><strong>Back up your database</strong>. This process will change your database values and in case of emergency you may need to revert to a backup.</li>\n";
-  $output .= "<li><strong>Back up your code</strong>. Hint: when backing up module code, do not leave that backup in the 'modules' or 'sites/*/modules' directories as this may confuse Drupal's auto-discovery mechanism.</li>\n";
+  $output .= "<li><strong>Back up your database</strong>. This process will change your database values. In case of emergency you may need to revert to a backup.</li>\n";
+  $output .= "<li><strong>Back up your code</strong>. Hint: when backing up module code, do not leave the backup in the 'modules' or 'sites/*/modules' directories as this may confuse Drupal's auto-discovery mechanism.</li>\n";

etc.

Status:Needs work» Needs review
StatusFileSize
new7.3 KB
new6.85 KB
PASSED: [[SimpleTest]]: [MySQL] 41,307 pass(es).
[ View ]

Fixed up.

Status:Needs review» Reviewed & tested by the community

Much better, thanks! I think this is ready to go and I'll get it committed shortly.

Status:Reviewed & tested by the community» Needs work

I committed the patch in #13 to Drupal 8.x.

There's another function in update.php that needs docs: function update_info_page()
Oh and another one: function update_access_denied_page()
So, we need a new patch. Thanks!

And update_selection_page(), update_script_selection_form(), update_helpful_links(), and update_results_page().

RE #16 - those seem to have docs. I think only the two mentioned in #15 still need them.

Ah, I thought I had the patch applied. Nevermind :)

StatusFileSize
new693 bytes
PASSED: [[SimpleTest]]: [MySQL] 47,862 pass(es).
[ View ]

Patch for #15

Status:Needs work» Needs review

Setting status

Status:Needs review» Reviewed & tested by the community
Issue tags:-docs-cleanup-2011

Patch meets Drupal coding standards and the doctags make sense to me. Marking RTBC.

Status:Reviewed & tested by the community» Needs work

Thanks!

The patch needs a bit of a grammar cleanup though:
Provides overview of Drupal database update.
needs some a/an/the added... and maybe a few more words about what it is really doing.

Status:Needs work» Needs review
StatusFileSize
new938 bytes
PASSED: [[SimpleTest]]: [MySQL] 48,787 pass(es).
[ View ]

Might still need a bit of wordsmithing. Also, those functions are kind of ugly. They both generate HTML without render arrays.

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

Thanks -- looks good to me! Committed to 8.x.

Looks like we need to port the above committed patches to 7.x now.

Status:Patch (to be ported)» Needs review
StatusFileSize
new6.85 KB
PASSED: [[SimpleTest]]: [MySQL] 39,553 pass(es).
[ View ]

Here we go, D7 style!

Version:7.x-dev» 8.x-dev
Status:Needs review» Needs work

Thanks! That patch looks good for 7.x, so I committed it.

One quick follow-up for both 8.x and 7.x:

+/**
+ * Renders form with a list of available database updates.
+ */
function update_selection_page() {

(this is in update.php and needs "a" as the second word).

Status:Needs work» Needs review
StatusFileSize
new408 bytes
new395 bytes
PASSED: [[SimpleTest]]: [MySQL] 48,952 pass(es).
[ View ]

Here's a patch for each.

Status:Needs review» Needs work
Issue tags:-Novice, -needs backport to D7

The last submitted patch, clean_up_api_docs-1606946-27-d8.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, clean_up_api_docs-1606946-27-d8.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+Novice, +needs backport to D7

Status:Needs review» Reviewed & tested by the community

If the test fails again, depending on what it is, we should probably report a "random test failure" bug. This patch is obviously fine...

There were some bot issues last night (hence my re-tests) :)

Status:Reviewed & tested by the community» Fixed

I think I can handle this one. :D

Committed and pushed to 8.x. Thanks!

Version:8.x-dev» 7.x-dev
Status:Fixed» Needs review
StatusFileSize
new408 bytes
PASSED: [[SimpleTest]]: [MySQL] 39,595 pass(es).
[ View ]

Re-uploading the patch for D7.

Status:Needs review» Fixed

Thanks! Committed to 7.x.

Version:7.x-dev» 6.x-dev
Status:Fixed» Needs review
StatusFileSize
new589 bytes
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es).
[ View ]

D6!

Status:Needs review» Reviewed & tested by the community

OK, we can add that to D6. :) I'll get it committed shortly.

Version:6.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Fixed

Thanks! Committed to 6.x. I'm going to leave this issue back as 7.x though since 8.x/7.x was the bulk of the work.

Status:Fixed» Reviewed & tested by the community
StatusFileSize
new449 bytes
PASSED: [[SimpleTest]]: [MySQL] 39,589 pass(es).
[ View ]

Hm, looks like a case of blind D8-to-D7 backport here :) Drupal 7 does not require PHP 5.3 anywhere, so "const" would never be used at all.

Moving straight to RTBC since I think it's a pretty trivial fix.

Status:Reviewed & tested by the community» Fixed

Hm, I disagree actually. Here is the comment as it is now (the part that #40 wants to remove):

- *
- * The constant is defined using define() instead of const so that PHP
- * versions prior to 5.3 can display proper PHP requirements instead of causing
- * a fatal error.

As I read it, this is actually specifically addressing the 7.x case, and I think it should stay. It's trying to say that D7 can be run on PHP 5.2 or 5.3, and the comment basically says "If you're running on 5.2, this is defined using define() not const so that Drupal can show requirements correctly for you."

Right?

Status:Fixed» Needs review

Nope, these were added in Drupal 8 only, around the time the rest of the codebase switched to using "const" (to explain why these particular places weren't). See:
#1355798: Installer on PHP <5.3 results in parse error instead of warning about old php
#1414394: Fix upgrade check for D8 and php 5.3

The idea is to say that even though Drupal 8 requires PHP 5.3, we're making install.php (and update.php) semi-PHP-5.2 compatible so you can get far enough to get a friendly message on the screen telling you that the rest of Drupal 8 isn't.

In Drupal 7, the whole codebase is PHP 5.2-compatible, so I don't think there's any reason to have comments in specific places saying so?

Status:Needs review» Reviewed & tested by the community

OK. Then let's get the patch in #40 committed. Any other places we need to patch?

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 7.x. Thanks!

Any other places we need to patch?

Looking at the committed patch, I don't see any. Thanks!

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

Issue summary:View changes

Updated issue summary.