Files: 
CommentFileSizeAuthor
#19 drupal.make-path-pass-coder-review.1533266-19.patch9.97 KBamitgoyal
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,852 pass(es), 7 fail(s), and 0 exception(s).
[ View ]

Comments

krishworks’s picture

code review status of path module as of now.

FILE: /Users/neokrish/d8/drupal/core/modules/path/path.admin.inc
--------------------------------------------------------------------------------
FOUND 7 ERROR(S) AND 1 WARNING(S) AFFECTING 8 LINE(S)
--------------------------------------------------------------------------------
  93 | ERROR   | Missing parameter type at position 1
  96 | ERROR   | Data type of return value is missing
116 | ERROR   | Missing parameter type at position 1
124 | ERROR   | If the line declaring an array spans longer than 80
     |         | characters, each element should be broken into its own line
167 | WARNING | A comma should follow the last multiline array item. Found: ]
255 | ERROR   | Missing parameter type at position 1
292 | ERROR   | The first index in a multi-value array must be on a new line
308 | ERROR   | Array closing indentation error, expected 2 spaces but found 4
--------------------------------------------------------------------------------

FILE: /Users/neokrish/d8/drupal/core/modules/path/path.api.php
--------------------------------------------------------------------------------
FOUND 3 ERROR(S) AFFECTING 3 LINE(S)
--------------------------------------------------------------------------------
16 | ERROR | Missing parameter type at position 1
37 | ERROR | Missing parameter type at position 1
56 | ERROR | Missing parameter type at position 1
--------------------------------------------------------------------------------

FILE: /Users/neokrish/d8/drupal/core/modules/path/path.info
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
1 | WARNING | No PHP code was found in this file and short open tags are not
   |         | allowed by this install of PHP. This file may be using short
   |         | open tags but PHP does not allow them.
--------------------------------------------------------------------------------

FILE: /Users/neokrish/d8/drupal/core/modules/path/path.test
--------------------------------------------------------------------------------
FOUND 24 ERROR(S) AND 4 WARNING(S) AFFECTING 23 LINE(S)
--------------------------------------------------------------------------------
  12 | ERROR   | Missing function doc comment
  33 | ERROR   | Missing function doc comment
  41 | ERROR   | Missing function doc comment
  45 | ERROR   | If the line declaring an array spans longer than 80
     |         | characters, each element should be broken into its own line
  96 | WARNING | Line exceeds 80 characters; contains 81 characters
  96 | ERROR   | Comments may not appear after statements.
  97 | WARNING | Line exceeds 80 characters; contains 83 characters
  97 | ERROR   | Comments may not appear after statements.
  98 | ERROR   | Comments may not appear after statements.
117 | ERROR   | Inline comments must start with a capital letter
117 | ERROR   | Inline comments must end in full-stops, exclamation marks, or
     |         | question marks
152 | WARNING | Line exceeds 80 characters; contains 87 characters
152 | ERROR   | Comments may not appear after statements.
153 | WARNING | Line exceeds 80 characters; contains 83 characters
153 | ERROR   | Comments may not appear after statements.
154 | ERROR   | Comments may not appear after statements.
189 | ERROR   | Missing parameter type at position 1
221 | ERROR   | Missing function doc comment
229 | ERROR   | Missing function doc comment
240 | ERROR   | If the line declaring an array spans longer than 80
     |         | characters, each element should be broken into its own line
292 | ERROR   | Missing function doc comment
300 | ERROR   | Missing function doc comment
304 | ERROR   | If the line declaring an array spans longer than 80
     |         | characters, each element should be broken into its own line
435 | ERROR   | Missing function doc comment
443 | ERROR   | Missing function doc comment
447 | ERROR   | If the line declaring an array spans longer than 80
     |         | characters, each element should be broken into its own line
511 | ERROR   | Missing function doc comment
519 | ERROR   | Missing function doc comment
--------------------------------------------------------------------------------

krishworks’s picture

Assigned:Unassigned» krishworks
kim.pepper’s picture

