Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon’s picture

Component: other » documentation

fix component

jhodgdon’s picture

Issue tags: +Novice, +Needs backport to D7

tag

markgifford’s picture

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

jhodgdon’s picture

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

markgifford’s picture

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

bunthorne’s picture

FileSize
1.58 KB

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

bunthorne’s picture

Status: Active » Needs work
bunthorne’s picture

FileSize
0 bytes

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

jhodgdon’s picture

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

bunthorne’s picture

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.

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
1.17 KB
11.58 KB

First stab at it.

jhodgdon’s picture

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.

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
7.3 KB
6.85 KB

Fixed up.

jhodgdon’s picture

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.

jhodgdon’s picture

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!

Albert Volkman’s picture

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

jhodgdon’s picture

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

Albert Volkman’s picture

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

udaksh’s picture

Patch for #15

Albert Volkman’s picture

Status: Needs work » Needs review

Setting status

disasm’s picture

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.

jhodgdon’s picture

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.

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
938 bytes

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

jhodgdon’s picture

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.

Albert Volkman’s picture

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

Here we go, D7 style!

jhodgdon’s picture

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).

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
408 bytes
395 bytes

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.

Albert Volkman’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

Albert Volkman’s picture

Status: Needs work » Needs review
Issue tags: +Novice, +Needs backport to D7
jhodgdon’s picture

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...

Albert Volkman’s picture

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

webchick’s picture

Status: Reviewed & tested by the community » Fixed

I think I can handle this one. :D

Committed and pushed to 8.x. Thanks!

Albert Volkman’s picture

Version: 8.x-dev » 7.x-dev
Status: Fixed » Needs review
FileSize
408 bytes

Re-uploading the patch for D7.

jhodgdon’s picture

Status: Needs review » Fixed

Thanks! Committed to 7.x.

Albert Volkman’s picture

Version: 7.x-dev » 6.x-dev
Status: Fixed » Needs review
FileSize
589 bytes

D6!

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

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

jhodgdon’s picture

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.

David_Rothstein’s picture

Status: Fixed » Reviewed & tested by the community
FileSize
449 bytes

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.

jhodgdon’s picture

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?

David_Rothstein’s picture

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?

jhodgdon’s picture

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?

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 7.x. Thanks!

David_Rothstein’s picture

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.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.