Files: 
CommentFileSizeAuthor
#7 drupal.make-statistics-module-pass-coder-review.1533390-07.patch19.62 KBvisabhishek
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,588 pass(es).
[ View ]
#6 drupal-make_statistics_module_pass_coder_review-1533390-06.patch19.75 KBvisabhishek
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,426 pass(es).
[ View ]
#2 drupal-statistics-coding-standards-1533390-2.patch36.63 KBtheduke
PASSED: [[SimpleTest]]: [MySQL] 41,238 pass(es).
[ View ]

Comments

NROTC_Webmaster’s picture

Status:Active» Postponed
theduke’s picture

Assigned:Unassigned» theduke
StatusFileSize
new36.63 KB
PASSED: [[SimpleTest]]: [MySQL] 41,238 pass(es).
[ View ]

I'm taking this on.

Patch attached.

Coder reports no problems.

Code Sniffer reports several errors, but I consider all of those to be false positives/invalid:

FILE: .../modules/statistics/lib/Drupal/statistics/Tests/StatisticsAdminTest.php
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
185 | ERROR | Using the e flag in preg_match is a possible security risk. For
     |       | details see http://drupal.org/node/750148
--------------------------------------------------------------------------------

This seems to be an error in Drupal Code Sniffer.
See http://drupal.org/node/1779020

FILE: .../drupal8.local/public_html/core/modules/statistics/statistics.admin.inc
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
318 | ERROR | If the line declaring an array spans longer than 80 characters,
     |       | each element should be broken into its own line
--------------------------------------------------------------------------------

In my opinion, changing this would not do anything for readability. It would actually harm it.

FILE: ...dev/drupal8.local/public_html/core/modules/statistics/statistics.module
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
  32 | ERROR | If the line declaring an array spans longer than 80 characters,
     |       | each element should be broken into its own line
327 | ERROR | If the line declaring an array spans longer than 80 characters,
     |       | each element should be broken into its own line
--------------------------------------------------------------------------------

Line 32: All the output in the top is difficult to make readable, but I think it's alright the way it is.
Line 327: Same as previous file.

theduke’s picture

Status:Postponed» Needs review
TravisCarden’s picture

Status:Needs review» Postponed

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

sphism’s picture

Status:Postponed» Active

We have the go ahead with all these issues again, see #1518116: [meta] Make Core pass Coder Review for more details

visabhishek’s picture

Assigned:theduke» visabhishek
Issue summary:View changes
Status:Active» Needs review
StatusFileSize
new19.75 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,426 pass(es).
[ View ]

I have created a patch. Please review.

visabhishek’s picture

StatusFileSize
new19.62 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,588 pass(es).
[ View ]

I have updated this patch as per https://drupal.org/comment/8601579#comment-8601579.

jepSter’s picture

I'm reviewing the latest state of the core according to this issue now.

jepSter’s picture

*deleted because duplication*

jepSter’s picture

Issue tags:+Amsterdam2014

There isn't any state of the coder module right now, which makes it possible to review the code with coder. See https://www.drupal.org/node/2182043#comment-9208083. Have you reviewed the statistics module anyhow by command line?

xjm’s picture

Version:8.0.x-dev» 8.1.x-dev
Assigned:visabhishek» Unassigned
Priority:Normal» Minor
Status:Needs review» Postponed

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.