Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Part of meta-issue #1310084: [meta] API documentation cleanup sprint
Files:
- core/authorize.php
- core/install.php
- core/update.php
Comment | File | Size | Author |
---|---|---|---|
#40 | 1606946-40.patch | 449 bytes | David_Rothstein |
#37 | clean_up_api_docs-1606946-37.patch | 589 bytes | Albert Volkman |
#35 | clean_up_api_docs-1606946-35.patch | 408 bytes | Albert Volkman |
#27 | clean_up_api_docs-1606946-27-d8.patch | 395 bytes | Albert Volkman |
#27 | clean_up_api_docs-1606946-27-d7-do-not-test.patch | 408 bytes | Albert Volkman |
Comments
Comment #1
jhodgdonfix component
Comment #2
jhodgdontag
Comment #3
markgifford CreditAttribution: markgifford commentedI'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
Comment #4
jhodgdonAh. I guess we need a new issue for the XML-RPC module. I'll add that. Thanks!
Comment #5
markgifford CreditAttribution: markgifford commentedSorry, 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
Comment #6
bunthorne CreditAttribution: bunthorne commentedNovices welcome? Here is a novice patch for authorize.php. More work will be needed, no doubt.
Comment #7
bunthorne CreditAttribution: bunthorne commentedComment #8
bunthorne CreditAttribution: bunthorne commentedPatch for install.php attached. Keeping status as needs work.
Comment #9
jhodgdonThanks 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:
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 *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:
--> 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
Comment #10
bunthorne CreditAttribution: bunthorne commentedThe 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.
Comment #11
Albert Volkman CreditAttribution: Albert Volkman commentedFirst stab at it.
Comment #12
jhodgdonThanks, looks mostly good! A couple of things to address:
a)
I'm not fond of this change. We don't normally use verbs in 3rd person in in-code comments. Same here:
and elsewhere in the patch.
Generally, I would actually not make any changes to in-code comments. Just change the documentation block comments (/** */).
b)
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)
I would not take "the" out here.
d)
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:
and
and elsewhere
f) I wouldn't remove the blank line here:
Normally we do want a blank line between functions.
g) Don't change code lines, just documentation, such as here:
etc.
Comment #13
Albert Volkman CreditAttribution: Albert Volkman commentedFixed up.
Comment #14
jhodgdonMuch better, thanks! I think this is ready to go and I'll get it committed shortly.
Comment #15
jhodgdonI 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!
Comment #16
Albert Volkman CreditAttribution: Albert Volkman commentedAnd update_selection_page(), update_script_selection_form(), update_helpful_links(), and update_results_page().
Comment #17
jhodgdonRE #16 - those seem to have docs. I think only the two mentioned in #15 still need them.
Comment #18
Albert Volkman CreditAttribution: Albert Volkman commentedAh, I thought I had the patch applied. Nevermind :)
Comment #19
udaksh CreditAttribution: udaksh commentedPatch for #15
Comment #20
Albert Volkman CreditAttribution: Albert Volkman commentedSetting status
Comment #21
disasm CreditAttribution: disasm commentedPatch meets Drupal coding standards and the doctags make sense to me. Marking RTBC.
Comment #22
jhodgdonThanks!
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.
Comment #23
Albert Volkman CreditAttribution: Albert Volkman commentedMight still need a bit of wordsmithing. Also, those functions are kind of ugly. They both generate HTML without render arrays.
Comment #24
jhodgdonThanks -- looks good to me! Committed to 8.x.
Looks like we need to port the above committed patches to 7.x now.
Comment #25
Albert Volkman CreditAttribution: Albert Volkman commentedHere we go, D7 style!
Comment #26
jhodgdonThanks! That patch looks good for 7.x, so I committed it.
One quick follow-up for both 8.x and 7.x:
(this is in update.php and needs "a" as the second word).
Comment #27
Albert Volkman CreditAttribution: Albert Volkman commentedHere's a patch for each.
Comment #29
Albert Volkman CreditAttribution: Albert Volkman commented#27: clean_up_api_docs-1606946-27-d8.patch queued for re-testing.
Comment #31
Albert Volkman CreditAttribution: Albert Volkman commented#27: clean_up_api_docs-1606946-27-d8.patch queued for re-testing.
Comment #32
jhodgdonIf the test fails again, depending on what it is, we should probably report a "random test failure" bug. This patch is obviously fine...
Comment #33
Albert Volkman CreditAttribution: Albert Volkman commentedThere were some bot issues last night (hence my re-tests) :)
Comment #34
webchickI think I can handle this one. :D
Committed and pushed to 8.x. Thanks!
Comment #35
Albert Volkman CreditAttribution: Albert Volkman commentedRe-uploading the patch for D7.
Comment #36
jhodgdonThanks! Committed to 7.x.
Comment #37
Albert Volkman CreditAttribution: Albert Volkman commentedD6!
Comment #38
jhodgdonOK, we can add that to D6. :) I'll get it committed shortly.
Comment #39
jhodgdonThanks! 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.
Comment #40
David_Rothstein CreditAttribution: David_Rothstein commentedHm, 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.
Comment #41
jhodgdonHm, I disagree actually. Here is the comment as it is now (the part that #40 wants to remove):
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?
Comment #42
David_Rothstein CreditAttribution: David_Rothstein commentedNope, 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?
Comment #43
jhodgdonOK. Then let's get the patch in #40 committed. Any other places we need to patch?
Comment #44
webchickCommitted and pushed to 7.x. Thanks!
Comment #45
David_Rothstein CreditAttribution: David_Rothstein commentedLooking at the committed patch, I don't see any. Thanks!
Comment #46.0
(not verified) CreditAttribution: commentedUpdated issue summary.