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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anonymous’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
FileSize
2.49 KB
1.21 KB

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
FileSize
1.21 KB

@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
FileSize
1.21 KB

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
tatarbj’s picture

Assigned: Unassigned » tatarbj
Issue summary: View changes
Status: Closed (fixed) » Needs review
FileSize
4.47 KB

I'm sorry but need to reopen this ticket because ban module has unsolved coding standards errors. I'm attaching patch which solve those.
The discussion about it is followable here: https://www.drupal.org/node/1518116

jaxxed’s picture

Assigned: tatarbj » Unassigned
Status: Needs review » Reviewed & tested by the community
FileSize
3.91 KB

Coder/phpcs now reports no issues after applying #20 patch (see attached issues list coder_issues_ban_2015_09-07.txt for issues without patch)

pfrenssen’s picture

I reviewed the patch and it is looking great! I found a spelling mistake, but this is not 100% in scope of this issue, so leaving status to RTBC.

  1. +++ b/core/modules/ban/src/Plugin/migrate/destination/BlockedIp.php
    @@ -34,15 +34,15 @@ class BlockedIP extends DestinationBase implements ContainerFactoryPluginInterfa
    -   *  The plugin definiiton.
    +   *   The plugin definiiton.
    

    I would take the opportunity to fix this spellign error if we are touching this line.

  2. +++ b/core/modules/ban/src/Tests/Migrate/d7/MigrateBlockedIPsTest.php
    @@ -38,7 +38,7 @@ protected function setUp() {
    -  public function testBlockedIPs() {
    +  public function testBlockedIps() {
    

    The codesniffer doesn't like this?

tatarbj’s picture

Hi @pfrenssen,
the first spelling mistake is fixed in my new patch.
The second one what you mention, if it's not fixed the codesniffer throws this error:

FILE: .../core/modules/ban/src/Tests/Migrate/d7/MigrateBlockedIPsTest.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 41 | ERROR | Public method name
    |       | "MigrateBlockedIPsTest::testBlockedIPs" is not in
    |       | lowerCamel format
----------------------------------------------------------------------

So in this case it thinks it's not correct because there are two capital letters which brakes this rule, i think we should accept it. Btw it's the same with this change:

-  function testIPAddressValidation() {
+  public function testIpAddressValidation() {

where the change is not just the visibility but the IP goes to Ip as well.

tatarbj’s picture

Status: Reviewed & tested by the community » Needs review
tatarbj’s picture

Status: Needs review » Reviewed & tested by the community

Because the test was ok, i'm taking the issue back to rtbc based on others' check.

jaxxed’s picture

FileSize
714 bytes

23-20 interdiff added

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

@tatarbj opening a closed issue after 3 years messes up issue statistics and how we assign credit for issues - can you please move your patch to a new issue.

tatarbj’s picture

Hi people, here is the new issue, please follow this one: #2566313: Make Ban module pass Coder Review

Status: Fixed » Closed (fixed)

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