Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Part of meta-issue #1310084: [meta] API documentation cleanup sprint
Part of meta-issue #1310084: [meta] API documentation cleanup sprint
Comments
Comment #1
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedWorking on this.
Comment #2
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedVoilà. After having dived into this code, I would like to congratulate dww on a great job of documenting the Update manager module.
In the depths of this module I found an unused argument. After the clean-up the docblock for this function now includes:
They contain exactly the same data. I've opened a separate issue to remove the duplicate and unused argument: #1505146: Remove unused argument from update_calculate_project_update_status(). Hoping to get that committed so I can remove the $project @param from this patch.
Comment #3
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedUpdated, as #1505146: Remove unused argument from update_calculate_project_update_status() and #1481156: Incorrect logic in creating url to fetch information about project updates were committed to D8.
Comment #4
jhodgdonThanks! This looks pretty good. A few things I would fix:
a)
"module file of the module used to"... that seems excessively convoluted. Can't we just say "Module for testing update manager functionality" or something equally short?
b) Also we need to agree on a standard for capitalization here. Throughout:
- If you are referring to the module, it should be "the Update Manager module". Don't use "update.module" to refer to the module as a whole, only to refer to that specific file. [this standard is not open to discussion at this point]
- If you are referring to the manager, it should be "the update manager" (open to discussion on this one -- I would be open to calling it "the Update Manager" if you think update manager is a proper name, but not "the Update manager" with mixed case).
c) in update.api.php
Can we get rid of this "mere mortals" thing? It is very condescending.
d) and a few lines down:
should be ; before "for example".
e) in update.manager.inc
- Verb tenses do not agree in that last line.
- Need comma before "and" in the list in the last line.
f)
I think you can leave out "The update manager module" in this?
g) a few lines down in the patch...
Is "killswitch" a word? Maybe "kill switch" would be better?
h) Several of the function one-line descriptions are a bit terse and could use "the" in them:
i) Technically, "e-mail" is not a countable noun. Use "e-mail messages" or "e-mail" here:
j) Callbacks http://drupal.org/node/1354#callbacks
So that line that says "Callback for ..." should be inside, and the line that says "Orders projects..." should be the one-line description.
k)
(and many other places) ... In general, please don't add data types for param/return to these API cleanup patches. It makes the patches take even longer to review. See the meta issue for details on what we do want fixed. Thanks!
l)
I'm not convinced that "A string containing" really adds any clarity to this documentation...
m) update.test
We are not having docblocks on getInfo(), setUp(), or tearDown() functions [not my choice but that is the standard]
Comment #5
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedThanks for the review!
a) Changed. I agree it sounds strange. It made me go "hmm..." too when I wrote it, but changing it to something along the lines that you suggested also made me go "hmm...", as I've always considered a module the product of many files, and describing the .module file as "the module" seems strange to me. But I've seen it used in many .module files so I'll go with the flow.
b) I agree. When I decided to go with "Update manager" I looked in the .info file to see what the modules name really was, and I saw it was "Update manager", but when I look at the name property in the .info file just now, I see that it's called "Update Manager". Not sure when that happened, or if I read it with tired eyes when I was working on the patch a week ago. Anyway, I've changed "Update manager" to "Update Manager" when it's in reference to the module and changed it to "update manager" when it's talking about the functionality. Also did the same with update.module.
c) Agreed.
d) Changed.
e) Not sure what you meant about verb tenses in this context, but I've gone ahead and changed the sentence to "Sets up a batch
tothat downloads, extracts and verifies the selected releases."To serial comma or not to serial comma, that is the question, uncertainty..., and Drupal preference. Following suit ;)
f) More concise. I like it.
g) Changed.
h) The two cases you mention there were a product of several minutes of word-shuffling to try to get them below 80 characters without losing important information. I can't figure out how to succeed in "un-tersifying" without going past 80 characters.
i) Ah, learned something new there. Makes sense.
j) Changed.
k) I added data types where I felt it was helpful to clarify, as the description did not reveal it. But if there is an effort in progress of adding them in one go I'll go ahead and remove the changes. Done!
l) I added those to clarify the data types, but I agree that it gets a bit repetitive without much benefit. Removed it.
m) Removed.
Comment #6
xjmSorry for the delay in reviewing this! I checked the interdiff against @jhodgdon's review above, and then reviewed the whole patch myself. Here's what I found:
Stray datatype. Also, either the comma should be replaced with a semicolon, or the word "or" should be added.
"OK" should at least be uppercase, but I think it would be better to replace it with "alright" or "acceptable" or some such.
Can we put quotes around 'Available updates' here? Otherwise it looks like the Available (updates report) rather than (Available updates) report.
Can we say "Determines" instead of "Figures out"? Personifying code always seems a bit odd, but I find this one particularly egregious. :)
Stray datatype.
Technically this is correct, but "updated update" made me blink. Can we say "current update status information" or something?
Let's omit the "This is complicated..." sentence; it's needless verbage. Also , the verb tenses through this paragraph are inconsistent.
"etc." is missing a period here. Also, the sentence actually does not make sense to me--maybe there should not be parentheses?
Stray datatype.
Stray datatype.
Either the comma should be a semicolon, or we need the word "or".
Stray datatype.
I had to read this sentence three times before I understood it. There should be a comma before "depending."
Serial comma please. :)
Stray added datatype here.
Minor point, but the subject of this sentence is the
$create
parameter, so the "attempt to create" sounds odd. I'd suggest:"Whether to attempt to create the directory if it does not already exist."
Fine point but I'd capitalized "Update Manager" here for clarity. (When it's used as an adjective it seems to be the title to me.) Just my opinion, though.
Minor point but the singular "their" here niggles. Can we simply omit it and say "to enter file transfer credentials"? (Also, should filetransfer be one word? I'm actually not sure so don't change it without looking it up.) :)
Comma splice. Let's make a second sentence here, e.g.: "Allowed values are 'foo', 'bar', and 'baz'.
Serial comma please. :)
Why are we removing this? Does it not actually throw the exception?
I think this change is actually confusing, because it sounds like the file is returning something, when the subject is actually the function itself. Maybe:
"Returns the local path if the file has already been downloaded."
Hm. I think it would be better to either explicitly spell out what the keys are in a list, or explicitly say "See update_get_projects() for the keys used." (and then make sure that function does in fact document them.)
I'd change these to say "when a security update is missing" and "when an update is missing".
Should this be "the Update Manager module"? Does that fit?
The word order here is a bit awkward in English. How about "Sets the version to 7.0 when no project-specific mapping is defined."?
This is really nitpicky, but I think "two" should be spelled out here (which affects the rewrapping). Questionable whether that's in scope, though.
As above; I think this should be "Update Manager pages."
Comment #7
jhodgdonRE item #17/28 in comment #6 -- we either need to call it "Update Manager" (if it's a proper name of this part of the Drupal system) or "update manager" (if it isn't). I'm OK with either one. But "Update manager" is not OK, as it's not the correct capitalization either way. Choose one and be consistent.
And if it's the name of a module, it should be "the Whatever Name module" for sure.
Comment #8
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedThanks for the reviews! They are much appreciated. I will get to them as soon as possible.
Comment #9
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedwould not aggrandize the patch, as the lines were touched anyway, but I will
remove them. I am a bit hesitant though, as it feels like lost work, as the
person that is going to add this data type in a future patch has to research
this again, but of course for the data types in question, not much research is
necessary ;)
Also, changed the commas to semi-colons. I'll make a mental note on this :)
manager" where I felt it was referring to the update manager functionality of
the Update Manager module and Update Manager where it was referring to the
module. In my opinion, in a lot of places this is a very fine line.
I've gone ahead and changed your first 2 suggestions to Update Manager, as I see
your point in them referring more to the update/install form of the Update
Manager module.
"filetransfer" gives 511,000 results on Google and "file transfer" gives
22,900,000 results. I'm not saying it's a good indicator for correctness to
follow the masses, but... ;) Wikipedia thinks it should be "file transfer". God,
I'm really not using good sources, am I? ;) Couldn't find an answer in various
dictionaries.
dpm(update_get_projects());
and having had alook at the array there and comparing them with the inline comments in
hook_update_projects_alter(), I have added the description as the @return of
update_get_projects(). I used "See update_get_projects() for the keys used." as
you suggested for update_create_fetch_task(). Thanks!
correctly what the method does. I suppose it is not important to say that it is
used to "reset", as the method should be independent from the callers. Your
suggestion works great.
Manager module.
Comment #10
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedJust thought of something. update_get_projects() only returns enabled projects, so the project_status key in the returned array will always be TRUE. Updated the @return doc to reflect this.
Comment #11
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedRerolling as #1329410: Extend update data collection to support localization update just got committed.
Comment #12
xjmThanks @Tor Arne Thune!
authorize.inc
stuff, but it appears we usefiletransfer
in code but "file transfer" in the docs. So that's fine.@throws
, I asked in IRC and everyone seems to agree we should use it in functional code too. :)I found just one thing in the interdiff:
This doesn't have subject/verb agreement (are scanned).
The latest patch looks great to me aside from that.
Comment #13
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedGood catch. Changed!
Also updated the patch for #1492188: Update module creates duplicate queue items that got committed.
Comment #15
Berdir#13: update-module-api-doc-cleanup-1431634-13-D8.patch queued for re-testing.
Comment #16
xjmWe're tracking the random test failures in #1549534: Random failures.
Comment #17
xjmAlright, that's all I've got! :)
Comment #18
jhodgdonWe are, as usual, over major/critical bug/task thresholds for Drupal 7/8. And there are three update.module issues in the critical/major CNR/RTBC queue. So, unless someone wants to volunteer to reroll all three pf those patches right afterwards, I won't be able to commit this patch any time soon. Sorry for the delay...
Comment #19
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedI'd be happy to reroll after. Was going to add a comment suggesting it. I've been keeping an eye on them anyway.
Comment #20
jhodgdonI'm discussing process revisions with catch, webchick, and Dries for when we're over thresholds, so I'm going to postpone this for a while anyway... I read through the first 1/4 of the patch though, and it all looks good so far!
Comment #21
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedNoting this down for myself:
Will reroll these after this patch has been committed:
#1484216: Race condition in _update_create_fetch_task() (PDO Exceptions) #21Got committed.#1183208: Remove variable_get('clean_url') and switch to index.php/path pattern for dirty URL support #94Got committed.#1036780: Drupal.org should collect stats on enabled sub-modules and core modulesGot committed.#1263040: No notifications sent when updates are available (followup and tests) #28
#952394: "No available releases found" - fetching update information exceeds timeout #85
Comment #22
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedRerolled after #1484216: Race condition in _update_create_fetch_task() (PDO Exceptions) got committed to D8.
Comment #23
webchickThis looks good. I'd like to hold it until after wednesday's release though so we dont' inadvertently break other patches.
Comment #24
jhodgdonre #23 - sounds reasonable! I'll plan to hit the "re-test" link on May 3rd (Thursday), give it a final review, and then commit to 8.x if there are no "avoid commit conflicts" tagged issues that conflict.
Comment #25
Tor Arne Thune CreditAttribution: Tor Arne Thune commented#1183208: Remove variable_get('clean_url') and switch to index.php/path pattern for dirty URL support and #1036780: Drupal.org should collect stats on enabled sub-modules and core modules were committed to D8, but patch still applies with git apply.
Comment #26
jhodgdonThe patch still applied. I applied it and went through all the files under modules/update and found a couple of things that we should fix:
a) update.authorize.inc around line 127:
This is in the middle of a function. It should be using // comments, not a /** */ doc block.
b) update.compare.inc around line 403:
This section should be reformatted somehow. It's definitely not going to render well on api.drupal.org as it is. Probably needs to be made into a list.
c) update.manager.inc is still inconsistently capitalizing "Update Manager" vs. "Update manager" vs. "update manager". Actually, this is bothering me enough to set the patch back to "needs work" (until I saw this, I was going to say we should just do the other fixes in a follow-up, but since this was discussed above, I think we should fix it now).
d) update.manager.inc around line 771:
Our standards just say to use the exception class name after @throws, so "on failure." should be omitted. See http://drupal.org/node/1354#classes
e) update.manager.inc around line 800
Extra blank line at end of docblock.
f) update.manager.inc around line 818
Keeping in mind that function one-line descriptions are displayed on pages that don't contain the whole function doc, if we can avoid using $url (a parameter variable) in the description, that would be preferable.
g) CSS files do not have @file docblocks at the top.
h) update.module around line 253
Needs a "the" in there.
i) update.module around line 361
Inaccurate description. This function returns the message, and does not print it anywhere.
j) top of update.test
I don't think we need that first sentence, since it duplicates the one-line description. :)
k) All through update.test, the module is referred to as "the update module". It should be referred to as "the Update Manager module". I think we should fix it in the getInfo() descriptions as well as in the doc headers.
Comment #27
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedThank you! For what it's worth I prefer this issue to be set to Needs review rather than opening many little follow-up issues later.
a) Hmm, yes. Didn't want to touch things inside functions, but maybe I have done
so? Anyway, I've changed it to use the inline comment style.
b) You are of course right about it not rendering right on api.d.o. Changed it
to list form.
c) I found one place where the text was referring to the Update Manager module and
not the update manager functionality of the module (the summary of the @file
directive). I also changed Update manager to Update Manager in the @defgroups.
The other places I feel are rightly cased. (Update Manager when referring to
the module, update manager where referring to the update manager functionality
of the module.)
d) Changed.
e) Removed.
f) Rephrased.
g) Ah, wasn't aware the API docs initiative touched those as there is a "Clean up
CSS" initiative too. Makes sense though. Added @file blocks to the two CSS
files.
h) I remember rephrasing this summary many times to make it fit on one line. I've
added "the" and changed "Determines" to "Resolves" as the line was 81
characters.
i) Changed.
j) I agree. Changed.
k) Changed in the doc blocks, one comment in a test function and the getInfo
descriptions.
Comment #28
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedRe-rolled because of #1541676: Convert Simpletest base test classes to PSR-0.
Comment #29
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedRe-rolled because of #1594256: Convert update tests to PSR-0.
Comment #30
jhodgdonThis looks mostly excellent, thanks! I found these few items to fix:
a) core/modules/update/lib/Drupal/update/Tests/UpdateTestBase.php
This is no longer accurate (not the fault of this patch!). The test XML files are in core/modules/update/tests, and the test class files have moved, so I think we need to say "(in the core/modules/update/tests directory)" instead.
b) The documentation in update.compare.inc for update_calculate_project_update_status() was partially updated to change "It does..." to "... is done". But the update was not complete, for instance:
c) There's also a grammar problem in update.manager.inc in the update_manager_update_ready_form() docs:
gives -> give, asks -> ask.
d) It looks like we decided on no-capitals for "update manager" when referring to the system, as opposed to "the Update Manager module". This is still not quite consistent, however:
Basically, if "update manager" doesn't have "module" after it, it shouldn't be capitalized.
Comment #31
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedThank you for continuing with this! The interdiff speaks for itself.
Comment #32
jhodgdonRE (b) - the wording in there is still a bit awkward, but at least it's using consistent tenses... I think this is OK to commit. I'll take care of that shortly. Thanks!
Comment #33
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedThanks for the RTBC. This patch will conflict with #1463656: Add a Drupal kernel; leverage HttpFoundation and HttpKernel, so it should probably wait until that is committed. It's tagged with "Avoid commit conflicts," so you probably have it in your sight. Just thought I would mention it to save you some work.
Comment #34
jhodgdonAgreed, this needs to wait on that issue. Thanks for the heads up!
Comment #35
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedRe-rolled after #1463656: Add a Drupal kernel; leverage HttpFoundation and HttpKernel got merged. I had to add another @return directive, as the kernel patch added a return to update_test_mock_page(). See interdiff.
Comment #36
jhodgdonThanks! That looks fine.
There aren't any "avoid commit conflicts" issues this conflicts with now, so I'll try and get it committed early next week (unless someone comes up with another issue to avoid conflicts with -- this patch touches 17 (!) files in the update module, so I think it qualifies as "large"... so I'll leave it at RTBC a few days before the commit).
Comment #37
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedWhile working on the backport I found a missing '(optional)'.
Comment #38
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedAnd here's the D7 backport of the D8 patch in #37.
What I did was use a diff viewer to visualize the differences between the files of the update module for D7 and D8. The changes that would have an impact on the docblocks changed by this issue were incorporated into the backport.
Modifications done during the D7 backport:
update.test:
update_test.module:
update.api.php:
(Function made public in D8.)
update.authorize.inc:
update.compare.inc:
(This parameter was added in D8.)
(This parameter was removed in D8 because it was superfluous. See comment #2.)
(This parameter was added in D8.)
update.module:
(D8 no longer use these functions, so the doc was generalized to "cache API" for D8 in another issue)
Comment #39
jhodgdonThanks! I've committed the patch in #37 to Drupal 8.x!!
I haven't yet reviewed the patch for 7.x, but the approach in the previous comment looks correct.
Comment #40
jhodgdonThanks -- the 7.x patch looks good to me! I'm re-uploading it just to make sure that it doesn't have any syntax problems I might have missed. Once it passes all the tests, I plan to commit it in a few days (probably June 15). There do not appear to be any "avoid commit conflicts" issues it will conflict with.
Comment #41
jhodgdonCommitted to 7.x -- thanks!!!
Comment #42
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedGreat! Thanks for the teamwork :)