Make includes directory, files starting with D-G pass Coder Review

I'm going to see if I can tidy up these files...

Starting with a very simple fix to errors.inc

changing status

Simple change for form.inc

Not sure what to do with these errors in fom.inc, eg:
Potential problem: form_set_error() and form_error() only accept filtered text, be sure all !placeholders for $variables in t() are fully sanitized using check_plain(), filter_xss() or similar

In the gettext.inc file there's this issue:

Potential problem: drupal_set_message() only accepts filtered text, be sure all !placeholders for $variables in t() are fully sanitized using check_plain(), filter_xss() or similar. (Drupal Docs)

So all I've done is wrap the 2 variables in check_plain()

I'm not entirely sure this is correct when using the get_t() function instead of regular t()...

Hey, @sphism. Thanks for taking this on! We have a one-patch-per-issue policy, so you'll want to make all your changes in a single batch. I'll follow this issue and help if needed. :)

Also, we've amended the initiative such that working against Drupal Code Sniffer errors—at least until we have a working version of Coder Review for d8. The sniffer is much more thorough.

Hi Travis,

Ok I'll roll all the changes into 1 patch and post it in here. TO be honest I was pretty happy just to get my head around the issue queue, upload a patch, and have it pass the test. First time I've done it.

I written a lot of custom drupal code, and got a lot of it through coder in the past so figured I'd come help out.

I had trouble installing drupal code sniffer on my Mac... but can try again, or is it ok to use the coder 8.x port: http://drupal.org/node/1539824

For the currently agreed-upon workflow, @sphism, see #1518116-21: [meta] Make Core pass Coder Review. If you're having trouble with it, jump into #Drupal-Contribute on IRC and ask for help. If you simply can't manage it right now, just upload a patch for what you've already done, explain what it includes, and unassign the issue from yourself so someone else can pick up where you left off. Good job trying something new and contributing to the community!

Hey Travis, spent a loooong time trying to get PEAR, phpcs and drupalcs all working on my mac... it seems like it's nearly working but can't quite get it to do anything, I'll see if I can get any help on irc.

I've put all the changes I've done into 1 patch file and uploaded it here.

... and changed status

Until you can get drupalcs working on your machine, @sphism, here's its output after your patch has been applied. Warning: it's looong. This patch is going to take a lot of work!

Hey Travis, thanks for that.

The last project I worked on (for 18 months) had over 2000 coder issues, got it down to Zero :)

I'll see how far I can get with it... now I can actually see what I need to fix it will be much easier, thanks for uploading that file.

You're very welcome, @sphism. Hopefully this patch won't take you 18 months! ;-)

ha, writing 10 modules took 18 months, fixing 2000+ coder issues only took a week or so, lots of them were changes in the coding standards from drupal 5 to 6.

Having a look at them now.

Not sure about the arrays in date.inc that are over 80 chars

I think maybe they looked better before, but here it is combined with the other changes... and already I'm being called to go do something else :) will look more tomorrow.

cool, thanks for the heads up about whitespace at the ends of lines.

There was 1 array shorter than 80 chars, but seemed odd to just have 1 like that so i changed it...

(UPDATED) Regarding #1518116-33: [meta] Make Core pass Coder Review, I'd do either this:

element_set_attributes($element,  array(
  'id',
  'name',
  'value',
  'size',
  'step',
  'min',
  'max',
  'maxlength',
  'placeholder',
));

or this:

element_set_attributes(
  $element,  
  array(
    'id',
    'name',
    'value',
    'size',
    'step',
    'min',
    'max',
    'maxlength',
    'placeholder',
  )
);

I prefer the second but apparently @jhodgon prefers the first, so no clear consensus on which. But one of the two. :)

Regarding #1518116-32: [meta] Make Core pass Coder Review:

in form.inc there are lines like this:

$batch =& batch_get();

which get this error:
4597 | ERROR | An operator statement must be followed by a single space

looks fine to me ???

I'd say that's a bug. When we find bugs like that, we should file an issue against the relevant (either drupalcs or coder):