Assigned:krishworks» kim.pepper
Status:Active» Needs review
StatusFileSize
new3.51 KB
PASSED: [[SimpleTest]]: [MySQL] 40,053 pass(es).
[ View ]

Here is a first run at getting coder review passing.

Kim

TravisCarden’s picture

Status:Needs review» Postponed

Thanks for the patch, @kim.pepper. Adding @param and @return types has been excluded from this effort, to be handled in a separate issue. See "Proposed resolution" #5 in the meta issue.

Let's postpone until the meta issue is un-stalled (see #1518116-35: [meta] Make Core pass Coder Review)—unless you can confirm that this issue is unaffected by the blocker.

jhodgdon’s picture

Also, changes like this are NOT part of the Drupal coding standards:

-function path_admin_form($form, &$form_state, $path = array('source' => '', 'alias' => '', 'langcode' => LANGUAGE_NOT_SPECIFIED, 'pid' => NULL)) {
+function path_admin_form($form, &$form_state, $path = array(
+    'source' => '',
+    'alias' => '',
+    'langcode' => LANGUAGE_NOT_SPECIFIED,
+    'pid' => NULL)) {

See
#1539712: [policy, no patch] Coding standards for breaking function calls, function declarations, and language constructs across lines

kim.pepper’s picture

Thanks for the feedback Jennifer. I was a bit confused about where to break the lines in functions with default args like this. I will hold of making any changes until this is issue re-activated.

Kim

Lars Toomre’s picture

Status:Postponed» Active

When this patch next gets re-rolled, the array in setUp() of PathLanguageTest.php really could use breaking up into several lines to make it more readable.

Lars Toomre’s picture

Issue tags:-Novice

I just rolled a further docs clean up patch in #1811948: Further clean up of API docs for Path module. As a whole, the Path module looks pretty good from a docs perspective. Let's try to move this issue forward now to make Path module completely conform with Coder review.

Is the code review summary from April 2012 still accurate? Perhaps someone can help here with an update?

I am unable to run Coder at the moment on this module. It would be great if someone could post a current code sniff. Thanks in advance!!

Given the current state of the Coder sniffs, I am not sure if this is a novice issue since one has to interpret some false positive sniffs that are not part of Drupal standards. Hence, I am removing the Novice tag.

TravisCarden’s picture

Status:Active» 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:kim.pepper» visabhishek
Issue summary:View changes
visabhishek’s picture

Status:Active» Needs review
StatusFileSize
new14.08 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,574 pass(es).
[ View ]

I have created a patch, Please review..

visabhishek’s picture

StatusFileSize
new14.04 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal.make-path-pass-coder-review.1533266-14.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, 14: drupal.make-path-pass-coder-review.1533266-14.patch, failed testing.

kim.pepper’s picture

Issue tags:+needs re-roll
tim.plunkett’s picture

Issue tags:-needs re-roll+Needs reroll
amitgoyal’s picture

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new9.97 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,852 pass(es), 7 fail(s), and 0 exception(s).
[ View ]

Reroll of #14.

Status:Needs review» Needs work

The last submitted patch, 19: drupal.make-path-pass-coder-review.1533266-19.patch, failed testing.

kim.pepper’s picture

+++ b/core/modules/path/src/Tests/PathAliasTest.php
@@ -83,9 +91,10 @@ function testAdminAlias() {
-    $edit['alias'] = "- ._~!$'"()*@[]?&+%#,;=:" . // "Special" ASCII characters.
-      "%23%25%26%2B%2F%3F" . // Characters that look like a percent-escaped string.
-      "éøïвβ中國書۞"; // Characters from various non-ASCII alphabets.
+    // "Special" ASCII characters.
+    // Characters that look like a percent-escaped string.
+    // Characters from various non-ASCII alphabets.
+    $edit['alias'] = "- ._~!$'"()*@[]?&+%#,;=:%23%25%26%2B%2F%3Féøïвβ中國書۞";

I think this change is breaking the tests.

xjm’s picture

Version:8.0.x-dev» 8.1.x-dev
Assigned:visabhishek» Unassigned
Priority:Normal» Minor
Status:Needs work» 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.