Part of meta-issue #1518116: [meta] Make Core pass Coder Review

I've separated this out from #1533096: Make includes directory, files starting with D-G pass Coder Review because there seem to be lots of fiddly issues, whereas all the other files are pretty tidy

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards Instructions
Files: 
CommentFileSizeAuthor
#13 2054345-13.patch4.25 KBsergeypavlenko
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2054345-13.patch. Unable to apply patch. See the log in the details link for more information. View
#7 2054345-7.patch7.08 KBsergeypavlenko
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,655 pass(es). View
#4 form.inc-coder-2054345-4.patch20.65 KBsphism
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch form.inc-coder-2054345-4.patch. Unable to apply patch. See the log in the details link for more information. View

Comments

sphism’s picture

FILE: /www/drupal8/drupal/core/includes/form.inc
--------------------------------------------------------------------------------
FOUND 53 ERROR(S) AND 15 WARNING(S) AFFECTING 67 LINE(S)
--------------------------------------------------------------------------------
  455 | ERROR   | Expected 1 space before "=>"; 0 found
  696 | ERROR   | Last parameter comment requires a blank newline after it
  704 | ERROR   | Expected 1 space between asterisk and tag; 3 found
  707 | ERROR   | Expected 1 space between asterisk and tag; 3 found
  709 | ERROR   | Expected 1 space between asterisk and tag; 3 found
  713 | ERROR   | Expected 1 space between asterisk and tag; 3 found
  810 | WARNING | Line exceeds 80 characters; contains 81 characters
  952 | ERROR   | If the line declaring an array spans longer than 80
      |         | characters, each element should be broken into its own line
 1332 | ERROR   | Expected 1 space after "="; 2 found
 1392 | ERROR   | If the line declaring an array spans longer than 80
      |         | characters, each element should be broken into its own line
 1491 | ERROR   | If the line declaring an array spans longer than 80
      |         | characters, each element should be broken into its own line
 1771 | WARNING | Line exceeds 80 characters; contains 84 characters
 1875 | WARNING | Line exceeds 80 characters; contains 82 characters
 1905 | ERROR   | If the line declaring an array spans longer than 80
      |         | characters, each element should be broken into its own line
 1954 | ERROR   | Expected 1 space before "/"; 0 found
 1954 | ERROR   | Expected 1 space after "/"; 0 found
 2089 | ERROR   | Inline comments must end in full-stops, exclamation marks, or
      |         | question marks
 2124 | WARNING | Line exceeds 80 characters; contains 86 characters
 2128 | WARNING | Line exceeds 80 characters; contains 84 characters
 2129 | WARNING | Line exceeds 80 characters; contains 82 characters
 2258 | ERROR   | Comment indentation error, expected only 1 spaces
 2261 | ERROR   | Comment indentation error, expected only 1 spaces
 2262 | ERROR   | Comment indentation error, expected only 3 spaces
 2263 | ERROR   | Comment indentation error, expected only 5 spaces
 2313 | ERROR   | Inline comments must start with a capital letter
 2334 | ERROR   | Doc comment for var $form does not match actual variable name
      |         | $element at position 1
 2699 | ERROR   | Last parameter comment requires a blank newline after it
 2721 | ERROR   | Expected 1 space between asterisk and tag; 7 found
 2803 | WARNING | Line exceeds 80 characters; contains 85 characters
 2985 | ERROR   | If the line declaring an array spans longer than 80
      |         | characters, each element should be broken into its own line
 3026 | ERROR   | Expected 1 space after "="; 2 found
 3033 | ERROR   | Expected 1 space after "="; 2 found
 3095 | ERROR   | If the line declaring an array spans longer than 80
      |         | characters, each element should be broken into its own line
 3162 | ERROR   | If the line declaring an array spans longer than 80
      |         | characters, each element should be broken into its own line
 3317 | ERROR   | Function comment short description must start with a capital
      |         | letter
 3363 | ERROR   | Function comment short description must start with a capital
      |         | letter
 3386 | ERROR   | Function comment short description must start with a capital
      |         | letter
 3435 | ERROR   | Last parameter comment requires a blank newline after it
 3444 | ERROR   | Expected 1 space between asterisk and tag; 5 found
 3470 | ERROR   | Expected 1 space between asterisk and tag; 5 found
 3524 | ERROR   | If the line declaring an array spans longer than 80
      |         | characters, each element should be broken into its own line
 3604 | ERROR   | Function comment short description must start with a capital
      |         | letter
 3679 | WARNING | Line exceeds 80 characters; contains 82 characters
 3701 | ERROR   | Function comment short description must start with a capital
      |         | letter
 4077 | WARNING | Line exceeds 80 characters; contains 81 characters
 4237 | ERROR   | If the line declaring an array spans longer than 80
      |         | characters, each element should be broken into its own line
 4256 | ERROR   | If the line declaring an array spans longer than 80
      |         | characters, each element should be broken into its own line
 4289 | ERROR   | If the line declaring an array spans longer than 80
      |         | characters, each element should be broken into its own line
 4308 | ERROR   | If the line declaring an array spans longer than 80
      |         | characters, each element should be broken into its own line
 4327 | ERROR   | If the line declaring an array spans longer than 80
      |         | characters, each element should be broken into its own line
 4416 | ERROR   | If the line declaring an array spans longer than 80
      |         | characters, each element should be broken into its own line
 4435 | ERROR   | If the line declaring an array spans longer than 80
      |         | characters, each element should be broken into its own line
 4531 | ERROR   | If the line declaring an array spans longer than 80
      |         | characters, each element should be broken into its own line
 4562 | ERROR   | If the line declaring an array spans longer than 80
      |         | characters, each element should be broken into its own line
 4699 | ERROR   | If the line declaring an array spans longer than 80
      |         | characters, each element should be broken into its own line
 4823 | ERROR   | Doc comment for var $name does not match actual variable name
      |         | $class at position 2
 4914 | WARNING | Line exceeds 80 characters; contains 88 characters
 4930 | WARNING | Line exceeds 80 characters; contains 96 characters
 4942 | WARNING | Line exceeds 80 characters; contains 103 characters
 4958 | WARNING | Line exceeds 80 characters; contains 91 characters
 4969 | WARNING | Line exceeds 80 characters; contains 99 characters
 4999 | ERROR   | Last parameter comment requires a blank newline after it
 5004 | ERROR   | Expected 1 space between asterisk and tag; 5 found
 5009 | ERROR   | Expected 1 space between asterisk and tag; 5 found
 5016 | ERROR   | Expected 1 space between asterisk and tag; 5 found
 5111 | ERROR   | Inline comments must end in full-stops, exclamation marks, or
      |         | question marks
 5143 | ERROR   | If the line declaring an array spans longer than 80
      |         | characters, each element should be broken into its own line
 5156 | WARNING | Line exceeds 80 characters; contains 82 characters
