CommentFileSizeAuthor
#40 update-module-api-doc-cleanup-1431634-38-D7.patch92.68 KBjhodgdon
#38 update-module-api-doc-cleanup-1431634-38-D7-do-not-test.patch92.68 KBTor Arne Thune
#37 update-module-api-doc-cleanup-1431634-37-D8.patch95 KBTor Arne Thune
#37 interdiff-1431634-35-37.txt713 bytesTor Arne Thune
#35 update-module-api-doc-cleanup-1431634-35-D8.patch94.99 KBTor Arne Thune
#35 interdiff-1431634-31-35.txt774 bytesTor Arne Thune
#31 update-module-api-doc-cleanup-1431634-31-D8.patch94.68 KBTor Arne Thune
#31 interdiff-1431634-29-31.txt6.82 KBTor Arne Thune
#29 update-module-api-doc-cleanup-1431634-29-D8.patch94.49 KBTor Arne Thune
#28 update-module-api-doc-cleanup-1431634-28-D8.patch93.34 KBTor Arne Thune
#27 update-module-api-doc-cleanup-1431634-27-D8.patch93.29 KBTor Arne Thune
#27 interdiff-1431634-22-27.txt10.3 KBTor Arne Thune
#22 update-module-api-doc-cleanup-1431634-22-D8.patch87.7 KBTor Arne Thune
#13 update-module-api-doc-cleanup-1431634-13-D8.patch87.7 KBTor Arne Thune
#13 interdiff-1431634-11-13.txt762 bytesTor Arne Thune
#11 update-module-api-doc-cleanup-1431634-11-D8.patch87.67 KBTor Arne Thune
#11 interdiff-10-11.txt1.72 KBTor Arne Thune
#10 update-module-api-doc-cleanup-1431634-10-D8.patch87.21 KBTor Arne Thune
#10 interdiff-9-10.txt717 bytesTor Arne Thune
#9 update-module-api-doc-cleanup-1431634-9-D8.patch87.14 KBTor Arne Thune
#9 interdiff-5-9.txt15.38 KBTor Arne Thune
#5 update-module-api-doc-cleanup-1431634-5-D8.patch85.82 KBTor Arne Thune
#5 interdiff-3-5.txt18.41 KBTor Arne Thune
#3 update-module-api-doc-cleanup-1431634-3-D8.patch88.64 KBTor Arne Thune
#3 interdiff-2-3.txt1.46 KBTor Arne Thune
#2 update-module-api-doc-cleanup-1431634-2-D8.patch87.85 KBTor Arne Thune
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Tor Arne Thune’s picture

Assigned: Unassigned » Tor Arne Thune

Working on this.

Tor Arne Thune’s picture

Status: Active » Needs review
FileSize
87.85 KB

Voilà. 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:

 * @param $project
 *   An array containing information about a specific project.
 * @param $project_data
 *   An array containing information about a specific project.

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.

jhodgdon’s picture

Status: Needs review » Needs work

Thanks! This looks pretty good. A few things I would fix:

a)

+++ b/core/modules/update/tests/update_test.module
@@ -1,6 +1,11 @@
 <?php
 
 /**
+ * @file
+ * The module file of the module used to test Update manager functionality.
+ */
+

"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

+ * interaction with the Update manager module: mere mortals need not apply. The

Can we get rid of this "mere mortals" thing? It is very condescending.

d) and a few lines down:

+ * primary use-case for this hook is to add projects to the list, for example,
...
+ * module might want to hide projects from the list, for example, if there is a

should be ; before "for example".

e) in update.manager.inc

