Add missing type hinting to Tracker module docblocks

Setting the status so we can see what the bot thinks...

@bleen18 Here is a detailed review of the patch in #1.

I could easily make the needed changes myself, but if I update the patch, I will not be able to RTBC it for @jhodgdon. Perhaps you can incorporate the needed changes from below? Thanks in advance.

After applying this patch locally, I grepped through all of the Tracker module and confirm that with these type hinting additions, this module would have complete type hinting coverage.

+++ b/core/modules/tracker/tracker.moduleundefined
@@ -160,9 +160,9 @@ function tracker_cron() {
- *   has permission to access the content.
+ * @return bool
+ *   TRUE if a user is accessing tracking info for their own account and has

This change to _tracker_myrecent_access() docblock is correct.

+++ b/core/modules/tracker/tracker.moduleundefined
@@ -177,9 +177,9 @@ function _tracker_myrecent_access($account) {
- *   has permission to access the content.
+ * @return bool
+ *   TRUE if a user has permission to access the account for $account and has

This change to _tracker_user_access() docblock is also correct.

+++ b/core/modules/tracker/tracker.moduleundefined
@@ -260,11 +260,11 @@ function tracker_comment_delete($comment) {
  *
- * @param $nid
+ * @param int $nid
  *   A node ID.
- * @param $uid
+ * @param int $uid
  *   The node or comment author.
- * @param $changed
+ * @param int $changed
  *   The node updated timestamp or comment timestamp.

Each of these three changes to the docblock for _tracker_add() are correct.

+++ b/core/modules/tracker/tracker.moduleundefined
@@ -299,12 +299,12 @@ function _tracker_add($nid, $uid, $changed) {
- * @param $nid
+ * @param int $nid
  *   A node ID.

This change in docblock for _tracker_calculate_changed() is correct.

+++ b/core/modules/tracker/tracker.moduleundefined
@@ -299,12 +299,12 @@ function _tracker_add($nid, $uid, $changed) {
- *  is the greatest.
+ * @return int
+ *  The $node->changed timestamp, or most recent comment timestamp, whichever is
+ *  the greatest.

The indentation for this portion of the docblock is incorrect. (Needs to be in one more space.)

However, the added type hint of int is correct.

+++ b/core/modules/tracker/tracker.moduleundefined
@@ -321,11 +321,11 @@ function _tracker_calculate_changed($nid) {
  *
- * @param $nid
+ * @param int $nid
  *  The node ID.
- * @param $uid
+ * @param int $uid
  *   The author of the node or comment.
- * @param $changed
+ * @param int $changed
  *   The last changed timestamp of the node.
  */

The type hints added for _tracker_remove() are all correct. I also confirmed that there is no @return directive required.

One thing that does need to be done for this docblock is to add '(optional)' at the start of the optonal variables and to explicitly state what their default values are with 'Defaults to NULL.'

+++ b/core/modules/tracker/tracker.pages.incundefined
@@ -12,6 +12,12 @@
  *
+ * @param obj $account
+ *   (optional) The user object whos tracking data will be displayed.
+ * @param bool $set_title
+ *   If an $account is supplied and this is TRUE then the page title will be set
+ *   to the username in question.
+ *

The docblock for tracker_page() needs to be changed.

The first @param directive should be 'obj|null' and the phrase 'Defaults to NULL.' needs to be added to the end of its explanation.

The type hint for the second @param is correct. However, the explanation needs to start with '(optional)' and it also needs to state what the default value is.

Thanks for the patch @bleen18! This time I figured out from looking at the Test results what the exact patch file name was. Previously, I did not know to look there.

This is another nit pickity review of the patch so that we arrive at something that is pristine when it gets to RTBC stage:

+++ b/core/modules/tracker/tracker.moduleundefined
@@ -160,9 +160,9 @@ function tracker_cron() {
  *
- * @return boolean
- *   TRUE if a user is accessing tracking info for their own account and
- *   has permission to access the content.
+ * @return bool
+ *   TRUE if a user is accessing tracking info for their own account and has
+ *   permission to access the content.

Thanks for the change from 'boolean' to 'bool'. With patch applied, I can confirm that all of these type of changes are correct and that there are no more 'boolean' type hints in this patch.

+++ b/core/modules/tracker/tracker.moduleundefined
@@ -260,11 +260,11 @@ function tracker_comment_delete($comment) {
  *
- * @param $nid
+ * @param int $nid
  *   A node ID.
- * @param $uid
+ * @param int $uid
  *   The node or comment author.
- * @param $changed
+ * @param int $changed
  *   The node updated timestamp or comment timestamp.

These added type hints are all correct.

Reading the description for $changed, I think that could be clarified as it confused me at first. Perhaps something like 'A timestamp value, from when either a comment was added, or the last time the node was updated.'? What do you think?

+++ b/core/modules/tracker/tracker.moduleundefined
@@ -299,12 +299,12 @@ function _tracker_add($nid, $uid, $changed) {
  *
- * @param $nid
+ * @param int $nid
  *   A node ID.
  *
- * @return
- *  The $node->changed timestamp, or most recent comment timestamp, whichever
- *  is the greatest.
+ * @return int
+ *   The $node->changed timestamp, or most recent comment timestamp, whichever
+ *   is the greatest.
  */

These type hints for _tracker_calculate_changed() are correct.

+++ b/core/modules/tracker/tracker.moduleundefined
@@ -321,12 +321,12 @@ function _tracker_calculate_changed($nid) {
+ * @param int $nid
+ *   The node ID.
+ * @param int $uid
+ *   (optional) The author of the node or comment. Defaults to NULL.
+ * @param int $changed
+ *   (optional) The last changed timestamp of the node. Defaults to NULL.

My understanding in _tracker_remove() is that the second two of the added type hints here both need to be 'int|null'. Is that what was being referred to in #4? I note that my review from #3 was incorrect for this docblock.

+++ b/core/modules/tracker/tracker.pages.incundefined
@@ -12,6 +12,13 @@
+ * @param obj $account
+ *   (optional) The user object whos tracking data will be displayed. Defaults
+ *   to NULL.
+ * @param bool $set_title

My understanding is that this sort of type hint should be spelled out as 'object'. In addition, for this docblock, the correct type hint should properly be 'object|null'.

The second added type hint here is correct.

====== Other observations from reading through the Tracker files with this patch applied:

1) I wonder whether we should file a new issue to add type hinting in the function declaration in functions like tracker_comment_update($comment). I would think that at minimum it should be 'object $comment' if not 'Comment $comment'. What do you think?

2) We did not add the type hint for $account in this patch for either _tracker_myrecent_access() or _tracker_user_access(). However, reading both functions I believe that both type hints are incorrect.

The first function is clearly referencing $account as an object. Hence, I believe that $account type hint should be 'object'.

The second $account variable is used in the call to user_view_access() where it can be either an integer or an object. Hence, the second type hint should properly be 'int|object'.

3) Finally, with this patch applied locally, I can confirm that we have caught all of the type hinting for @param and @return directives in the Tracker module.

Thanks again for this patch @bleen18. As I stated in other sub-issue for this initiative, I could roll a patch that incorporates these review issues. However, if I do so, my understanding is that neither you nor I could then elevate the result to RTBC. Hence, I would like to keep myself at the review status for this issue so that we can generate a pristine patch that is ready for final review and commit. Again @bleen18, thanks for your help in getting us there!