Add missing type hinting to Update module docblocks

Thanks for this patch @bleen18. From previous experience with wotking with @jhodgdon on these types of documentation issues, she only wants to commit changes that are between /** and */ in issues such as these. Hence, my recommendation would be to split your patch into two parts.

The changes to _update_create_fetch_task(), _update_refresh() and _update_fetch_data() (along with the proposed changes to their docblocks) should go into a new issue to be posted in the Update.module queue. The rest of the patch then should be re-rolled for here.

@bleen 18 The patch in #4 is for Tracker module, not the Update module. Looks like you uploaded the wrong patch to this issue.

@bleen18 I am having a problem getting the new patch like in the other issue. When I try to retrieve 1811202.patch using curl, I am getting the first one (i.e. #1), not the version uploaded in #7.

This common patch name problem is why it is the Drupal 'best practice' to try to create files with unique names like '[issue #]-[comment #]-th-update.patch'. Could you upload #7 again using a unique name so that I can get it download? Thanks in advance.

Ah thanks @bleen18... I did not realize that. I was using simply what shown on the green bar.

How did you get the actual link so I do not bother you again with this type of issue?

Thanks for rolling this patch @bleen18! I am sorry about my confusion in getting this patch down to my local machine.

Here is the start of another nit-picky review. Again, I am focused on making sure we get a resulting RTBC patch that is ready to be committed so that we minimize any push back there, since these type of patches take much time and attention to detail to review correctly and completely.

First, I noted that there are several places in this module where #1487760: [policy, no patch] Decide on documentation standards for namespaced items applies:
- MockFileTransfer.php,
- update.authorize.inc, and
- update.manager.inc.

With this patch locally applied, I see that we are missing many @param type hints:
- two in update.api.php,
- numerous in update.authorize.inc,
- thirteen more in update.compare.inc,
- nine more in update.fetch.inc,
- two in update.install,
- six in update.manager.inc,
- none in update.module, and
- none in update.report.inc.

Additionally, with this patch applied locally, there are missing type hints in the following files:
- one in update.api.php,
- five in update.compare.inc,
- four in update.fetch.inc,
- two in update.install,
- two in update.manager.inc, and
- none in update.module.

Also, checked and with this applied there are no '(Optional)' capitalization issues in this patch.

I am going to save this here and continue with the detailed dreditor comments/review of the patch in the next comment.

Below are my detail notes from reviewing the patch in #7:

+++ b/core/modules/update/update.moduleundefined
@@ -244,7 +244,7 @@ function update_menu() {
- * @return
+ * @return bool
  *   TRUE if the current user can access the updater menu items; FALSE

This type hint is correct in update_manager_access().

Looks like we are missing a @return directive for _update_no_data().

+++ b/core/modules/update/update.moduleundefined
@@ -372,11 +372,11 @@ function _update_no_data() {
  *
- * @param $refresh
+ * @param bool $refresh
  *   (optional) Boolean to indicate if this method should refresh the cache
  *   automatically if there's no data. Defaults to FALSE.

This type hint addition is correct as is the @return type hint for update_get_available().

+++ b/core/modules/update/update.moduleundefined
@@ -436,7 +436,7 @@ function update_get_available($refresh = FALSE) {
  *
- * @param $project
+ * @param array $project
  *   Associative array of information about a project. See update_get_projects()
  *   for the keys used.

This type hint is correct in update_create_fetch_task().

We should add array to the function declaration in whatever coding fix issue we use to add array to function declarations.

+++ b/core/modules/update/update.moduleundefined
@@ -470,7 +470,7 @@ function update_fetch_data() {
  *
- * @return
+ * @return array
  *   Array of data about available releases, keyed by project shortname.

This type hint addition for _update_get_cached_available_releases() is correct.

+++ b/core/modules/update/update.moduleundefined
@@ -495,11 +495,11 @@ function _update_get_cached_available_releases() {
  *
- * @param $key
+ * @param string $key
  *   Unique key to indicate what message to build, always 'status_notify'.
- * @param $message
+ * @param array $message
  *   Reference to the message array being built.
- * @param $params
+ * @param array $params
  *   Array of parameters to indicate what kind of text to include in the message
  *   body. This is a keyed array of message type ('core' or 'contrib') as the

All of these type hints for update_mail() are correct.

+++ b/core/modules/update/update.moduleundefined
@@ -536,18 +536,18 @@ function update_mail($key, &$message, $params) {
  *
- * @param $msg_type
+ * @param string $msg_type
  *   String to indicate what kind of message to generate. Can be either 'core'
  *   or 'contrib'.
- * @param $msg_reason
+ * @param int $msg_reason
  *   Integer constant specifying why message is generated.
- * @param $report_link
+ * @param bool $report_link
  *   (optional) Boolean that controls if a link to the updates report should be
  *   added. Defaults to FALSE.

These type hints in _update_message_text() are correct.

+++ b/core/modules/update/update.moduleundefined
@@ -536,18 +536,18 @@ function update_mail($key, &$message, $params) {
- * @param $langcode
+ * @param string $langcode
  *   (optional) A language code to use. Defaults to NULL.

This type hint should be 'string|null' at minimum. Have we double checked that the $langcode is a string and not an integer. I get confused by the language constant definitions.

+++ b/core/modules/update/update.moduleundefined
@@ -536,18 +536,18 @@ function update_mail($key, &$message, $params) {
- * @return
+ * @return string
  *   The properly translated error message for the given key.

This type hint is correct.

+++ b/core/modules/update/update.moduleundefined
@@ -623,6 +623,17 @@ function _update_message_text($msg_type, $msg_reason, $report_link = FALSE, $lan
+ *
+ * @param array $a
+ *   Associative array of information about a project. See update_get_projects()
+ *   for the keys used.
+ * @param array $b
+ *   Associative array of information about a project. See update_get_projects()
+ *   for the keys used.
+ *

My understanding is that helper functions for uasort() functions do not require @param or @return directives.

+++ b/core/modules/update/update.moduleundefined
@@ -641,10 +652,13 @@ function _update_project_status_sort($a, $b) {
  *
+ * @return string
+ *   Rendered HTML for the last time we checked for update data.

I think I mentioned it elsewhere, but let me repeat. theme() functions should not include a @return directive according to [#1354].

+++ b/core/modules/update/update.moduleundefined
@@ -748,11 +762,11 @@ function update_verify_update_archive($project, $archive_file, $directory) {
- * @param $cid
+ * @param string $cid
  *   The cache ID to save the data with.
- * @param $data
+ * @param mixed $data
  *   The data to store.
- * @param $expire
+ * @param int $expire
  *   One of the following values:
  *   - CacheBackendInterface::CACHE_PERMANENT: Indicates that the item should

All three of these type hints for _update_cache_set() are correct.

+++ b/core/modules/update/update.moduleundefined
@@ -807,7 +821,7 @@ function _update_cache_get($cid) {
  *
- * @return
+ * @return array
  *   Associative array of cache items, keyed by cache ID.

I think this should be 'array|false' since db_query() can return FALSE if the $cid value was not found.

+++ b/core/modules/update/update.moduleundefined
@@ -830,11 +844,11 @@ function _update_get_cache_multiple($cid_prefix) {
  *
- * @param $cid
+ * @param string $cid
  *   (optional) Cache ID of the record to clear from the private update module
  *   cache. If empty, all records will be cleared from the table except fetch
  *   tasks. Defaults to NULL.
- * @param $wildcard
+ * @param bool $wildcard
  *   (optional) If TRUE, cache IDs starting with $cid are deleted in addition to
  *   the exact cache ID specified by $cid. Defaults to FALSE.

This first one is incorrect. It should be 'string|null'. The second type hint is correct. What do you think?

+++ b/core/modules/update/update.moduleundefined
@@ -886,7 +900,7 @@ function update_cache_flush() {
- * @return
+ * @return string
  *   An eight character string uniquely identifying this Drupal installation.

This type hint is correct.

+++ b/core/modules/update/update.moduleundefined
@@ -900,11 +914,11 @@ function _update_manager_unique_identifier() {
- * @param $create
+ * @param bool $create
  *   (optional) Whether to attempt to create the directory if it does not
  *   already exist. Defaults to TRUE.
  *
- * @return
+ * @return string
  *   The full path to the temporary directory where update file archives should
  *   be extracted.

These type hint additions for _update_manager_extract_directory() are correct.

+++ b/core/modules/update/update.moduleundefined
@@ -922,11 +936,11 @@ function _update_manager_extract_directory($create = TRUE) {
- * @param $create
+ * @param bool $create
  *   (optional) Whether to attempt to create the directory if it does not
  *   already exist. Defaults to TRUE.
  *
- * @return
+ * @return string
  *   The full path to the temporary directory where update file archives should
  *   be cached.

Both of these type hint additions in _update_manager_cache_directory() are correct.

+++ b/core/modules/update/update.moduleundefined
@@ -973,7 +987,7 @@ function update_clear_update_disk_cache() {
  *
- * @param $path
+ * @param string $path
  *   A string containing a file path or (streamwrapper) URI.

This type hint in update_delete_file_if_stale() is correct.

There is quite a bit already in this patch @bleen18. How would you like to approach the additional items noted in #12? Would you like to add some more to this patch or just leave all of the rest until the follow up patch after this is committed?

I also would appreciate some guidance from @jhodgdon if she looks into this issue. Thanks in advance.