@@ -297,11 +303,11 @@ function update_manager_update_form_validate($form, &$form_state) {
 }
 
 /**
- * Submit function for the main update form.
+ * Form submission handler for update_manager_update_form().
  *
- * This sets up a batch to download, extract and verify the selected releases
+ * Sets up a batch to download, extract and verify the selected releases.

- Verb tenses do not agree in that last line.
- Need comma before "and" in the list in the last line.

f)

+++ b/core/modules/update/update.module
@@ -2,10 +2,13 @@
 
 /**
  * @file
- * The "Update status" module checks for available updates of Drupal core and
- * any installed contributed modules and themes. It warns site administrators
- * if newer releases are available via the system status report
- * (admin/reports/status), the module and theme pages, and optionally via email.
+ * The Update manager module handles updates of Drupal and contributed projects.

I think you can leave out "The update manager module" in this?

g) a few lines down in the patch...

+ * killswitch for...

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:

+ * Tries to get update information from cache and refreshes it when necessary.
...
+ * Returns the appropriate message text when site is out of date or not secure.

i) Technically, "e-mail" is not a countable noun. Use "e-mail messages" or "e-mail" here:

+ * notification e-mails generated by update_cron().

j) Callbacks http://drupal.org/node/1354#callbacks

@@ -603,10 +621,9 @@ function _update_message_text($msg_type, $msg_reason, $report_link = FALSE, $lan
 }
 
 /**
- * Private sort function to order projects based on their status.
+ * Callback for uasort() within update_requirements().
  *
- * @see update_requirements()
- * @see uasort()
+ * Orders projects based on their status.
  */
 function _update_project_status_sort($a, $b) {

So that line that says "Callback for ..." should be inside, and the line that says "Orders projects..." should be the one-line description.

k)

- * @param $data
+ * @param array $data

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

@@ -284,11 +287,11 @@ function theme_update_status_label($variables) {
  * @param array $variables
  *   An associative array containing:
  *   - version: An array of data about the latest released version, containing:
- *     - version: The version number.
- *     - release_link: The URL for the release notes.
- *     - date: The date of the release.
- *     - download_link: The URL for the downloadable file.
- *   - tag: The title of the project.
+ *     - version: A string containing the version number.
+ *     - release_link: A string containing the URL for the release notes.
+ *     - date: A string containing the date of the release.
+ *     - download_link: A string containing the URL for the downloadable file.
+ *   - tag: A string containing the title of the project.

I'm not convinced that "A string containing" really adds any clarity to this documentation...

m) update.test

