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

  • The files in the core directory fail on the Drupal 8 branch test code review tab with critical and normal errors. The failures are not itemized on that tab.
  • Drupal Code Sniffer reports seven errors with the latest patch here applied: 
    FILE: /home/quickstart/websites/d8.dev/core/update.php
--------------------------------------------------------------------------------
FOUND 7 ERROR(S) AFFECTING 7 LINE(S)
--------------------------------------------------------------------------------
  46 | ERROR | Missing function doc comment
  56 | ERROR | Missing function doc comment
 160 | ERROR | You must use "/**" style comments for a function comment
 170 | ERROR | You must use "/**" style comments for a function comment
 250 | ERROR | You must use "/**" style comments for a function comment
 275 | ERROR | You must use "/**" style comments for a function comment
 329 | ERROR | Function comment short description must be on a single line
--------------------------------------------------------------------------------

    All of these should be dealt with in #1606946: Clean up API docs for files in core directory rather than here.

  • The d8 Coder sandbox isn't working for me anymore. Maybe someone else can figure it out.

Note: You can easily limit drupalcs to testing the files immediately under the "core" directory by using this command from your Drupal site document root: drupalcs core/*.*

CommentFileSizeAuthor
#13 core-coder-fixes-1605318-13.patch25.66 KBTravisCarden
#11 core-coder-fixes-1605318-11.patch26.19 KBTravisCarden
#9 core-coder_review-1605318-9.patch1.29 KBunderq
#5 core-coder_review-1605318-5.patch1.36 KBunderq
#1 core-coder_review-1605318-1.patch3.72 KBunderq
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

underq’s picture

underq CreditAttribution: underq commented
Status: Active » Needs review
FileSize
core-coder_review-1605318-1.patch3.72 KB

I think there are some errors with my new comments, because my english skill is low :)

TravisCarden’s picture

TravisCarden CreditAttribution: TravisCarden commented
Status: Needs review » Needs work

Thanks @underq. The missing doxygen comment blocks should be handled in #1310084: [meta] API documentation cleanup sprint. It looks like the files in the "core" directory might have been overlooked in that initiative, though, so you could open a new issue for it there.

While removing the line breaks after all the inline comments gets drupalcs to stop complaining, I think we need comment from some other core devs as to whether that adversely affects the meaning and effectiveness of those comments.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

RE #2
- Yes you are right! Those files like update.php were missed in the API docs cleanup sprint, and we need a new issue.
- Line breaks... it depends...

This looks good, because the comment clearly goes with the next two lines of code:

 // update_fix_d8_requirements() needs to run before bootstrapping beyond path.
 // So bootstrap to DRUPAL_BOOTSTRAP_LANGUAGE then include unicode.inc.
-
 drupal_bootstrap(DRUPAL_BOOTSTRAP_LANGUAGE);
 include_once DRUPAL_ROOT . '/core/includes/unicode.inc';

This doesn't look so good, because I don't think this comment belongs with the next lines of code (though I am not sure?). Maybe the comment actually should be moved up a line? That comment does not follow our coding standards for comments anyway...

   switch ($op) {
     // update.php ops.
-
     case 'selection':

This should probably be:

   // Go through the update.php operations.
   switch ($op) {
     case 'selection':
TravisCarden’s picture

TravisCarden CreditAttribution: TravisCarden commented

New docs issue: #1606946: Clean up API docs for files in core directory.

underq’s picture

underq CreditAttribution: underq commented
FileSize
core-coder_review-1605318-5.patch1.36 KB

Thanks for your help.

@TravisCarden, @jhodgdon I haven't edit doxygen missing, but I have resolved syntax errors.

underq’s picture

underq CreditAttribution: underq commented
Status: Needs work » Needs review

oups

TravisCarden’s picture

TravisCarden CreditAttribution: TravisCarden commented

My only concern with moving the // update.php ops. comment is that it doesn't seem to be meant describe the whole switch but rather to distinguish the cases from the default. Notice that last comment:

switch ($op) {
  // update.php ops.

  case 'selection':
    // ...

  case 'Apply pending updates':
    // ...

  case 'info':
    // ...

  case 'results':
    // ...

  // Regular batch ops : defer to batch processing API.
  default:
    // ...
}
jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Needs work

OK, my mistake on moving that comment then! Let's get it moved back (without the blank line), and call it good.

underq’s picture

underq CreditAttribution: underq commented
Status: Needs work » Needs review
FileSize
core-coder_review-1605318-9.patch1.29 KB

Apply #7

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

That's better. :)

Now all we need is an issue summary (see step #7 in the summary of #1518116: [meta] Make Core pass Coder Review).

TravisCarden’s picture

TravisCarden CreditAttribution: TravisCarden commented
FileSize
core-coder-fixes-1605318-11.patch26.19 KB

The patch in #9 fixes all the issues in the PHP files, but drupalcs reports problems with the text files, too. Here's a new patch.

Status: Needs review » Needs work

The last submitted patch, core-coder-fixes-1605318-11.patch, failed testing.

TravisCarden’s picture

TravisCarden CreditAttribution: TravisCarden commented
Status: Needs work » Needs review
FileSize
core-coder-fixes-1605318-13.patch25.66 KB

Upstream changes. Here's a new patch.

Also, drupalcs still complains about one issue with this patch applied:

FILE: /home/quickstart/websites/d8.dev/core/update.php
--------------------------------------------------------------------------------
332 | ERROR | Function comment short description must be on a single line
--------------------------------------------------------------------------------

It refers to this:

/**
 * Returns (and optionally stores) extra requirements that only apply during
 * particular parts of the update.php process.
 */
