This is part of meta-issue #1518116: [meta] Make Core pass Coder Review focused on the new D8 Ban module.

  • 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.
  • >> There are 0 minor / 0 critical / 0 normal issues.

  • Whether the directory has been tested with Drupal Code Sniffer (patched per #36) with the patch applied, whether it passes or fails, and what settings were used.
  • >> It has been tested using 7.x-2.x branch of Coder module as Drupal Code Sniffer module is part of Coder module now. I tried applying this patch #36 on Code Sniffer module but nothing happens. No errors and no changes. Even if I make those changes manually, it doesn't ignore excluded sniffs.

  • 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.
  • >> #1536122: Drupal 8 port no longer works with 7.x-2.x branch of Coder module. So I have tested it using #1816302: Code Review 7.x-1.2 to 8.x-1.x Patch patch. This patch also takes care of {system} table issue.

Files: 
CommentFileSizeAuthor
#9 coder_issues-ban-1816690-d8-2012_10_23.patch1.21 KBamitgoyal
PASSED: [[SimpleTest]]: [MySQL] 46,475 pass(es).
[ View ]
#6 coder_issues-ban-1816690-d8-2012_10_22.patch1.21 KBamitgoyal
PASSED: [[SimpleTest]]: [MySQL] 42,803 pass(es).
[ View ]
#3 coder_issues-ban-1816690-d8-2012_10_22.patch1.21 KBamitgoyal
PASSED: [[SimpleTest]]: [MySQL] 42,802 pass(es).
[ View ]
#3 coder_issues_ban_2012_10_22.txt2.49 KBamitgoyal

Comments

earnie’s picture

Lars Toomre’s picture

#1816732: Add missing type hinting to Ban module docblocks is open to deal separately with any missing type hinting from @param and/or @return directives.

amitgoyal’s picture

Assigned:Unassigned» amitgoyal
Status:Active» Needs review
StatusFileSize
new2.49 KB
new1.21 KB
PASSED: [[SimpleTest]]: [MySQL] 42,802 pass(es).
[ View ]

Based on the learnings from #1533244: Make help module pass Coder Review issue, I have made fixes, related to coding standards, in Ban module. As per learning there, I have ignored following issues,

- Standard simpletest functions getInfo(), setUp(), and tearDown() don't get doxygen comments
- No scope modifier specified for function getInfo() in class files

Attaching a Patch here which will fix the remaining issues. I am also attaching Coder output coder_issues_ban_2012_10_22.txt so that we can see the list of all issues.

Kars-T’s picture

Status:Needs review» Reviewed & tested by the community

Two comments are now below 80 chars and the array has an "," at its end. Seems okay to me.

TravisCarden’s picture

Status:Reviewed & tested by the community» Needs work

A couple of tiny things, actually:

+++ b/core/modules/ban/lib/Drupal/ban/Tests/IpAddressBlockingTest.php
@@ -22,12 +22,12 @@ public static function getInfo() {
-   * Test a variety of user input to confirm correct validation and saving of data.
+   * Tests variety of user input to confirm correct validation & saving of data.

It's not proper to use an ampersand in place of the word "and" in full English prose. Let's just use the original comment as-is and wrap it.

+++ b/core/modules/ban/lib/Drupal/ban/Tests/IpAddressBlockingTest.php
@@ -76,7 +76,7 @@ function testIPAddressValidation() {
-    // TODO: on some systems this test fails due to a bug or inconsistency in cURL.
+    // TODO: on some systems this test fails due to a bug/inconsistency in cURL.

As long as we're here, why don't we capitalize this sentence? "on" -> "On"

Thanks!

amitgoyal’s picture

Status:Needs work» Needs review
StatusFileSize
new1.21 KB
PASSED: [[SimpleTest]]: [MySQL] 42,803 pass(es).
[ View ]

@TravisCarden - I have changed "on" -> "On" and attached the updated patch.

Regarding first point "It's not proper to use an ampersand in place of the word "and" in full English prose. Let's just use the original comment as-is and wrap it.", if we wrap it to next line then there would be new issue,

Function comment short description must be on a single line

So I haven't changed it. Please let me know if you have some other solution.

Lars Toomre’s picture

@amitgoyal Thanks for the patch in #7. I hope to get to reviewing this later today (once it goes green).

Perhaps you could do all of us a favor and post some simple instructions about how you generated the txt file attached in #3? To my knowledge, I am unable to do so.

The Ban module is a fairly small module that was spun out from an include file in the system module. Action is a similarly new D8 module that lived in the System module in D7.

Not that I expect anyone to post a patch for those modules, but for my own edification, I would love to get a sense of what lays ahead of us for some of the older and bigger modules. Is it possible to generate a report similar to what is in #3 for some of the modules with lots of legacy code? Such modules include Comments, Node, Poll, System, Taxonomy, and/or User.

If you can easily do so, could you post any such reports in the appropriate sub-issue(s) of #1518116: [meta] Make Core pass Coder Review? Thanks in advance for your help @amitgoyal. I am so glad to see us inching forward on this initiative and congrats for getting the patch for the Helper module committed last week!!

TravisCarden’s picture

Status:Needs review» Needs work
+++ b/core/modules/ban/lib/Drupal/ban/Tests/IpAddressBlockingTest.php
@@ -22,12 +22,12 @@ public static function getInfo() {
-   * Test a variety of user input to confirm correct validation and saving of data.
+   * Tests variety of user input to confirm correct validation & saving of data.

How about "Tests various user input to confirm correct validation and saving of data."? That preserves the English and satisfies the sniff.

amitgoyal’s picture

Status:Needs work» Needs review
StatusFileSize
new1.21 KB
PASSED: [[SimpleTest]]: [MySQL] 46,475 pass(es).
[ View ]

Sure @TravisCarden. Here is the updated patch.

TravisCarden’s picture

Thanks, @amitgoyal. That looks better! If you can update the issue summary per the meta issue's Proposed Resolution #7 we can RTBC this.

amitgoyal’s picture

@Lars - Here are the steps to check the status of drupal 8 modules using coder or generate the txt file attached in #3,

1) Clone (git clone --recursive --branch 8.x http://git.drupal.org/project/drupal.git) the latest D8 branch on your system or update your existing D8 branch (git pull).
2) Clone (git clone --recursive --branch 7.x-2.x http://git.drupal.org/project/coder.git) the 7.x-2.x branch of Coder module.
3) Download and apply this patch #1816302: Code Review 7.x-1.2 to 8.x-1.x Patch to 7.x-2.x branch of Coder module.
4) Enable the Code Review module and review any core / contributed module from Configuration->Coder (admin/config/development/coder). Make sure to check "Drupal CodeSniffer" and "minor (most)" in the Reviews section. The output by Code Review module can be copied into text file.

Hope this will help you and others if they want to review modules for d8.

@TravisCarden - I am not very sure what do you want me to do. As per my understanding and points mentioned in #7, I have added instructions here to code review any d8 module.

TravisCarden’s picture

@amitgoyal: Sorry, I had a bad hyperlink in my comment. Here's proposed resolution #7 from the meta issue:

Note in your issue summary:

  • 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 (patched per #36) 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.

That's what I'm asking for. :) Does that make sense?

amitgoyal’s picture

@TravisCarden - I have updated the issue summary. Please let me know if this is what you are asking for. If not then please help me in doing so.

TravisCarden’s picture

Status:Needs review» Reviewed & tested by the community

That looks great, @amitgoyal. Thanks!

webchick’s picture

Assigned:amitgoyal» jhodgdon

Passing the buck. :)

jhodgdon’s picture

I will get this committed soon -- but there's a big Views sprint going on and commits are delaying their patch reviews, so postponing a few days. Sorry!

catch’s picture

jhodgdon’s picture

Assigned:jhodgdon» Unassigned
Status:Reviewed & tested by the community» Fixed

Thanks! This one is committed to 8.x.

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

Anonymous’s picture

Issue summary:View changes