+  /**
+   * Returns general information about the tests in this class.
+   */
   public static function getInfo() {

We are not having docblocks on getInfo(), setUp(), or tearDown() functions [not my choice but that is the standard]

Tor Arne Thune’s picture

Status: Needs work » Needs review
FileSize
18.41 KB
85.82 KB

Thanks 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 to that 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.

xjm’s picture

Status: Needs review » Needs work

Sorry 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:

  1. +++ b/core/modules/update/update.authorize.incundefined
    @@ -168,11 +175,16 @@ function update_authorize_batch_copy_project($project, $updater_name, $local_url
    + * @param bool $success
    + *   TRUE if the batch operation was successful, FALSE if there were errors.
    
    @@ -230,11 +242,16 @@ function update_authorize_update_batch_finished($success, $results) {
    + * @param bool $success
    + *   TRUE if the batch operation was a success, FALSE if there were errors.
    

    Stray datatype. Also, either the comma should be replaced with a semicolon, or the word "or" should be added.

  2. +++ b/core/modules/update/update.compare.incundefined
    @@ -15,14 +15,17 @@
    + * available updates fetched from the network, it is ok to invalidate it
    

    "OK" should at least be uppercase, but I think it would be better to replace it with "alright" or "acceptable" or some such.

  3. +++ b/core/modules/update/update.compare.incundefined
    @@ -53,25 +56,25 @@ function update_get_projects() {
    + * should be included or not in the Available updates report), those are only
    

    Can we put quotes around 'Available updates' here? Otherwise it looks like the Available (updates report) rather than (Available updates) report.

  4. +++ b/core/modules/update/update.compare.incundefined
    @@ -211,8 +214,13 @@ function _update_process_info_list(&$projects, $list, $project_type, $status) {
    + * Figures out what project a given file object belongs to.
    

    Can we say "Determines" instead of "Figures out"? Personifying code always seems a bit odd, but I find this one particularly egregious. :)

  5. +++ b/core/modules/update/update.compare.incundefined
    @@ -211,8 +214,13 @@ function _update_process_info_list(&$projects, $list, $project_type, $status) {
    + * @return string
    + *   The canonical project short name.
    

    Stray datatype.

  6. +++ b/core/modules/update/update.compare.incundefined
    @@ -285,13 +295,16 @@ function update_process_project_info(&$projects) {
    + *   An array of installed projects with updated update status information.
    

    Technically this is correct, but "updated update" made me blink. Can we say "current update status information" or something?

  7. +++ b/core/modules/update/update.compare.incundefined
    @@ -327,32 +340,30 @@ function update_calculate_project_data($available) {
    + * snapshots if possible). This is complicated to describe, but an example will
    + * help clarify. For the target major version, find the highest patch level. If
    

    Let's omit the "This is complicated..." sentence; it's needless verbage. Also , the verb tenses through this paragraph are inconsistent.

  8. +++ b/core/modules/update/update.compare.incundefined
    @@ -327,32 +340,30 @@ function update_calculate_project_data($available) {
    + * there is a release at that patch level with no extra ("beta", etc), then we
    

    "etc." is missing a period here. Also, the sentence actually does not make sense to me--maybe there should not be parentheses?

  9. +++ b/core/modules/update/update.compare.incundefined
    @@ -381,9 +391,13 @@ function update_calculate_project_data($available) {
    + * @param array $available
    + *   Data about available project releases of a specific project.
    

    Stray datatype.

  10. +++ b/core/modules/update/update.fetch.incundefined
    @@ -25,7 +29,10 @@ function update_manual_status() {
    + * @param array $context
    + *   Reference to an array used for Batch API storage.
    

    Stray datatype.

  11. +++ b/core/modules/update/update.fetch.incundefined
    @@ -70,12 +77,12 @@ function update_fetch_data_batch(&$context) {
    + *   TRUE if the batch operation was successful, FALSE if there were errors.
    

    Either the comma should be a semicolon, or we need the word "or".

  12. +++ b/core/modules/update/update.fetch.incundefined
    @@ -250,14 +258,17 @@ function _update_create_fetch_task($project) {
    + * @return string
    + *   The URL for fetching information about updates to the specified project.
    

    Stray datatype.

  13. +++ b/core/modules/update/update.fetch.incundefined
    @@ -299,10 +311,10 @@ function _update_get_fetch_url_base($project) {
    - * This method checks the status of the site using the new data and depending
    - * on the configuration of the site, notifies administrators via email if there
    + * This method checks the status of the site using the new data and depending on
    + * the configuration of the site, notifies administrators via e-mail if there
      * are new releases or missing security updates.
    

    I had to read this sentence three times before I understood it. There should be a comma before "depending."

  14. +++ b/core/modules/update/update.installundefined
    @@ -2,26 +2,25 @@
    + * Install, update and uninstall functions for the Update Manager module.
    

    Serial comma please. :)

  15. +++ b/core/modules/update/update.manager.incundefined
    @@ -331,7 +337,12 @@ function update_manager_update_form_submit($form, &$form_state) {
    + * @param bool $success
    + *   TRUE if the batch operation was successful, FALSE if there were errors.
    

    Stray added datatype here.

  16. +++ b/core/modules/update/update.moduleundefined
    @@ -872,14 +897,15 @@ function _update_manager_unique_identifier() {
    + *   (optional) If TRUE, attempt to create the directory if it does not already
    + *   exist. Defaults to TRUE.
    
    @@ -893,14 +919,15 @@ function _update_manager_extract_directory($create = TRUE) {
    + *   (optional) If TRUE, attempt to create the directory if it does not already
    + *   exist. Defaults to TRUE.
    

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

  17. +++ b/core/modules/update/update.manager.incundefined
    @@ -47,7 +47,7 @@
    - * Form constructor for the Update manager update form.
    + * Form constructor for the update manager update form.
    
    @@ -481,7 +481,7 @@
    - * Form constructor for the Update manager install form.
    + * Form constructor for the update manager install form.
    
    @@ -544,11 +544,11 @@
    - *   The Update manager operation we're in the middle of. Can be either 'update'
    + *   The update manager operation we're in the middle of. Can be either 'update'
    
    +++ b/core/modules/update/update.moduleundefined
    @@ -958,7 +958,7 @@
    - * Deletes stale files and directories from the Update manager disk cache.
    + * Deletes stale files and directories from the update manager disk cache.
    

    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.

  18. +++ b/core/modules/update/update.manager.incundefined
    @@ -354,15 +365,21 @@ function update_manager_download_batch_finished($success, $results) {
    + * the user is redirected to authorize.php to enter their file transfer
    

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

  19. +++ b/core/modules/update/update.manager.incundefined
    @@ -465,18 +481,19 @@ function update_manager_update_ready_form_submit($form, &$form_state) {
    + *   String representing the context from which we're trying to install, can be:
    + *   'module', 'theme' or 'report'.
    

    Comma splice. Let's make a second sentence here, e.g.: "Allowed values are 'foo', 'bar', and 'baz'.

  20. +++ b/core/modules/update/update.manager.incundefined
    @@ -729,22 +749,20 @@ function update_manager_install_form_submit($form, &$form_state) {
    + * These functions are used by the update manager to copy, extract and verify
    + * archive files.
    

    Serial comma please. :)

  21. +++ b/core/modules/update/update.manager.incundefined
    @@ -729,22 +749,20 @@ function update_manager_install_form_submit($form, &$form_state) {
    - * @throws Exception on failure.
    

    Why are we removing this? Does it not actually throw the exception?

  22. +++ b/core/modules/update/update.manager.incundefined
    @@ -795,7 +813,7 @@ function update_manager_archive_verify($project, $archive_file, $directory) {
    - * If the file has already been downloaded, returns the the local path.
    + * If the file has already been downloaded, it returns the local path.
    

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

  23. +++ b/core/modules/update/update.moduleundefined
    @@ -428,7 +438,12 @@ function update_get_available($refresh = FALSE) {
    + * @param $project
    + *   Associative array of information about a project as created by
    + *   update_get_projects(), including keys such as 'name' (short name), and the
    + *   'info' array with data from a .info file for the project.
    

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

  24. +++ b/core/modules/update/update.testundefined
    @@ -164,7 +171,7 @@ class UpdateCoreTestCase extends UpdateTestHelper {
    -   * Check the messages at admin/modules when missing an update.
    +   * Checks the messages at admin/modules when missing an update.
    
    @@ -181,7 +188,7 @@ class UpdateCoreTestCase extends UpdateTestHelper {
    -   * Check the messages at admin/modules when missing a security update.
    +   * Checks the messages at admin/modules when missing a security update.
    

    I'd change these to say "when a security update is missing" and "when an update is missing".

  25. +++ b/core/modules/update/update.testundefined
    @@ -216,7 +223,7 @@ class UpdateCoreTestCase extends UpdateTestHelper {
    -   * Tests the update module when the update server returns 503 (Service unavailable) errors.
    +   * Tests the update module when the update server returns 503 errors.
    

    Should this be "the Update Manager module"? Does that fit?

  26. +++ b/core/modules/update/update.testundefined
    @@ -225,6 +232,9 @@ class UpdateCoreTestCase extends UpdateTestHelper {
    +   * Resets to 7.0 the version used when no project-specific mapping is defined.
    

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

  27. +++ b/core/modules/update/update.testundefined
    @@ -309,17 +322,17 @@ class UpdateTestContribCase extends UpdateTestHelper {
    +   * names of the modules included in each project. In this test case, we have 2
    

    This is really nitpicky, but I think "two" should be spelled out here (which affects the rewrapping). Questionable whether that's in scope, though.

  28. +++ b/core/modules/update/update.testundefined
    @@ -657,7 +673,7 @@ class UpdateTestUploadCase extends UpdateTestHelper {
    -   * Check the messages on Update manager pages when missing a security update.
    +   * Checks the messages on update manager pages when missing a security update.
    

    As above; I think this should be "Update Manager pages."

jhodgdon’s picture

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

Tor Arne Thune’s picture

Thanks for the reviews! They are much appreciated. I will get to them as soon as possible.

Tor Arne Thune’s picture

Status: Needs work » Needs review
FileSize
15.38 KB
87.14 KB
  1. I left in the datatypes I researched when adding @params and @returns, as that
    would 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 :)
  2. Acceptable works great. Thanks!
  3. Agreed. It crossed my mind too.
  4. Sure.
  5. Removed.
  6. That works.
  7. Rewrote the paragraph.
  8. Added. It makes sense in the context, but not sure how to put it in other words.
  9. Removed.
  10. Removed.
  11. Sure.
  12. Removed.
  13. Added.
  14. That is the question.
  15. Removed.
  16. Sounds good.
  17. Following jhodgdon's review in #4 I went ahead and changed it to "update
    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.
  18. Sounds good with or without to me so removed it.
    "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.
  19. Sure thing.
  20. Added.
  21. I thought @throws was just for classes and such. Re-added it!
  22. I see your point. Sounds good.
  23. After having executed dpm(update_get_projects()); and having had a
    look 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!
  24. Sure thing.
  25. Oh yes, had missed that one. And it fits as well!
  26. I remember changing this sentence so many times to make it fit and still explain
    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.
  27. Changed.
  28. In my opinion this refers to the update manager functionality of the Update
    Manager module.
Tor Arne Thune’s picture

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

Tor Arne Thune’s picture

xjm’s picture

Status: Needs review » Needs work

Thanks @Tor Arne Thune!

  • Regarding the datatypes, I can definitely see where you're coming from, but I want to make these patches less painful for committer review as much as possible. :) My strategy would be to document the types you had in the "remaining tasks" of this issue's summary, or even file a followup with those lines from your original patch.
  • Regarding file transfer/filetransfer, I was thinking of all the authorize.inc stuff, but it appears we use filetransfer in code but "file transfer" in the docs. So that's fine.
  • Regarding @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:

+++ b/core/modules/update/update.compare.incundefined
@@ -356,14 +377,14 @@
+ * Given a target major version, the available releases is scanned looking for
+ * the specific release to recommend (avoiding beta releases and development

This doesn't have subject/verb agreement (are scanned).

The latest patch looks great to me aside from that.

Tor Arne Thune’s picture

Status: Needs work » Needs review
FileSize
762 bytes
87.7 KB

Good catch. Changed!
Also updated the patch for #1492188: Update module creates duplicate queue items that got committed.

Status: Needs review » Needs work
Issue tags: -Needs backport to D7, -docs-cleanup-2011

The last submitted patch, update-module-api-doc-cleanup-1431634-13-D8.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
Issue tags: +Needs backport to D7, +docs-cleanup-2011
xjm’s picture

We're tracking the random test failures in #1549534: Random failures.

xjm’s picture

Status: Needs review » Reviewed & tested by the community

Alright, that's all I've got! :)

jhodgdon’s picture

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

Tor Arne Thune’s picture

I'd be happy to reroll after. Was going to add a comment suggesting it. I've been keeping an eye on them anyway.

jhodgdon’s picture

I'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!

Tor Arne Thune’s picture

webchick’s picture

This looks good. I'd like to hold it until after wednesday's release though so we dont' inadvertently break other patches.

jhodgdon’s picture

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

Tor Arne Thune’s picture

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs work

The 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:

  /**
   * The batch API uses a session, and since all the arguments are serialized
   * and unserialized between requests, although the FileTransfer object
   * itself will be reconstructed, the connection pointer itself will be lost.
   * However, the FileTransfer object will still have the connection variable,
   * even though the connection itself is now gone. So, although it's ugly, we
   * have to unset the connection variable at this point so that the
   * FileTransfer object will re-initiate the actual connection.
   */
  unset($filetransfer->connection);

This is in the middle of a function. It should be using // comments, not a /** */ doc block.

b) update.compare.inc around line 403:

 * example:
 *
 * 1.6-bugfix <-- recommended version because 1.6 already exists.
 * 1.6
 *
 * or
 *
 * 1.6-beta
 * 1.5 <-- recommended version because no 1.6 exists.
 * 1.4
 *
 * It also looks for the latest release from the same major version, even a beta

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:

 *
 * @throws Exception on failure.
 */
function update_manager_archive_extract($file, $directory) {

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

 * @return array
 *   An array of error messages to display if the archive was invalid. If there
 *   are no errors, it will be an empty array.
 *
 */
function update_manager_archive_verify($project, $archive_file, $directory) {
 

Extra blank line at end of docblock.

f) update.manager.inc around line 818

/**
 * Copies a file from $url to the temporary directory for updates.

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

/**
 * Access callback: Determines if current user can access updater menu items.
 

Needs a "the" in there.

i) update.module around line 361

/**
 * Prints a warning message when there is no data about available updates.
 */
function _update_no_data() {
 

Inaccurate description. This function returns the message, and does not print it anywhere.

j) top of update.test

/**
 * @file
 * Tests for the Update Manager module.
 *
 * This file contains tests for the Update Manager module. The overarching
 

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.

Tor Arne Thune’s picture

Status: Needs work » Needs review
FileSize
10.3 KB
93.29 KB

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

Tor Arne Thune’s picture

Tor Arne Thune’s picture

jhodgdon’s picture

Status: Needs review » Needs work

This looks mostly excellent, thanks! I found these few items to fix:

a) core/modules/update/lib/Drupal/update/Tests/UpdateTestBase.php

+ * files we fetch say is available. We have dummy XML files (in the 'tests'
+ * subdirectory) that describe various scenarios of what's available for

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:

// good:
- * Given a target major version, it scans the available releases looking for
+ * Given a target major version, the available releases are scanned looking for
...
// not fixed:
+ * It also looks for the latest release from the same major version, even a beta
+ * release, to display to the user as the "Latest version" option. Additionally,
+ * it finds the latest official release from any higher major versions that have
+ * been released to provide a set of "Also available" options.
...
+ * Finally, and most importantly, it keeps scanning the release history until it
+ * gets to the currently installed release, searching for anything marked as a

c) There's also a grammar problem in update.manager.inc in the update_manager_update_ready_form() docs:

 ... The point of this page is to encourage the user to
+ * backup their site, gives them the opportunity to put the site offline, and
+ * then asks them to confirm that the update should continue. ...

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:

// update.manager.inc
+ * @defgroup update_manager_update Update Manager: update
...
+ * Update Manager for updating existing code.
...
+ * Form constructor for the Update Manager update form.
...
+ * @defgroup update_manager_install Update Manager: install
...
+ * Update Manager for installing new code.
...
+ * Form constructor for the Update Manager install form.
...
+ * @defgroup update_manager_file Update Manager: file management
etc.

Basically, if "update manager" doesn't have "module" after it, it shouldn't be capitalized.

Tor Arne Thune’s picture

Status: Needs work » Needs review
FileSize
6.82 KB
94.68 KB

Thank you for continuing with this! The interdiff speaks for itself.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

RE (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!

Tor Arne Thune’s picture

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

jhodgdon’s picture

Agreed, this needs to wait on that issue. Thanks for the heads up!

Tor Arne Thune’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
774 bytes
94.99 KB

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

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

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

Tor Arne Thune’s picture

While working on the backport I found a missing '(optional)'.

Tor Arne Thune’s picture

And 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:

- * core/modules/update/tests directory) that describe various scenarios of
+ * modules/update/tests directory) that describe various scenarios of what's

update_test.module:

- * @return StreamedResponse|Response
- *   A StreamedResponse object containing the content of the XML release file
- *   for the specified project if one is available; a Response object with no
- *   content otherwise.

update.api.php:

- *   update_process_info_list(), so look there for examples of how to populate
+ *   _update_process_info_list(), so look there for examples of how to populate

(Function made public in D8.)

update.authorize.inc:

- *   - updater_name: The name of the Drupal\Core\Updater\Updater class to use
+ *   - updater_name: The name of the Updater class to use for this project.

update.compare.inc:

- *   (optional) Array of additional elements to be collected from the .info
- *   file. Defaults to array().

(This parameter was added in D8.)

+ * @param $project
+ *   An array containing information about a specific project.

(This parameter was removed in D8 because it was superfluous. See comment #2.)

- *   (optional) Array of additional elements to be collected from the .info
- *   file. Defaults to array().

(This parameter was added in D8.)

update.module:

- * using the cache API, there are private helper functions that implement these
+ * using cache_set(), cache_get(), and cache_clear_all(), there are private

(D8 no longer use these functions, so the doc was generalized to "cache API" for D8 in another issue)

jhodgdon’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Needs review

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

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
92.68 KB

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

jhodgdon’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 7.x -- thanks!!!

Tor Arne Thune’s picture

Great! Thanks for the teamwork :)

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