function update_extra_requirements($requirements = NULL) {
  // ...
}

@jhodgdon, perhaps you could suggest a resolution to this one since it would involve changing the substance of the comment.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

RE #13 - that issue should be fixed in the API docs cleanup issue (which you filed separately, yes?). This issue should be postponed until that one is fixed.
#1606946: Clean up API docs for files in core directory

TravisCarden’s picture

TravisCarden CreditAttribution: TravisCarden commented

Re #14: Oh, that would be covered by the API cleanup intiative. (I wish I'd read the whole summary over there sooner!) In that case, there's nothing else to do here. And actually, this patch shouldn't conflict with anything in that initiative. I'll still postpone it if you prefer, but I think it's safe to RTBC. It's a very low-impact patch.

TravisCarden’s picture

TravisCarden CreditAttribution: TravisCarden commented
Issue summary: View changes

Updated issue summary.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

OK. I don't have a problem with this patch, but I wouldn't want to mark this issue fixed until Coder reports back clean, which is I thought why the "pass coder review" issues were in general being postponed until the "clean up API docs" issues were done?

TravisCarden’s picture

TravisCarden CreditAttribution: TravisCarden commented
Status: Needs review » Postponed

@jhodgdon: I had been trying to advance any individual issues that didn't seem dependent on the API docs cleanup, but you make a good point. Postponing.

TravisCarden’s picture

TravisCarden CreditAttribution: TravisCarden commented
Issue summary: View changes

Updated summary.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Issue summary: View changes

Actually the coder review brings no warnings:

$ drush coder core/*.txt core/*.php --no-empty
Drupal Coding Standards

Status Messages:
Coder found 13 projects, 13 files, 0 warnings were flagged to be ignored

Coder found 13 projects, 13 files, 0 warnings were flagged to be ignored

This issue can be closed?

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

Um... Thanks, but you only tested Coder on 13 files. There are several more files than that in Core.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

@jhodgdon: I know that Drupal core consists of much more than 13 files.

But there ist a meta issue (https://drupal.org/node/1518116) that covers the hole D8 core. This issue (as I understand it) covers only files in the root folder of core/ hierarchy.
There are a lot of issues that cover core/includes/, core/themes/, profiles/ and core/modules/.

Or do you mean the core/lib/ folder?

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

Oh sorry! I thought this was the meta-issue.

But looking at the list on #1518116: [meta] Make Core pass Coder Review, I guess that this issue should cover everything directly in the core directory, plus everything under assets, misc, scripts, tests, and lib. Directories modules, themes, includes, and profiles are covered by other issues.

However, we may need to split this issue up. When the meta-issue was first created, I think this portion was much smaller than it is now. It may be too big for a single issue.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

I agree, at least the core/lib/ folder should be split into another issue.
The most files in core/{assets,misc,scripts} are not covered by the coder review and wont blow this issue (IMO).

I looked initially at the attached diff and concluded that only the files in the root of core/ are covered by this issue. We should be more precise in the issue summary?

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

OK. Please file a separate issue then and add it to the main issue, or at least go back there and add a comment stating that the core/lib directory is not covered. Then this one can be closed. Thanks!

mgifford’s picture

mgifford
he/him
Primary language English
CreditAttribution: mgifford commented
Status: Postponed » Active
Related issues: +#1606946: Clean up API docs for files in core directory

Status: Active » Needs review

darol100 queued 13: core-coder-fixes-1605318-13.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 13: core-coder-fixes-1605318-13.patch, failed testing.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Version: 8.0.x-dev » 8.1.x-dev
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

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Priority: Normal » Minor
Issue tags: -Novice
pfrenssen’s picture

pfrenssen
Location Sofia
CreditAttribution: pfrenssen commented
Status: Postponed » Closed (duplicate)

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.