Make form.inc pass Coder Review [#2054345]

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

Comment	File	Size	Author
#13	2054345-13.patch	4.25 KB	sergeypavlenko
#13
#7	2054345-7.patch	7.08 KB	sergeypavlenko
#7
#4	form.inc-coder-2054345-4.patch	20.65 KB	sphism
#4

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

sphism CreditAttribution: sphism commented 31 July 2013 at 06:35

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
--------------------------------------------------------------------------------

Comment #2

sphism CreditAttribution: sphism commented 31 July 2013 at 06:35

Assigned:

Unassigned

» sphism

Comment #3

sphism CreditAttribution: sphism commented 9 August 2013 at 00:40

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

Comment #4

sphism CreditAttribution: sphism commented 9 August 2013 at 04:11

Status:

Active

» Needs review

File	Size
form.inc-coder-2054345-4.patch	20.65 KB

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',
              ),
            )
          ),
        )
      );

Comment #5

AjitS

he/him

Marathi

Pune

CreditAttribution: AjitS commented 30 May 2014 at 13:57

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

Comment #6

30 May 2014 at 13:58

Status:

Needs review

» Needs work

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

Comment #7

sergeypavlenko CreditAttribution: sergeypavlenko commented 1 June 2014 at 15:55

Issue summary:	View changes
Status:	Needs work	» Needs review

File	Size
2054345-7.patch	7.08 KB

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

Comment #8

sergeypavlenko CreditAttribution: sergeypavlenko commented 1 June 2014 at 17:47

Assigned:

sphism

» sergeypavlenko

Change assigned.

Comment #9

dcam CreditAttribution: dcam commented 6 June 2014 at 05:25

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....

Comment #10

sergeypavlenko CreditAttribution: sergeypavlenko commented 6 June 2014 at 06:26

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

Comment #11

dcam CreditAttribution: dcam commented 6 June 2014 at 06:29

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

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

Comment #12

Jalandhar CreditAttribution: Jalandhar commented 3 September 2014 at 06:17

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

Patch #7 needs reroll.

Comment #13

sergeypavlenko CreditAttribution: sergeypavlenko commented 10 September 2014 at 08:46

Status:

Needs work

» Needs review

File	Size
2054345-13.patch	4.25 KB

I'm rerilled patch.

Comment #14

24 October 2014 at 19:57

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

Comment #15

24 October 2014 at 20:04

Status:

Needs review

» Needs work

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

Comment #16

apratt CreditAttribution: apratt commented 25 October 2014 at 16:28

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

Comment #17

xjm

she/her

English

CreditAttribution: xjm commented 12 November 2014 at 00:20

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.

Comment #18

xjm

she/her

English

CreditAttribution: xjm commented 12 November 2014 at 00:23

Assigned:	sergeypavlenko	» Unassigned
Issue tags:	-Novice

Comment #19

pfrenssen

Sofia

CreditAttribution: pfrenssen commented 21 September 2015 at 11:52

Status:

Postponed

» Fixed

Closing in favor of #2571965: [meta] Fix PHP 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.