--------------------------------------------------------------------------------
sphism’s picture

Assigned: Unassigned » sphism
sphism’s picture

I'm going to work on this now and post a patch soon

sphism’s picture

Status: Active » Needs review
FileSize
20.65 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch form.inc-coder-2054345-4.patch. Unable to apply patch. See the log in the details link for more information. View

Here's a patch which improves form.inc, doesn't fix 100% of errors reported, but it's a good start, this is what coder reports after applying the patch:

FILE: /www/drupal8/drupal/core/includes/form.inc
--------------------------------------------------------------------------------
FOUND 19 ERROR(S) AND 6 WARNING(S) AFFECTING 25 LINE(S)
--------------------------------------------------------------------------------
  697 | ERROR   | Last parameter comment requires a blank newline after it
  705 | ERROR   | Expected 1 space between asterisk and tag; 3 found
  708 | ERROR   | Expected 1 space between asterisk and tag; 3 found
  710 | ERROR   | Expected 1 space between asterisk and tag; 3 found
  714 | ERROR   | Expected 1 space between asterisk and tag; 3 found
 2298 | ERROR   | Comment indentation error, expected only 1 spaces
 2299 | ERROR   | Comment indentation error, expected only 3 spaces
 2300 | ERROR   | Comment indentation error, expected only 5 spaces
 2736 | ERROR   | Last parameter comment requires a blank newline after it
 2758 | ERROR   | Expected 1 space between asterisk and tag; 7 found
 3027 | ERROR   | Key specified for array entry; first entry has no key
 3223 | ERROR   | Key specified for array entry; first entry has no key
 3498 | ERROR   | Last parameter comment requires a blank newline after it
 3507 | ERROR   | Expected 1 space between asterisk and tag; 5 found
 3533 | ERROR   | Expected 1 space between asterisk and tag; 5 found
 4148 | WARNING | Line exceeds 80 characters; contains 81 characters
 5083 | WARNING | Line exceeds 80 characters; contains 88 characters
 5099 | WARNING | Line exceeds 80 characters; contains 96 characters
 5111 | WARNING | Line exceeds 80 characters; contains 103 characters
 5127 | WARNING | Line exceeds 80 characters; contains 91 characters
 5138 | WARNING | Line exceeds 80 characters; contains 99 characters
 5168 | ERROR   | Last parameter comment requires a blank newline after it
 5173 | ERROR   | Expected 1 space between asterisk and tag; 5 found
 5178 | ERROR   | Expected 1 space between asterisk and tag; 5 found
 5185 | ERROR   | Expected 1 space between asterisk and tag; 5 found
--------------------------------------------------------------------------------

So many of the errors require rewriting function calls and/or arrays across multiple lines, I think this is better but honestly I think a lot of it should just be written in a different style, for example:

$batch['error_message'] = t('Please continue to <a href="@error_url">the error page</a>',
        array(
          '@error_url' => url(
            $url,
            array(
              'query' => array(
                'id' => $batch['id'],
                'op' => 'finished',
              ),
            )
          ),
        )
      );
AjitS’s picture

4: form.inc-coder-2054345-4.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 4: form.inc-coder-2054345-4.patch, failed testing.

sergeypavlenko’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
7.08 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,655 pass(es). View

Hi

I removed all the unnecessary change lines. Making actual patch for the current version of Drupal 8.

sergeypavlenko’s picture

Assigned: sphism » sergeypavlenko

Change assigned.

dcam’s picture

Issue summary: View changes
Issue tags: +Needs manual testing

Updating in preparation for DrupalCon Austin sprints. See http://www.hook42.com/blog/prepping-drupalcon-austin-sprints-sprint-lead....

sergeypavlenko’s picture

Hi

Why here need manual tests? If the code has not been edited.

dcam’s picture

Issue summary: View changes
Issue tags: -Needs manual testing

Good point. It just needs reviewing, not actual testing.

Jalandhar’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Patch #7 needs reroll.

sergeypavlenko’s picture

Status: Needs work » Needs review
FileSize
4.25 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2054345-13.patch. Unable to apply patch. See the log in the details link for more information. View

Hi

I'm rerilled patch.

apratt queued 13: 2054345-13.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 13: 2054345-13.patch, failed testing.

apratt’s picture

Much of this patch involves deprecated code now and the new code appears to be coder compliant. I suspect this issue could be closed.

xjm’s picture

Version: 8.0.x-dev » 8.1.x-dev
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.

xjm’s picture

Assigned: sergeypavlenko » Unassigned
Issue tags: -Novice
pfrenssen’s picture

Status: Postponed » Fixed

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.

pfrenssen’s picture

Status: Fixed » Closed (duplicate)