1. Search the project issue queue to see if a bug report is already filed.
2. File an issue if you don't find one.
3. Link the issue here in your summary along with the failure it causes. (See #1512434: Make Aggregator module pass Coder Review for a lovely.)
4. Also add the issue to the summary at #1361508: [META] Tracking issue for Coder Advisory Review test issues

Thanks!

And finally, regarding #1518116-30: [meta] Make Core pass Coder Review:

should this:
in file.inc

    // Unknown error.
    default:
      drupal_set_message(t('The file %file could not be ..........

be this

    default:
      // Unknown error.
      drupal_set_message(t('The file %file could not be ..........

the error is : 1516 | ERROR | Line indented incorrectly; expected at least 6 spaces, found 4 (line 1516 is // Unknown error.)

Neither is clearly right or wrong to me (with the inline comments for the switch statement), so let's have some more review on it here in this issue, and if there isn't a clear path we can open a coding standards issue for it.

Thanks for all your work on this! I've spent some time inside those files and I know just how much fun they are. ;)

@xjm thanks for such a quick response, here's a patch which contains everything except the things I asked about. Finally getting somewhere with this :) Thanks for your help.

Regarding: element_set_attributes($element, array('id', 'name', 'value', 'size', 'step', 'min', 'max', 'maxlength', 'placeholder'));

With errors like this:
3963 | ERROR | If the line declaring an array spans longer than 80 characters, each element should be broken into its own line

Personally I never do this, it becomes incredibly ugly if you have say function($string, $array, $string2, $array2);

element_set_attributes($element,  array(
  'id',
  'name',
  'value',
  'size',
  'step',
  'min',
  'max',
  'maxlength',
  'placeholder',
));

This I like slightly more, but I never do it

element_set_attributes(
  $element,  
  array(
    'id',
    'name',
    'value',
    'size',
    'step',
    'min',
    'max',
    'maxlength',
    'placeholder',
  )
);

This is what I do in my own code, ie I don't split inline arrays in function calls:

$map = array(
  'id',
  'name',
  'value',
  'size',
  'step',
  'min',
  'max',
  'maxlength',
  'placeholder',
);
element_set_attributes($element, $map);

Presumably you would need to unset($map) after you used it though so maybe people don't like it...

If you don't like the 3rd one then I'd go with the 2nd over the 1st for the reason mentioned.... looking at it again I would only do the 3rd one, the first 2 just look wrong.

Is there a particular reason the 3rd one isn't preferred?

see #1547082: Don't throw error for =& for an issue regarding:

$batch =& batch_get();

which get this error:
4597 | ERROR | An operator statement must be followed by a single space

regarding #23 i'm not sure how best to comment the cases within a switch statement

Re: #24, the first two are not wrong at all; those patterns are used throughout core and I use the second in my own code. (I dislike the first for the same reason you describe). The third is less desirable because it defines a throw-away variable that is not used anywhere else, which is needless overhead and also more to figure out when you read the function (it's not actually "variable"). That's our practice generally. I'd say only use a throw-away map in something like a foreach.

Thanks for filing the followup.

Ok well that explains it, i'll go with the 2nd one then.

By the way I got drupalcs set up on my mac :) turns out the phpcs file was looking for CodeSniffer/CLI.php in the wrong place, MAMP puts things in quite bizarre places.

Here's the patch with the form.inc element_set_attributes formatted as the 2nd option in #24

Thanks NROTC_Webmaster... and damn those infernal trailing whitespaces :)

Yeah the file.mimetypes.inc only had 2 errors and both were the return types so I just added them, but can remove.

So with the comments I split them where the sentence naturally breaks, (graphic design background) but I can change that so it just splits as close to 80 char limit (although personally i find it harder to read)

This patch doesn't make everything pass, but it's a lot closer, one question I'm not sure about is how to comment cases in a switch statement which I manages to accidentally post in the meta thread instead of here: #1518116-31: [meta] Make Core pass Coder Review

The other thing i'm not really sure about is when you have somthing like:

form.inc
$batch['form_state'] = array_intersect_key($form_state, array_flip(array('programmed', 'rebuild', 'storage', 'no_redirect', 'redirect')));
error: 890 | ERROR ? | If the line declaring an array spans longer than 80 characters, each element should be broken into its own line

I guess it could be:

$batch['form_state'] = array_intersect_key($form_state, array_flip(array(
  'programmed',
  'rebuild',
  'storage', 
  'no_redirect',
  'redirect'
)));

but i'm not hugely keen on that, any thoughts?

I'm done for today but i'll go through and tidy up everything you've suggested and make other changes within the next couple days

Here's the next patch, which takes patch from #29 and applies all the changes mentioned in #30

The only thing i didn't do was change the form.inc form_select_options($element, $choices = NULL) {... summary, since it's already been changed and committed.

I think I can now go though and make the final changes which i had queries about.

This patch is getting close to being finished, I think the only remaining things are false positives.

Running each file through drupalcs:

date.inc: no errors - Done

errors.inc: ~15 errors for @param and @return types - Done

file.mimetypes.inc: 2 errors for @param and @return types - Done

form.inc: no errors - Done

file.inc: Lots of @param and @return type errors but everything else is Done (except issues below)

This one is inside @code so it's fine:

 * @code
 *   $remote_stream_wrappers = array_diff_key(file_get_stream_wrappers(STREAM_WRAPPERS_ALL), file_get_stream_wrappers(STEAM_WRAPPERS_LOCAL));
 * @endcode
 *
166 | WARNING | Line exceeds 80 characters; contains 141 characters

Seems fine to use a case to 'Not Do' something

case FILE_EXISTS_REPLACE:
  // Do nothing here, we want to overwrite the existing file.
  break;

1024 | ERROR   | Empty CASE statements are not allowed

gettext.inc: Lots of @param and @return type errors but everything else is Done (except issues below)
This one relates to how we comment cases in a switch statement, I think this is correct

    // Store string in memory (only supports single strings).
    case 'mem-store':
      $strings[isset($value['msgctxt']) ? $value['msgctxt'] : ''][$value['msgid']] = $value['msgstr'];
      return;

    // Called at end of import to inform the user.
    case 'db-report':

388 | ERROR | Line indented incorrectly; expected at least 6 spaces, found 4
  393 | ERROR | Line indented incorrectly; expected at least 6 spaces, found 4

That 2nd line is 81 characters long - seems fine to me and matches the formatting of similar code around it

else {
  $f = "(" . $element_stack[$i - 2] . $op . $element_stack[$i - 1] . ")";
}

649 | ERROR | If the line declaring an array spans longer than 80 characters, each element should be broken into its own line

... and changing status...

+++ b/core/includes/errors.incundefined
@@ -108,7 +108,8 @@ function _drupal_decode_exception($exception) {
+    // The first element in the stack is the call,
+    // the second element gives us the caller.

Comments should wrap at the laste word just before the 80-column limit. In this case they should wrap just after the word "us."

+++ b/core/includes/errors.incundefined
@@ -184,7 +185,8 @@ function error_displayable($error = NULL) {
+  // Do it early because drupal_set_message()
+  // triggers a drupal_theme_initialize().

same comment as above

+++ b/core/includes/errors.incundefined
@@ -252,8 +254,8 @@ function _drupal_log_error($error, $fatal = FALSE) {
+      // We fallback to a maintenance page at this point,
+      // because the page generation itself can generate errors.

same comment as above

+++ b/core/includes/file.incundefined
@@ -826,7 +827,11 @@ function file_usage_delete(stdClass $file, $module, $type = NULL, $id = NULL, $c
+      watchdog('file', 'File %file (%realpath) could not be copied because the destination %destination is invalid. This is often caused by improper use of file_copy() or a missing stream wrapper.', array(
+        '%file' => $source->uri,
+        '%realpath' => $realpath,
+        '%destination' => $destination,

I'm not sure about this one... I cant find anything in http://drupal.org/coding-standards#array, but it just seems wrong. Anyone?

+++ b/core/includes/form.incundefined
@@ -1980,7 +1982,7 @@ function _form_builder_handle_input_element($form_id, &$element, &$form_state) {
+      // be explicitly distinguished from missing input (see below).

Capitalize "B"

-26 days to next Drupal core point release.

Thanks everyone! Couple followups for #36:

+++ b/core/includes/form.incundefined
@@ -1980,7 +1982,7 @@ function _form_builder_handle_input_element($form_id, &$element, &$form_state) {
+      // be explicitly distinguished from missing input (see below).

Capitalize "B"

No, actually. This comment is the second half of a sentence that begins on the line above it.

Regarding the array formatting question, see #1539712: [policy, no patch] Coding standards for breaking function calls and language constructs across lines and also background discussion in #1512434: Make Aggregator module pass Coder Review. I personally would perfer to see each parameter on its own line, with the array children (of course) indented, but @jhodgdon has preferred the format here.

I personally would perfer to see each parameter on its own line, with the array children (of course) indented, but @jhodgdon has preferred the format here.

I'm personally with you. Do we have an issue open somewhere to discuss this?

Thanks for the feedback :)

I'll get on and make these changes within the next few days.

Ok, so I slacked off for a while and patch #34 no longer applies cleanly, so i followed the instructions here:

http://learndrupal.org/lesson/b93deb54-264f-19a4-49eb-db74bc6c2712

and re-rolled the patch. It just needed one change to form.inc

Now that it applies cleanly to 8.x 352645e I will fix form.inc, apply the changes mentioned in #36 and #37 and upload another patch soon....

This patch extends the pre-rolled patch from #40 and includes all feedback from #36 and #37 plus a few alterations based on the formatting of #1539712-7: [policy, no patch] Coding standards for breaking function calls and language constructs across lines

I also found a couple more places where I had split the comments on a comma instead of the 80 char limit...

Hoping this one passes review :)

Yeah I should be able to make an interdiff between #40 and #41

Right this should be an interdiff between #40 and #41 above.

Note that there's a change to form.inc function theme_number($variables) --- this is the part that changed since #34 and had to be re-rolled in #40

Thanks, @NROTC_Webmaster. The outcome of #1539712: [policy, no patch] Coding standards for breaking function calls and language constructs across lines will affect this patch. I'm postponing this issue until it's resolved.

postponed...noooooooo......

:(

ho hum, just hoping i don't have to re-roll that patch again :)

No worries

Hehe. Thanks for bein' a sport, @sphism. ;-)

We have the go ahead with all these issues again, see #1518116: [meta] Make Core pass Coder Review for more details

Right, after mucking about all day with the broken coder 8.x module and php code sniffer I think I finally have the proper drupalcs output for the include files D-G: (this excludes missing return and param type errors as per #36)

FILE: /www/drupal8/drupal/core/includes/database.inc
--------------------------------------------------------------------------------
FOUND 7 ERROR(S) AND 2 WARNING(S) AFFECTING 9 LINE(S)
--------------------------------------------------------------------------------
   2 | ERROR   | Missing file doc comment
  40 | WARNING | Line exceeds 80 characters; contains 81 characters
 112 | WARNING | Line exceeds 80 characters; contains 82 characters
 533 | ERROR   | Missing comment for @return statement
 542 | ERROR   | Missing comment for @return statement
 551 | ERROR   | Missing comment for @return statement
 563 | ERROR   | Last parameter comment requires a blank newline after it
 565 | ERROR   | Missing comment for @return statement
 666 | ERROR   | Missing function doc comment
--------------------------------------------------------------------------------

FILE: /www/drupal8/drupal/core/includes/entity.api.php
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
 260 | ERROR | Doc comment for var $entity does not match actual variable name
     |       | $translation at position 1
 414 | ERROR | Doc comment for var $display does not match actual variable name
     |       | $displays at position 3
--------------------------------------------------------------------------------

FILE: /www/drupal8/drupal/core/includes/entity.inc
--------------------------------------------------------------------------------
FOUND 8 ERROR(S) AND 2 WARNING(S) AFFECTING 10 LINE(S)
--------------------------------------------------------------------------------
 397 | ERROR   | Missing comment for @return statement
 409 | WARNING | Line exceeds 80 characters; contains 83 characters
 448 | WARNING | Line exceeds 80 characters; contains 81 characters
 519 | ERROR   | Last parameter comment requires a blank newline after it
 522 | ERROR   | Expected 1 space between asterisk and tag; 3 found
 525 | ERROR   | Expected 1 space between asterisk and tag; 3 found
 573 | ERROR   | If the line declaring an array spans longer than 80
     |         | characters, each element should be broken into its own line
 732 | ERROR   | Parameter comment indentation must be 2 additional spaces at
     |         | position 2
 737 | ERROR   | Trailing punctuation for @see references is not allowed.
 828 | ERROR   | Trailing punctuation for @see references is not allowed.
--------------------------------------------------------------------------------

FILE: /www/drupal8/drupal/core/includes/errors.inc
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AND 4 WARNING(S) AFFECTING 6 LINE(S)
--------------------------------------------------------------------------------
 100 | WARNING | Line exceeds 80 characters; contains 90 characters
 181 | WARNING | Line exceeds 80 characters; contains 83 characters
 220 | ERROR   | Concat operator must be surrounded by spaces
 229 | WARNING | Line exceeds 80 characters; contains 81 characters
 276 | WARNING | Line exceeds 80 characters; contains 85 characters
 326 | ERROR   | If the line declaring an array spans longer than 80
     |         | characters, each element should be broken into its own line
--------------------------------------------------------------------------------

FILE: /www/drupal8/drupal/core/includes/file.inc
--------------------------------------------------------------------------------
FOUND 14 ERROR(S) AND 4 WARNING(S) AFFECTING 17 LINE(S)
--------------------------------------------------------------------------------
   78 | WARNING | Line exceeds 80 characters; contains 81 characters
  167 | WARNING | Line exceeds 80 characters; contains 142 characters
  542 | ERROR   | Expected 1 space after "="; 2 found
  643 | ERROR   | Functions must not contain multiple empty lines in a row;
      |         | found 2 empty lines
  789 | ERROR   | There must be an empty line before the parameter block
  803 | ERROR   | Inline comments must end in full-stops, exclamation marks, or
      |         | question marks
  805 | WARNING | Line exceeds 80 characters; contains 92 characters
  813 | ERROR   | Comments may not appear after statements.
  814 | WARNING | There must be no blank line following an inline comment
  814 | ERROR   | Comments may not appear after statements.
  868 | ERROR   | Inline comments must end in full-stops, exclamation marks, or
      |         | question marks
  927 | ERROR   | Doc comment for var $fid does not match actual variable name
      |         | $fids at position 1
 1003 | ERROR   | dir() is a function name alias, use getdir() instead
 1019 | ERROR   | More than 2 empty lines are not allowed
 1328 | ERROR   | unlink() is a function name alias, use domxml_unlink_node()
      |         | instead
 1331 | ERROR   | unlink() is a function name alias, use domxml_unlink_node()
      |         | instead
 1427 | ERROR   | Function comment short description must be on a single line
 1518 | ERROR   | Function comment short description must be on a single line
--------------------------------------------------------------------------------

FILE: /www/drupal8/drupal/core/includes/file.mimetypes.inc
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
 13 | ERROR | Invalid @return data type, expected array but found Array
 31 | ERROR | Invalid @return data type, expected array but found Array
--------------------------------------------------------------------------------

FILE: /www/drupal8/drupal/core/includes/database.inc
--------------------------------------------------------------------------------
FOUND 7 ERROR(S) AND 2 WARNING(S) AFFECTING 9 LINE(S)
--------------------------------------------------------------------------------
2 | ERROR | Missing file doc comment -- The @file is there but it's not at the top of the file, use Drupal\Core\Database\Database; comes first, this looks like a code sniffer error to me
40 | WARNING | Line exceeds 80 characters; contains 81 characters -- Changed the example from 10 rows to 5 rows
112 | WARNING | Line exceeds 80 characters; conta82 characters -- Altered the example a little to fit
533 | ERROR | Missing comment for @return statement -- Out of scope of this patch
542 | ERROR | Missing comment for @return statement -- Out of scope of this patch
551 | ERROR | Missing comment for @return statement -- Out of scope of this patch
563 | ERROR | Last parameter comment requires a blank newline after it -- Added newline as required
565 | ERROR | Missing comment for @return statement -- Out of scope of this patch
666 | ERROR | Missing function doc comment -- Added the following function doc comment:

/**
* Creates the keys sql from the given field specification.
*
* @param $spec
* The field specification array, as taken from a schema definition.
*/

--------------------------------------------------------------------------------

I believe these are all out of scope:

FILE: /www/drupal8/drupal/core/includes/database.inc
--------------------------------------------------------------------------------
FOUND 5 ERROR(S) AFFECTING 5 LINE(S)
--------------------------------------------------------------------------------
   2 | ERROR | Missing file doc comment
 533 | ERROR | Missing comment for @return statement
 542 | ERROR | Missing comment for @return statement
 551 | ERROR | Missing comment for @return statement
 566 | ERROR | Missing comment for @return statement
--------------------------------------------------------------------------------

FILE: /www/drupal8/drupal/core/includes/entity.api.php
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
260 | ERROR | Doc comment for var $entity does not match actual variable name
| | $translation at position 1 -- Changed to @param \Drupal\Core\Entity\EntityInterface $translation

414 | ERROR | Doc comment for var $display does not match actual variable name
| | $displays at position 3 -- changed $display to $displays
--------------------------------------------------------------------------------

All fixed

Processing entity.api.php [2764 tokens in 663 lines]... DONE in 1 second (0 errors, 0 warnings)

FILE: /www/drupal8/drupal/core/includes/entity.inc
--------------------------------------------------------------------------------
FOUND 8 ERROR(S) AND 2 WARNING(S) AFFECTING 10 LINE(S)
--------------------------------------------------------------------------------
397 | ERROR | Missing comment for @return statement -- Out of scope of this patch
409 | WARNING | Line exceeds 80 characters; contains 83 characters -- moved 'function' to next line
448 | WARNING | Line exceeds 80 characters; contains 81 characters -- Rejigged the comment paragraph
519 | ERROR | Last parameter comment requires a blank newline after it -- It has an @code...@endcode block followed by a newline, I think this may be a code sniffer issue
522 | ERROR | Expected 1 space between asterisk and tag; 3 found -- The @code block is the description of an @param - so this looks fine to me ???
525 | ERROR | Expected 1 space between asterisk and tag; 3 found -- As above ???
573 | ERROR | If the line declaring an array spans longer than 80
| | characters, each element should be broken into its own line -- made multiline function call, see #1539712: [policy, no patch] Coding standards for breaking function calls and language constructs across lines summary item 6 -- also added a comma after last array item, but no coma after last function parameter
732 | ERROR | Parameter comment indentation must be 2 additional spaces at
| | position 2 -- Added 1 missing space
737 | ERROR | Trailing punctuation for @see references is not allowed. -- Removed full stop (period) at end of @see as per https://drupal.org/node/1354#see
828 | ERROR | Trailing punctuation for @see references is not allowed. -- Removed full stop (period) at end of @see as per https://drupal.org/node/1354#see
--------------------------------------------------------------------------------

I've fixed everything except these, which I think is fine:

FILE: /www/drupal8/drupal/core/includes/entity.inc
--------------------------------------------------------------------------------
FOUND 4 ERROR(S) AFFECTING 4 LINE(S)
--------------------------------------------------------------------------------
 397 | ERROR | Missing comment for @return statement
 519 | ERROR | Last parameter comment requires a blank newline after it
 522 | ERROR | Expected 1 space between asterisk and tag; 3 found
 525 | ERROR | Expected 1 space between asterisk and tag; 3 found
--------------------------------------------------------------------------------

FILE: /www/drupal8/drupal/core/includes/errors.inc
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AND 4 WARNING(S) AFFECTING 6 LINE(S)
--------------------------------------------------------------------------------
100 | WARNING | Line exceeds 80 characters; contains 90 characters -- Rejigged the comment paragraph
181 | WARNING | Line exceeds 80 characters; contains 83 characters -- Put drupal_theme_initialize(). on the next line
220 | ERROR | Concat operator must be surrounded by spaces -- Added a space before the . concat operator
229 | WARNING | Line exceeds 80 characters; contains 81 characters -- Changed 2 single line comments into a paragraph comment
276 | WARNING | Line exceeds 80 characters; contains 85 characters -- Rejigged the comment paragraph
326 | ERROR | If the line declaring an array spans longer than 80
| | characters, each element should be broken into its own line -- Turned into multiline array as per https://drupal.org/coding-standards#array
--------------------------------------------------------------------------------

All fixed:

Processing errors.inc [2543 tokens in 391 lines]... DONE in 1 second (0 errors, 0 warnings)

FILE: /www/drupal8/drupal/core/includes/file.inc
--------------------------------------------------------------------------------
FOUND 14 ERROR(S) AND 4 WARNING(S) AFFECTING 17 LINE(S)
--------------------------------------------------------------------------------
78 | WARNING | Line exceeds 80 characters; contains 81 characters -- Moved last word to next line
167 | WARNING | Line exceeds 80 characters; contains 142 characters -- Formatted example array as multiline array
542 | ERROR | Expected 1 space after "="; 2 found -- removed space
643 | ERROR | Functions must not contain multiple empty lines in a row;
| | found 2 empty lines -- Removed additional newline
789 | ERROR | There must be an empty line before the parameter block -- Added newline before @params
803 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks -- added full stop (period)
805 | WARNING | Line exceeds 80 characters; contains 92 characters -- moved see [url] to new line comment
813 | ERROR | Comments may not appear after statements. -- Moved comment to line above code
814 | WARNING | There must be no blank line following an inline comment -- Fixed with above change
814 | ERROR | Comments may not appear after statements. -- Moved comment to line above code
868 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks -- added full stop
927 | ERROR | Doc comment for var $fid does not match actual variable name
| | $fids at position 1 -- Changed $fid to $fids, changed param description from The file id. to The file ids.
1003 | ERROR | dir() is a function name alias, use getdir() instead -- NOT Fixed: is this correct??? http://php.net/manual/en/function.dir.php
1019 | ERROR | More than 2 empty lines are not allowed -- Changed to 1 newline space
1328 | ERROR | unlink() is a function name alias, use domxml_unlink_node()
| | instead -- NOT FIXED: code sniff error: https://drupal.org/node/2032067
1331 | ERROR | unlink() is a function name alias, use domxml_unlink_node()
| | instead -- as above
1427 | ERROR | Function comment short description must be on a single line -- This was one line, i changed it to this:
* Creates a directory.
*
* Optionally creating missing components in the path to the directory.
1518 | ERROR | Function comment short description must be on a single line -- This was one line, i made it like this:
* Helper function.
*
* Ensures we don't pass a NULL as a context resource to mkdir().
--------------------------------------------------------------------------------

All fixed except these 3 which i believe are incorrect sniffs:

FILE: /www/drupal8/drupal/core/includes/file.inc
--------------------------------------------------------------------------------
FOUND 3 ERROR(S) AFFECTING 3 LINE(S)
--------------------------------------------------------------------------------
 1010 | ERROR | dir() is a function name alias, use getdir() instead
 1333 | ERROR | unlink() is a function name alias, use domxml_unlink_node()
      |       | instead
 1336 | ERROR | unlink() is a function name alias, use domxml_unlink_node()
      |       | instead
--------------------------------------------------------------------------------

I'm leaving both these errors because i think they are mistakes, and if they aren's then then should be fixed as part of the param and return updates:

FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
 13 | ERROR | Invalid @return data type, expected array but found Array
 31 | ERROR | Invalid @return data type, expected array but found Array
--------------------------------------------------------------------------------

Removed reference to non-existent database and filetransfer subdirectories.

This patch fixes:

• database.inc
• entity.inc
• entity.api.inc
• errors.inc
• file.inc
• file.mimetypes.inc (no changes)

I'll start up a different issue for form.inc since it contains tons of really fiddly issues and I don't want it to hold up progress on the other files

Thanks, @sphism. A few comments on the patch:

+++ b/core/includes/database.inc
@@ -34,15 +34,15 @@
  * @code
- * SELECT e.id, e.title, e.created FROM example e WHERE e.uid = $uid LIMIT 0, 10;
+ * SELECT e.id, e.title, e.created FROM example e WHERE e.uid = $uid LIMIT 0, 5;
  * @endcode

@@ -109,7 +109,7 @@
  * @code
- * INSERT INTO {example} (id, uid, path, name) VALUES (1, 2, 'home', 'Home path');
+ * INSERT INTO {example} (id, uid, path, name) VALUES (1, 2, 'path', 'Name');
  * @endcode

+++ b/core/includes/file.inc
@@ -164,7 +165,10 @@
  * @code
- *   $remote_stream_wrappers = array_diff_key(file_get_stream_wrappers(STREAM_WRAPPERS_ALL), file_get_stream_wrappers(STREAM_WRAPPERS_LOCAL));
+ *   $remote_stream_wrappers = array_diff_key(
+ *     file_get_stream_wrappers(STREAM_WRAPPERS_ALL),
+ *     file_get_stream_wrappers(STREAM_WRAPPERS_LOCAL)
+ *   );
  * @endcode

Does Coder/Sniffer complain about the line length of @code blocks in comments? Do our coding standards actually stipulate such a limit? My guess is that they don't and that Sniffer should be patched if it complains.

+++ b/core/includes/database.inc
@@ -562,6 +562,7 @@
+ * ¶

@@ -663,6 +664,12 @@
+ * ¶

+++ b/core/includes/file.inc
@@ -786,6 +789,7 @@
+ * ¶

@@ -1424,8 +1429,9 @@
+ * ¶

@@ -1515,8 +1521,9 @@
+ * ¶

Trailing whitespace.

+++ b/core/includes/entity.inc
@@ -406,8 +406,8 @@
+ * function should only be used as a callback, e.g. for menu title callbacks.

"e.g." should have a comma (,) after it, like this "...callback, e.g., for...".

+++ b/core/includes/file.inc
@@ -1424,8 +1429,9 @@
- * Creates a directory, optionally creating missing components in the path to
- * the directory.
+ * Creates a directory.
+ * ¶
+ * Optionally creating missing components in the path to the directory.

I think the second part of the original sentence is important to the summary. What about "Creates a directory, optionally creating missing path components." to keep it on one line?

There are a few issues lingering after applying the patch, too:

FILE: /var/www/d8.dev/core/includes/entity.inc
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
 519 | ERROR | Last parameter comment requires a blank newline after it
 522 | ERROR | Expected 1 space between asterisk and tag; 3 found
 525 | ERROR | Expected 1 space between asterisk and tag; 3 found
--------------------------------------------------------------------------------

FILE: /var/www/d8.dev/core/includes/file.inc
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 1010 | ERROR | dir() is a function name alias, use getdir() instead
--------------------------------------------------------------------------------

FILE: /var/www/d8.dev/core/includes/file.mimetypes.inc
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
 13 | ERROR | Invalid @return data type, expected array but found Array
 31 | ERROR | Data type of return value is missing
--------------------------------------------------------------------------------

Thanks @travis

code sniffer is complaining about those comment lengths, but i may not have it set up correctly, took ages to get it all working again

That trailing whitespace is such a pain, can i set netbeans up to take it out on save?

Those lingering errors all have comments in my posts above, I don't think any of them are real errors, the dir() one is really odd

See this question on Stackoverflow for configuring Netbeans to strip trailing whitespace on save. You can also do it from the command line.

Those lingering errors all look like genuine errors. Do you need help fixing them?

Ok, I have a new patch on it's way, it's really annoying having to do all the files together and posting one patch, should really have split all these inc files out into separate issues i guess...

FILE: /www/drupal8/drupal/core/includes/database.inc
--------------------------------------------------------------------------------
FOUND 5 ERROR(S) AND 2 WARNING(S) AFFECTING 7 LINE(S)
--------------------------------------------------------------------------------
2 | ERROR | Missing file doc comment --- it's there, there's just use [class] lines above it
40 | WARNING | Line exceeds 80 characters; contains 81 characters --- ok because it's a @code block
112 | WARNING | Line exceeds 80 characters; contains 82 characters --- ok because it's a @code block
533 | ERROR | Missing comment for @return statement --- out of scope of this patch
542 | ERROR | Missing comment for @return statement --- out of scope of this patch
551 | ERROR | Missing comment for @return statement --- out of scope of this patch
566 | ERROR | Missing comment for @return statement --- out of scope of this patch

ok... i've improved things a little, but not gone thru all the inc files... but it's dinner time so i'm stopping here :)

I'm working on this again today, i'm gonna be brutal this time and fix everything...

i'm excluding form.inc from this issue

adding followups so we can get this patch moving again

ILE: /www/drupal8/drupal/core/includes/database.inc
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 112 | WARNING | Line exceeds 80 characters; contains 82 characters
--------------------------------------------------------------------------------

This isn't really an issue since the comment is part of a code block, however, the example doesn't really make sense so i've opened a separate issue and will upload a patch shortly #2060037: Example in database.inc doesn't make sense

entity.api.php => no errors, no warnings
entity.inc => no errors, no warnings
errors.inc => no errors, no warnings

FILE: /www/drupal8/drupal/core/includes/file.inc
--------------------------------------------------------------------------------
FOUND 3 ERROR(S) AFFECTING 3 LINE(S)
--------------------------------------------------------------------------------
 1010 | ERROR | dir() is a function name alias, use getdir() instead
 1333 | ERROR | unlink() is a function name alias, use domxml_unlink_node()
      |       | instead
 1336 | ERROR | unlink() is a function name alias, use domxml_unlink_node()
      |       | instead
--------------------------------------------------------------------------------

Regarding "dir() is a function name alias, use getdir() instead" I've opened a separate issue since it requires rewriting a few lines of code #2060053: file.inc: dir() is a function name alias, use getdir() instead I'll post a patch for that in a moment

Regarding the unlink() ones, I have NO idea what to do here

file.mimetypes.inc => no errors, no warnings

form.inc: moved to separate issue #2054345: Make form.inc pass Coder Review cos there's lots to do I and didn't want to hold all these other ones up for another year ;)

another followup

Here's the new patch relating to all the changes in #63

bugger :(

How do i figure out what's causing that error?

#64: includes_d_g-1533096-64.patch queued for re-testing.

hhhmmmmm.... So the patch in #64 failed testing, I'm pretty sure it failed on 100's of tests, then i retested it, and it only failed on 1 test... So, is it possible that the testing is borked?

The patch in #61 passed testing, so I'm going to re-test both #61 and #64 - the first should pass the second should fail

If so then there are 3 code changes in that patch, formatting arrays on multiple lines, the rest are comment alterations.

2 of those code changes were in patch #61 so maybe the this part in entity.inc is the issue:

Drupal::moduleHandler()->invokeAll(
    'entity_bundle_' . $hook,
    array(
      $entity_type,
      $bundle,
      $bundle_new,
    )
  );

#61: includes_d_g-1533096-61.patch queued for re-testing.

#64: includes_d_g-1533096-64.patch queued for re-testing.

Good work, @sphism. I think this is close. I can confirm that the few code changes are merely formatting, that none of the comment line length changes mess with the substance of the comments, and that Coder Sniffer reports no more (in-scope) problems after the patch is applied. (I have not tested with Coder Review yet.) Following are the changes I would recommend.

@@ -531,6 +531,7 @@
+ *   Returns Condition object with the specified "OR" conjunction.

@@ -540,6 +541,7 @@
+ *   Returns Condition object with the specified "AND" conjunction.

@@ -549,6 +551,7 @@
+ *   Returns Condition object with the specified "XOR" conjunction.

@@ -562,7 +565,9 @@
+ *   Returns Condition object with the specified conjunction.

The subject of each of these sentences requires an article (i.e., "Returns a Condition object...".)

@@ -663,6 +668,12 @@
+ * Creates the keys sql from the given field specification.

I don't think "Creates the keys sql..." quite captures it. Maybe something more like "Creates the SQL to add table keys...".

@@ -663,6 +668,12 @@
+ * @param $spec

The data type (array) is missing. (May as well, since it's obvious, and we're touching the line anyway.)

@@ -84,7 +84,7 @@
+ *   The type of $entity; e.g., 'node' or 'user'.

I believe this should use a comma instead of the semicolon.

@@ -395,6 +402,7 @@
+ *   Returns the storage controller for this entity type.

This may be nitpicky, but I think "the given entity type" is clearer than "this entity type".

@@ -1515,8 +1519,9 @@
- * Helper function. Ensures we don't pass a NULL as a context resource to
- * mkdir().
+ * Helper function.
+ *
+ * Ensures we don't pass a NULL as a context resource to mkdir().

This isn't an adequate one line description. "Helper function to prevent passing NULL as a context resource to mkdir()." would fit.

Sweet! Thanks for the review travis... I'll make these changes Monday and post the patch

That maybe even get this committed, it only took 18 months :)

I left this as it is:

@@ -663,6 +668,12 @@
+ * @param $spec

NONE of the params in that file have their types in the doc blocks, I'd rather go through and add them all in one patch, that's part of another issue isn't it?

See: #1542800: Add missing @param in includes D-G

Yeah, this is odd "The type of $entity; e.g., 'node' or 'user'."

In british english you never write 'e.g.,' but apparently in US english you do (we mostly write 'eg' and 'ie' although formally it should be e.g. - my understanding is that a semicolon precedes a list, eg; foo, bah and baz. But the convention definitely seems to be, e.g., foo, bah and baz.)

Aaaaanyway, I've made all the other suggested changes, new patch attached

Also, is this how you make interdiffs? I've not made one before I don't think

Looks good, @sphism. As soon as it gets the green light from the testbot I'll mark it RTBC.

Did you manage to get Coder Review working in D8? If so, could you add some notes to the meta issue explaining what you had to do?

Here's how to make an interdiff: https://drupal.org/documentation/git/interdiff

The coder module for d8 doesn't work at all for me, I'm just using the command line phpcs... Standard=Drupal... Etc

The coder yml info files need to be added so you can install the modules... Which I may do, there's a few issues for that

All right. Testbot approves, and I'm ready for it to proceed. For the committer: I can confirm that the few code changes in the patch are merely formatting, that none of the comment line length changes mess with the substance of the comments, and that Coder Sniffer reports no more (in-scope) problems after the patch is applied.

Btw, @sphism, if you'd like to swap reviews, I just submitted a patch at #1533112-12: Make Contact module pass Coder Review. :)

The patch no longer applies because database.inc has changed:

56817e5f2df099298af1125ec024cb54d8739092 Aug 14, 2013 2:39:22 AM Jennifer Hodgdon
Issue #2057809 by StephaneQ, Berdir, chx, dawehner: Fix up param/return documentation in database API

Here's everything that was rejected: database.inc.rej

***************
*** 531,536 ****
   * Returns a new DatabaseCondition, set to "OR" all conditions together.
   *
   * @return Condition
   */
  function db_or() {
    return new Condition('OR');
--- 531,537 ----
   * Returns a new DatabaseCondition, set to "OR" all conditions together.
   *
   * @return Condition
+  *   Returns a Condition object with the specified "OR" conjunction.
   */
  function db_or() {
    return new Condition('OR');
***************
*** 540,545 ****
   * Returns a new DatabaseCondition, set to "AND" all conditions together.
   *
   * @return Condition
   */
  function db_and() {
    return new Condition('AND');
--- 541,547 ----
   * Returns a new DatabaseCondition, set to "AND" all conditions together.
   *
   * @return Condition
+  *   Returns a Condition object with the specified "AND" conjunction.
   */
  function db_and() {
    return new Condition('AND');
***************
*** 549,554 ****
   * Returns a new DatabaseCondition, set to "XOR" all conditions together.
   *
   * @return Condition
   */
  function db_xor() {
    return new Condition('XOR');
--- 551,557 ----
   * Returns a new DatabaseCondition, set to "XOR" all conditions together.
   *
   * @return Condition
+  *   Returns a Condition object with the specified "XOR" conjunction.
   */
  function db_xor() {
    return new Condition('XOR');
***************
*** 562,568 ****
   *
   * @param $conjunction
   *   The conjunction to use for query conditions (AND, OR or XOR).
   * @return Condition
   */
  function db_condition($conjunction) {
    return new Condition($conjunction);
--- 565,573 ----
   *
   * @param $conjunction
   *   The conjunction to use for query conditions (AND, OR or XOR).
+  *
   * @return Condition
+  *   Returns a Condition object with the specified conjunction.
   */
  function db_condition($conjunction) {
    return new Condition($conjunction);

Here's the reroll, excluding the return documentation we had included.

Right, patch #80 no longer applies, because of this commit:

3cf100d - webchick <webchick@24967.no-reply.drupal.org> Aug 15, 2013
Issue #1957142 by damiankloip, fubhy, cosmicdreams: Replace config() with Drupal::config().

Here's the new one

added link to form.inc issue

Unfortunately, this patch no longer applies. A few things to consider when rerolling:

• Adding doxygen type hinting has been descoped from this initiative and moved to #1800046: [META] Add missing type hinting to core docblocks, fix Drupal.Commenting.FunctionComment.Missing*, so those changes need to be removed from the patch. Please remember to follow steps 6 and 7 in the meta issue.
• Make sure that the patch addresses all issues reported by Sniffer when run as described above.
• Please don't do anything in the patch but fix coding standards issues.

Thanks!

Closed the related issue as a duplicate. It contains a patch for entity.inc in 7.x.

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.

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