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
FileSize
3.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
FileSize
14.08 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,574 pass(es). View

I have created a patch, Please review..

visabhishek’s picture

FileSize
14.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
FileSize
9.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.

tatarbj’s picture

Status: Postponed » Closed (duplicate)

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