Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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/*.*
Comment | File | Size | Author |
---|---|---|---|
#13 | core-coder-fixes-1605318-13.patch | 25.66 KB | TravisCarden |
#11 | core-coder-fixes-1605318-11.patch | 26.19 KB | TravisCarden |
#9 | core-coder_review-1605318-9.patch | 1.29 KB | underq |
#5 | core-coder_review-1605318-5.patch | 1.36 KB | underq |
#1 | core-coder_review-1605318-1.patch | 3.72 KB | underq |
Comments
Comment #1
underq CreditAttribution: underq commentedI think there are some errors with my new comments, because my english skill is low :)
Comment #2
TravisCarden CreditAttribution: TravisCarden commentedThanks @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.
Comment #3
jhodgdonRE #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:
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...
This should probably be:
Comment #4
TravisCarden CreditAttribution: TravisCarden commentedNew docs issue: #1606946: Clean up API docs for files in core directory.
Comment #5
underq CreditAttribution: underq commentedThanks for your help.
@TravisCarden, @jhodgdon I haven't edit doxygen missing, but I have resolved syntax errors.
Comment #6
underq CreditAttribution: underq commentedoups
Comment #7
TravisCarden CreditAttribution: TravisCarden commentedMy only concern with moving the
// update.php ops.
comment is that it doesn't seem to be meant describe the wholeswitch
but rather to distinguish thecase
s from thedefault
. Notice that last comment:Comment #8
jhodgdonOK, my mistake on moving that comment then! Let's get it moved back (without the blank line), and call it good.
Comment #9
underq CreditAttribution: underq commentedApply #7
Comment #10
jhodgdonThat's better. :)
Now all we need is an issue summary (see step #7 in the summary of #1518116: [meta] Make Core pass Coder Review).
Comment #11
TravisCarden CreditAttribution: TravisCarden commentedThe 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.
Comment #13
TravisCarden CreditAttribution: TravisCarden commentedUpstream changes. Here's a new patch.
Also, drupalcs still complains about one issue with this patch applied:
It refers to this:
@jhodgdon, perhaps you could suggest a resolution to this one since it would involve changing the substance of the comment.
Comment #14
jhodgdonRE #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
Comment #15
TravisCarden CreditAttribution: TravisCarden commentedRe #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.
Comment #15.0
TravisCarden CreditAttribution: TravisCarden commentedUpdated issue summary.
Comment #16
jhodgdonOK. 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?
Comment #17
TravisCarden CreditAttribution: TravisCarden commented@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.
Comment #17.0
TravisCarden CreditAttribution: TravisCarden commentedUpdated summary.
Comment #18
Anonymous (not verified) CreditAttribution: Anonymous commentedActually 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?
Comment #19
jhodgdonUm... Thanks, but you only tested Coder on 13 files. There are several more files than that in Core.
Comment #20
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?
Comment #21
jhodgdonOh 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.
Comment #22
Anonymous (not verified) CreditAttribution: Anonymous commentedI 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?
Comment #23
jhodgdonOK. 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!
Comment #24
mgiffordComment #27
xjmThanks 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 #28
xjmComment #29
pfrenssenClosing 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.