Make tracker module pass Coder Review

Here's a try

...and changing status.

Great job, @eiriksm! A few small things:

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

   This actually return an integer, not an object.

2. +++ b/core/modules/tracker/tracker.module
   @@ -319,19 +319,19 @@ function _tracker_calculate_changed($nid) {
      // The user only keeps his or her subscription if both of the following are true:
   -  //   (1) The node exists.
   -  //   (2) The user is either the node author or has commented on the node.
   +  // (1) The node exists.
   +  // (2) The user is either the node author or has commented on the node.
   

   This should be changed to follow our standard convention for formatting lists.

3. +++ b/core/modules/tracker/tracker.module
   @@ -364,7 +364,6 @@ function _tracker_remove($nid, $uid = NULL, $changed = NULL) {
          // If we're here, the item being removed is *possibly* the item that
          // established the node's changed timestamp.
   -
          // We just have to recalculate things from scratch.
   

   If we remove the line between these comments, they should just be treated as one. i.e. Remove the line break after the first one so it flows right into the second one: "...changed timestamp. We just...".

4. While we're at it, let's do the same for the comments right above it on lines 359-361.
5. There's a typo in the comment on line 370: "And then we push the out the...". Obviously, the first instance of the word "the" should be removed.

Thanks! A new patch is attached, and I think I got the list formatting this time (using drupal cs locally)

Perfect! Well done. I'll ask someone else to give it a look now so we can mark it RTBC.

Thanks @TravisCarden and @eiriksm; this looks great to me too.

+++ b/core/modules/tracker/tracker.moduleundefined
@@ -357,18 +357,17 @@ function _tracker_remove($nid, $uid = NULL, $changed = NULL) {
+      // And then we push out the new changed timestamp to our denormalized
       // tables.

One small suggestion while we are fixing this comment: Generally, core comments should follow fairly strict, prescriptive English grammar and writing standards. In this case, that means not starting the sentence with "And". :) (It's also better to avoid using pronouns like "you" and "we"; see the interface text guidelines.) I'd suggest:

// Then, push out the new, changed timestamp to the denormalized tables.

(This will also likely fit on one line.)

Edit: updated the suggested comment.

EDIT: Dont mind this. Uploading a new patch in 2mins.

New patch, in the git format-patch version, as first suggested by TravisCarden.

Alright, that looks good to me. Thanks @eiriksm!

Has someone verified that with these changes, the Tracker module actually passes Coder review? I've also asked on the meta issue for clarification of what "coder review" actually means, since the meta issue references quite a few different standards (coder, coder tough love, code sniffer, ...).

@TravisCarden confirmed to me on IRC that he ran drupalcs with the patch applied, which is stricter. At present there isn't actually a D8 version of coder even available, though someone is working on a sandbox. Maybe it would be good to confirm whether someone has tested with that sandbox as well?

CTL is not any part of any of the reviews. The summary in the other issue specifies drupalcs now, I believe.

That's right. I've clarified in the meta issue.

Just to clarify: I use drupalcs in my editor as well (as stated in #4), and this patch is based on the code passing drupalcs. Should probably have written that already at the first post to avoid confusion.

I added a comment to #1518116-21: [meta] Make Core pass Coder Review summarizing what I tried to suggest in IRC yesterday (it's possible I wasn't clear). Let's update the issue here with:

• Whether this directory either passes or fails on the Drupal 8 branch test code review tab. If it fails, also note whether the failures are itemized on that page.
• Whether the directory has been tested with Drupal Code Sniffer with the patch applied, whether it passes or fails, and what settings were used.
• Whether the directory has been tested with the D8 sandbox of Coder with the patch applied, whether it passes or fails, and what settings were used.

In each case note any failures that were not corrected, and why.

Setting back to NR until we can confirm these details. Thanks!

This passes coder with the exception of two things that are not accurate. (There is nothing to separate and no trailing space/punctuation)

Line 165: @see references should be separated by "," followed by a single space and with no trailing punctuation
 * @see tracker_menu()
Line 182: @see references should be separated by "," followed by a single space and with no trailing punctuation
 * @see tracker_menu()

Additionally line 168 in tracker.module exceeds 80 characters and should probably be changed to two lines. I'm not sure if this is detected by Drupalcs or not.

Thanks @NROTC_Webmaster. I agree that line 168 should be rewrapped or rewritten. It's the following inline comment:

  // This path is only allowed for authenticated users looking at their own content.

Regarding the false positives for the @see, can we see an issue is filed in coder for this already, file one if not, and then add it to the summary in #1361508: [META] Tracking issue for Coder Advisory Review test issues?

The issue has been filed and added to the tracker #1539698: False positive for @see

Sorry about the slow response, had a busy week.

I have now updated the patch with the suggestion in #17. To summarize this patch:

- The directory does not pass Drupal 8 branch test code review before this patch. But it is not itemized. That being said, it also says that the .install file has a critical error, and this patch does not touch tracker.install. Does anyone have a clue as to what that might be?
- The directory passes with the D8 port of coder (i did not get the @see remarks by the way)
- The directory passes Drupal Code sniffer (except the test), with one exception:

333 | WARNING | Line exceeds 80 characters; contains 83 characters

The line is the following:

// The user only keeps his or her subscription if both of the following are true:

IMO it is cleaner to keep that on one line, since the following lines are list items. Any thoughts on this?

I think saying both may be bit redundant instead it could just say
// The user only keeps his or her subscription if the following are true:

or, if you think it is worthwhile to say you could change it to
// Users only keep their subscriptions if both of the following are true:

Personally I think "both" should be there. But suggestion #2 is under 80 characters, so let's go with that.

New patch attached.

We should not ever go over 80 characters in a comment line, no matter what. The only exception is with content within a@code/@endcode code snippet where a line of sample code should not be wrapped according to our coding standards (because then we'd be providing a bad example).

Thanks!

The other exception is for @link/@endlink, which can exceed 80 characters (and if it is going to, put the @link ... @endlink on its own line).

Alright, I reviewed #21 and all those changes look good to me. It sounds like it passes both coder and drupalcs? Was this with the most sensitive settings?

One thought, not a blocker:

+++ b/core/modules/tracker/tracker.pages.incundefined
@@ -68,7 +68,9 @@ function tracker_page($account = NULL, $set_title = FALSE) {
-        'title' => array('data' => l($node->title, 'node/' . $node->nid) . ' ' . theme('mark', array('type' => node_mark($node->nid, $node->changed)))),
+        'title' => array(
+          'data' => l($node->title, 'node/' . $node->nid) . ' ' . theme('mark', array('type' => node_mark($node->nid, $node->changed))),

Maybe we could further clarify this by storing the result of the theme() call to a variable above this?

That sounds smart. That line was most definitely the one I was the least satisfied with, and it looks better now.

Code passes both drupalcs and coder now.

By the way, should we do cleanup on the test as well? Not many issues found:

FILE: ...ar/www/d8/core/modules/tracker/lib/Drupal/tracker/Tests/TrackerTest.php
--------------------------------------------------------------------------------
FOUND 6 ERROR(S) AND 1 WARNING(S) AFFECTING 7 LINE(S)
--------------------------------------------------------------------------------
  29 | ERROR   | Class property $other_user should use lowerCamel naming
     |         | without underscores
  31 | ERROR   | Missing function doc comment
  35 | WARNING | A comma should follow the last multiline array item. Found:
     |         | 'Tracker'
  39 | ERROR   | Missing function doc comment
  44 | ERROR   | If the line declaring an array spans longer than 80
     |         | characters, each element should be broken into its own line
 117 | ERROR   | If the line declaring an array spans longer than 80
     |         | characters, each element should be broken into its own line
 252 | ERROR   | If the line declaring an array spans longer than 80
     |         | characters, each element should be broken into its own line
--------------------------------------------------------------------------------

Yep, definitely should clean up the automated tests as well (although I think that first message there might be incorrect). Thanks @eiriksm!

Oh, something to be aware of: in #1512434: Make Aggregator module pass Coder Review we are discussing skipping the addition of datatypes in this sprint, since it is time-consuming to review those, and creating a separate sprint dedicated to that once we finish these.

I can see that. Here is a patch without the datatypes. This means it does not pass drupalcs any more, but still passes coder (on "minor" setting).

Two comments about the cleanup of the test:
- Did skip the first message.
- The two function doc issues are on the getInfo and setUp functions. While I could always write something here, this is something a little more universal. After looking at some other tests i can't seem to find that this is documented anywhere else either. Does that mean we should skip that one as well?

@eiriksm -- Aha, you are correct. The current standard is to omit docblocks for getInfo(), setUp(), and tearDown(). See #338403: Use {@inheritdoc} on all class methods (including tests) for the ongoing debate. ;)

i tested the patch in #28 but it is showing the following errors

~/www/drupal--8-sandbox/core/modules/tracker$ drupalcs tracker.module

FILE: /home/www/drupal--8-sandbox/core/modules/tracker/tracker.module
--------------------------------------------------------------------------------
FOUND 8 ERROR(S) AFFECTING 8 LINE(S)
--------------------------------------------------------------------------------
 262 | ERROR | Missing parameter type at position 1
 264 | ERROR | Missing parameter type at position 2
 266 | ERROR | Missing parameter type at position 3
 301 | ERROR | Missing parameter type at position 1
 304 | ERROR | Data type of return value is missing
 323 | ERROR | Missing parameter type at position 1
 325 | ERROR | Missing parameter type at position 2
 327 | ERROR | Missing parameter type at position 3
--------------------------------------------------------------------------------

Thanks for reviewing.

As I said, this patch was without parameter types (or, I used the word "datatypes") since cleaning up those was suggested to do in a different sprint. Therefore it is also as expected that you get these errors, and I also pointed this out in my comment that accompanied the patch, that is does not pass as it gives parameter type errors.

Setting back to needs review, if we all agree on separating data-type patches and other coding standard patches (like this one)?

Agreed, thanks @eiriksm and @dinathecool. So long as we document the failures are only @param/@return datatypes, that should be good. We are not including the datatypes in this sprint at the request of @jhodgdon, the core committer who will ultimately be reviewing these patches and committing them to core once they are RTBC.

I am sad to see that we are delaying the addition of @param/@return datatypes. Does it make sense to create a separate patch for those changes now too since so much of the work already was done?

RE #33 - sorry... you could make a patch but since it will probably be a while before it's committed, it probably won't apply by then.

Just as a note... The data types were also left out of the API Docs Cleanup initiative for the same reason. I really don't think they will *ever* be done in all of Core, because these coder and API docs initiatives were difficult enough (we've been doing the API cleanup since I think November), and adding data types will be a MUCH harder thing to accomplish (patch making and reviewing both), since (as I said above) it really means that at least 3 people (patch writer, reviewer, committer) need to read carefully through *every* function in every file of core in order to verify that the documented types for parameter and return value are complete and correct. And probably several times for each of the issues, as errors are found/fixed...

Thank you @jhodgdon. As you know, my key interest since last fall has been getting the type hinting in.

I agree with you that it will be overwhelming to review say, for instance, all the functions in includes files A-C in one go. As the committer who will be committing these @param/@return type hinting patches, perhaps you could give some guidance on the number of functions to target in such a patch? Should we target 5-10 functions in a patch or everything in one file (such as common.inc)?

Re: #35, this is all offtopic for this issue. Let's wait until we have more of the other patches in and then discuss it in its own issue.

Having hundreds more patches is even worse. I don't know what the solution is. Really I think the problem is rather intractable to fix all the functions in all of core, because it's not something we can do with a script or a casual glance.

Postponing till feature freeze. If you want to help in the meantime, please work on the blockers on the meta issue. Thanks!

I have created a patch , Please review

Patch #40 needs reroll.

Working on this at DrupalCon Amsterdam.

rerolled

Thanks for all the work here so far. See #1518116-86: [meta] Make Core pass Coder Review. This issue is postponed until the meta issue is either closed or reopened.

Closing in favor of #2571965: [meta] Fix PHP coding standards in core, stage 1. In this issue the coding standards will be fixed on a sniff-per-sniff basis rather than a module-per-module basis.