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
StatusFileSize
new20.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
StatusFileSize
new7.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
StatusFileSize
new4.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