Postponed until

  • We are ready to switch on automated testbot support for coding standards and add individual rules.
  • Core conforms to a single rule (needs issue).

Problem/Motivation

Drupal Core fails our automated coding standards tests.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because these are code style cleanups.
Issue priority Normal because no functional code is impacted, but following our own code style standards consistently will reduce tedium and confusion for contributors. Individual child issues should be minor.
Prioritized changes This is not a prioritized change for the beta phase.
Disruption These changes are moderately disruptive for Drupal core because they alter unrelated lines across many files, and they require careful review to ensure functional code cleanups are not introducing additional bugs or technical debt. Code style cleanups will also cause core minor version branches to diverge if they are not backported.

Proposed resolution

Let's have a "sprint" to make it pass. To participate:

  1. First, help with any of the Blocking issues. Progress on the core problem will be difficult at best until our tools are fixed. Your help in this area will be much appreciated!
  2. Next, choose an unclaimed issue from the Remaining Tasks section below.
  3. If there is not already an issue there, file a new issue and assign it to yourself. Use the following issue parameters.
    • Project: Drupal Core
    • Version: 8.x-dev
    • Component: As appropriate
    • Category: task
    • Title: Make [module or directory] pass Coder Review
    • Description: Part of meta-issue #1518116: [meta] Make Core pass Coder Review
    • Assigned: Assign it to yourself
    • Tags: coder-fixes-2012, coding standards, Novice
    • Parent issue: [meta] Make Core pass Coder Review (1518116)
  4. Edit this issue summary here, and add your issue to the Remaining Tasks section below.
  5. As always, when editing an issue summary, you should add a comment to the issue saying what you edited, so that anyone following this issue will get an email. In this case, provide a link to the new issue you filed and state which module/directory you are claiming.
  6. Clone TravisCarden's fork of Coder module, which has been modified for this initiative (git clone --branch=issue-1518116 git@github.com:TravisCarden/coder.git) and install it according to the project's instructions.
  7. Run Coder Sniffer against the relevant directory or files, e.g., drush dcs includes/.
  8. Make a patch that fixes the errors it reports in each file in your module/directory. Note: In general, please open separate issues for cleaning up things you notice that aren't directly reported by the automated code review and refrain from including fixes for them in your patches. We want to reduce as much as possible the collision between these fixes and other issues. (For a little background on why this is important, see: Diaries of a Core Maintainer #1: The Danger of Patch Context Switching.) You can also list other issues you notice under the "Remaining tasks" header of your issue summary.
  9. Attach the patch to the issue you filed, and set its status to "needs review". Leave it assigned to yourself to indicate you will continue working on it if it needs work. Monitor the issue status until it is marked "fixed".
  10. Note in your issue summary any failures that were not corrected, and why.
  11. Start over at the first step if desired!
  12. If you find that you cannot complete an issue you claimed:
    • Add a comment to the issue and un-assign it
    • If you have a partial patch, attach it to the issue
    • Return here and edit the issue summary - remove your user name from the "Assigned" column to indicate that the issue is available (but leave the link to the issue there). As usual, when editing issue summaries, also add a comment to the issue.

Pro Tips

You'll come across a few issues over and over in classes. Here are some suggestions for dealing with them:

  • When Coder Sniffer reports 'No scope modifier specified for function "X"', make it public. That's guaranteed not to break anything, and someone more knowledgeable can always correct you in review.

Blockers

The following issues are hindering or at least complicating this initiative. Resolving them will exponentially ease and speed our overall progress.

Queue Issue Notes
Coder #1805550: Modify sniff for "Missing function doc comment" for test classes Resolved.
Coder #1805544: Modify sniff for "Implements hook_foo_BAR_ID_bar() for xyz_bar()" Resolved.

(not a blocker) #1539712: [policy, no patch] Coding standards for breaking function calls and language constructs across lines (account for this by allowing for them to be wrapped or not)

Remaining tasks

The following modules and/or directories need to be done (see Proposed Resolution steps above). Add issue links and user names to the list as these are assigned. Use this format for the issue link: [#NNNN], where NNNN is the issue's node ID for the issue you filed. (NOTE: When pasting this into a line below, do not include the "code" tags, or the issues won't turn into nice color-coded links.)

Module/Directory Issue link
core
files in core directory #1605318: Make files in core directory pass Coder Review
core/includes
includes directory, files starting with A-C #1533094: Make includes directory, files starting with A-C pass Coder Review
includes directory, files starting with D-G (excluding form.inc) #1533096: Make includes directory, files starting with D-G pass Coder Review
form.inc #2054345: Make form.inc pass Coder Review
includes directory, files starting with H-M #1533100: Make includes directory, files starting with H-M pass Coder Review
includes directory, files starting with N-Z #1533102: Make includes directory, files starting with N-Z pass Coder Review
core/modules
action module #1816682: Make Action module pass Coder Review
aggregator module #1512434: Make Aggregator module pass Coder Review Assigned to: tatarbj
ban module #1816690: Make Ban module pass Coder Review
block module #1522384: Make Block module pass Coder Review
book module #1530700: Make Book module pass Coder Review
breakpoint module #1818016: Make Breakpoint module pass Coder Review
color module #1530724: Make Color module pass Coder Review
comment module #1532692: Make Comment module pass Coder Review
config module #1533202: Make config module pass Coder Review
contact module #1533112: Make Contact module pass Coder Review
contextual module #1533208: Make contextual module pass Coder Review
dblog module #1533228: Make dblog module pass Coder Review
entity module #1533230: Make entity module pass Coder Review
field module #1533232: Make field module pass Coder Review
file module #1533236: Make file module pass Coder Review
filter module #1533238: Make filter module pass Coder Review
forum module #1533240: Make forum module pass Coder Review
help module #1533244: Make help module pass Coder Review
image module #1533246: Make image module pass Coder Review
language module #1533248: Make language module pass Coder Review
locale module #1533250: Many coding standards clean-ups in locale module
menu module #1533252: Make menu module pass Coder Review
node module #1533254: Make node module pass Coder Review
path module #1533266: Make path module pass Coder Review
rdf module #1533380: Make RDF module pass Coder Review
search module #1533384: Make search module pass Coder Review
shortcut module #1533386: Make shortcut module pass Coder Review
simpletest module* #1533432: Make simpletest module pass Coder Review
statistics module #1533390: Make statistics module pass Coder Review
syslog module #1533392: Make syslog module pass Coder Review
system module (except tests subdirectory) #1533400: Make system module (except tests subdirectory) pass Coder Review
system module, tests subdirectory A-D #1533434: Make system module, tests subdirectory A-D pass Coder Review
system module, tests subdirectory E-I #1533436: Make system module, tests subdirectory E-I pass Coder Review
system module, tests subdirectory J-Z #1533438: Make system module, tests subdirectory J-Z pass Coder Review
taxonomy module #1533402: Make taxonomy module pass Coder Review
toolbar module #1533404: Make toolbar module pass Coder Review
tracker module #1533406: Make tracker module pass Coder Review
translation module #1533440: Make translation module pass Coder Review
update module #1533446: Make update module pass Coder Review
user module #1533448: Make user module pass Coder Review
core/themes
bartik theme #1533104: Make bartik theme pass Coder Review
engines directory #1539634: Make engines directory pass Coder Review
seven theme #1539636: Make seven theme pass Coder Review
stark theme #1539638: Make stark theme pass Coder Review
profiles
profiles directory #1533196: Make profiles directory pass Coder Review

* Except files subdirectory. We don't want to alter the files in the simpletest/files directory, since they are used in tests

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

TravisCarden’s picture

Issue summary: View changes

Updated references to this issue's node ID.

webchick’s picture

Awesome initiative! :D

xjm’s picture

Before you do anything, stop! Heads up!

  • Coder Tough Love (currently running as a part of the automated reviews on QA) lists many false positives.
  • However, CTL also catches some rules not yet covered by Coder.
  • The current goal is to move rules which are "official" from CTL to Coder, and then either disable or demote the priority of CTL rules and warnings.

See also:

Help with the above would be greatly appreciated. :)

xjm’s picture

xjm’s picture

I reopened #1358940: Remove Coder_Tough_Love dependency from PIFR_Coder. I recommend postponing this initiative until that is fixed.

TravisCarden’s picture

Status: Active » Postponed

Postponing per comments #2 and #4.

TravisCarden’s picture

Issue summary: View changes

Fixed a little formatting in the "Remaining tasks" table.

TravisCarden’s picture

Issue tags: +Coding standards

Adding tag.

TravisCarden’s picture

Issue summary: View changes

Fixed a couple of mistakes in issue template code.

TravisCarden’s picture

Issue summary: View changes

Added "coding standards" tag to new issue template and quick create link.

xjm’s picture

Issue summary: View changes

(xjm) Fix link.

cosmicdreams’s picture

Which version of coder should we use to check work as we go? The 7.x version? http://drupal.org/node/105916/release

TravisCarden’s picture

@cosmicdreams: Coder module doesn't seem to work for d8 yet (except on the QA server—presumably with some rigging). I've instead been using the Drupal Code Sniffer with good success. See Installation & Basic Usage.

NROTC_Webmaster’s picture

XJM,

The CTL dependency has been removed from PIFR so #1358940: Remove Coder_Tough_Love dependency from PIFR_Coder should be closed and this can get back on track.
I don't think #1299710: [meta] Automate the coding-standards part of patch review should be blocking this because it is simply testing new patches.
The only thing that I see potentially blocking this is #1361508: [META] Tracking issue for Coder Advisory Review test issues due to the missing coverage but I think that can be corrected along with any other improvements that need to be made.

Please correct me if I'm wrong though.

NROTC_Webmaster’s picture

Issue summary: View changes

Claimed block module.

TravisCarden’s picture

Issue summary: View changes

Claimed book module.

TravisCarden’s picture

Issue summary: View changes

Claimed Color module.

TravisCarden’s picture

Issue summary: View changes

Claimed Comment module.

NROTC_Webmaster’s picture

Issue summary: View changes

Updated issue summary.

NROTC_Webmaster’s picture

Issue summary: View changes

Updated issue summary.

NROTC_Webmaster’s picture

Issue summary: View changes

Added additional issues

TravisCarden’s picture

Issue summary: View changes

Added Config module.

TravisCarden’s picture

Issue summary: View changes

Claimed Contact module.

TravisCarden’s picture

Issue summary: View changes

Removed listings for non-existent includes sub-directories. Removed note about config module already passing.

NROTC_Webmaster’s picture

Issue summary: View changes

Added additional issues

NROTC_Webmaster’s picture

Issue summary: View changes

Updated issue summary.

NROTC_Webmaster’s picture

Issue summary: View changes

Updated issue summary.

sphism’s picture

Issue summary: View changes

added link to php module

sphism’s picture

Issue summary: View changes

poll module

sphism’s picture

Issue summary: View changes

rdf module

sphism’s picture

Issue summary: View changes

search module

sphism’s picture

Issue summary: View changes

shortcut

sphism’s picture

Issue summary: View changes

statistics module

sphism’s picture

Issue summary: View changes

syslog module

sphism’s picture

Issue summary: View changes

system module

sphism’s picture

Issue summary: View changes

taxonomy module

sphism’s picture

Issue summary: View changes

toolbar

sphism’s picture

Issue summary: View changes

tracker module

NROTC_Webmaster’s picture

Issue summary: View changes

Finished adding issues

xjm’s picture

Status: Postponed » Active

The latter two links were for reference and advertising. Thence "see also." :)

xjm’s picture

Issue summary: View changes

Removed non-existant Trigger module.

NROTC_Webmaster’s picture

As a note I would recommend people not to change the docblocks if the issues is still open in #1310084: [meta] API documentation cleanup sprint. I will try to go through and make comments in the specific issues when I get a chance.

NROTC_Webmaster’s picture

Issue summary: View changes

Updated issue summary.

sphism’s picture

Issue summary: View changes

assigning sphism to Includes D-G

eiriksm’s picture

Issue summary: View changes

Assigned tracker issue to myself

xjm’s picture

As a note I would recommend people not to change the docblocks if the issues is still open in #1310084: [meta] API documentation cleanup sprint. I will try to go through and make comments in the specific issues when I get a chance.

This is a good point. In fact, it might be wise to postpone the issue here on its API docs counterpart.

xjm’s picture

Issue summary: View changes

Assigning taxonomy to myself. (xjm)

lotyrin’s picture

I've started an incomplete-but-functional-for-this-purpose port of Coder to 8.x here: #1536122: Drupal 8 port https://github.com/lotyrin/coder

If anyone would prefer to use that.

NROTC_Webmaster’s picture

I have postponed the issues that relate to open issues from #1310084: [meta] API documentation cleanup sprint. I'm not saying people can't work on the postponed issues just not to make any changes to the docblocks for them.

jhodgdon’s picture

I'm trying to understand what is supposed to be addressed by this issue. The summary says to use Code Sniffer, but the title says Coder, and the comments seem to be discussing Coder. Which is right?

TravisCarden’s picture

I originally created this issue with the goal of getting core to pass Coder Review. Coder Tough Love came up because it was running on the testbot at the time, and people were concerned we not "fix" false positives it reported. That being resolved, however, we still don't have a version of Coder Review for d8, so we've instead been using Drupal Code Sniffer, which is even stricter. I know it's aggressive, but I'd like to continue using the sniffer. It's working well for people so far, it doesn't depend on a Coder Review module port, and I feel its added strictness adds real value. With your approval, we'll do so, and I'll change the issue to "Make Core conform to coding standards" to avoid confusion.

NROTC_Webmaster’s picture

I don't necessarily have a problem with this being switched to using drupalcs except for the fact that I have never used and I don't use the editors listed in the project except in very rare circumstances. I would actually lean more towards coder which requires drupal and some level a familiarity to submit corrections for it. I understand this is most likely the case if someone is using drupalcs but the project seems to indicate otherwise. The added benefit of coder is that because #1299710: [meta] Automate the coding-standards part of patch review is all about making new patches follow the best practices of coder this ensures that core already meets that requirement.

TravisCarden’s picture

drupalcs is primarily a command line tool; its integration with those editors is a secondary feature. All you need to use it is a PEAR package and a terminal. I know it's more thorough than Coder. I don't know if that means it tests everything Coder does. I notice that @webchick has an issue to integrate drupalcs with the test framework, too: #1382240: Integration with PIFR. [shrugs] In the end, I just want whatever works and does the more thorough, accurate job—which to me seems like drupalcs right now.

xjm’s picture

Here is the workflow I'd suggest:

  1. Review your directory's failures on the code review tab of the Drupal 8 branch tests if possible. (Failures for some directories are not itemized.) Correct these failures first. When you post your patch, make note in the comment or issue summary of whether you corrected these failures (as well as any that you did not correct and why), or whether your directory's failures are not itemized.
  2. Install and run Drupal Code Sniffer and correct the code style errors this module finds. As above, note in your comment of what settings/command you used, whether your directory passes drupalcs with your patch applied, as well as any failures you did not correct and why.
  3. Finally, before posting your patch, additionally test your directory with the patch applied the with the D8 port of Coder that is under development. Note in your comment what settings you used, whether the directory passes Coder, and any failures you did not correct and why.
  4. Summarize the above in your issue summary.

Note: You should not blindly correct something because a tool says it is wrong. Confirm that your changes follow our coding standards. If one of the tools gives a false positive or otherwise is incorrect, locate or file an issue for that bug with the appropriate project and link it in your post and here in the meta. For coder, also add the issue to #1361508: [META] Tracking issue for Coder Advisory Review test issues.

Does that work for everyone? It would also be good to add specific recommendations for what settings to use to the summary--maybe even a couple (cropped, reasonably sized) screenshots.

@lotyrin, would it be possible to move your work on the D8 port of coder to a Drupal.org sandbox so it is easier for us to test it and collaborate on it? Thanks!

xjm’s picture

Issue summary: View changes

Updated participation instructions to recommend use Drupal Code Sniffer until Coder module can be used on d8.

xjm’s picture

I added a step to the summary asking that participants document the results of each tool in their issues.

xjm’s picture

Issue summary: View changes

(xjm) Added section on documenting what tools were used and what failures were corrected.

jhodgdon’s picture

+1 on #21.

jhodgdon’s picture

Issue summary: View changes

Updated issue summary.

NROTC_Webmaster’s picture

Issue summary: View changes

Updated assigned modules

NROTC_Webmaster’s picture

Issue summary: View changes

Updated summary

TravisCarden’s picture

Issue summary: View changes

Reformatted the remaining tasks table a little bit. Broke themes directory into separate issues per theme. Assigned user module to lotyrin.

lotyrin’s picture

@xjm I've started the sandbox: http://drupal.org/node/1539824

lotyrin’s picture

Issue summary: View changes

Updated issue summary.

FluxSauce’s picture

Issue summary: View changes

Claiming toolbar.

FluxSauce’s picture

Issue summary: View changes

Claiming syslog

NROTC_Webmaster’s picture

Issue summary: View changes

Updated issue summary.

NROTC_Webmaster’s picture

NROTC_Webmaster’s picture

FluxSauce’s picture

Issue summary: View changes

Accepting Translation

NROTC_Webmaster’s picture

As requested in #1512434-30: Make Aggregator module pass Coder Review we will not be adding param/return types in this sprint.

NROTC_Webmaster’s picture

Issue summary: View changes

Updated issue summary.

infiniteluke’s picture

FYI
I added myself into the table above for the entity module.

infiniteluke’s picture

Issue summary: View changes

Adding myself as working on entity module

scottalan’s picture

Issue summary: View changes

Updated issue summary.

scottalan’s picture

I added myself to a few of the issues above, but wanted to wait and see if they also needed a doc block issue attached. I will at least run them through ignoring the doc blocks if that works.

NROTC_Webmaster’s picture

All of the docblocks should have been fixed in #1310084: [meta] API documentation cleanup sprint unless it was postponed for an open issue. However, if you notice something that gets flagged by coder or drupalcs and the issue in the doc sprint has been closed go ahead and fix it. The one exception is for adding datatypes to param/return which will be handled in a separate sprint.

NROTC_Webmaster’s picture

Issue summary: View changes

Updated issue summary.

sphism’s picture

Issue summary: View changes

added sphism to profiles directoy

TravisCarden’s picture

Issue summary: View changes

Unassigned rdf module.

TravisCarden’s picture

Issue summary: View changes

Unassigned user module.

TravisCarden’s picture

Issue summary: View changes

Unassigned includes directory, files starting with A-C.

sphism’s picture

Yeah I was looking at adding datatypes to param/return and it's a bit tricky, it's usually fairly obvious if it's int, string, bool but complicated if it's more like string|false or int|null or whatever. Happy to help with that sprint when it comes about, but will focus on other issues for this.

sphism’s picture

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

sphism’s picture

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

sphism’s picture

just putting all these here as I go... mainly for me to look up later, but feel free to chip in if you know what to do :)

in form.inc we have a lot of code like this:

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

Should we leave it? or do something like:

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

@sphism, rather than posting this feedback on the meta issue, please post it in the issue for the files you are working on so we don't spam everyone. I'll reply over in includes D-G.

TravisCarden’s picture

Status: Active » Postponed

Postponing pending resolution of #1539712: [policy, no patch] Coding standards for breaking function calls and language constructs across lines, as its outcome may significantly impact patches here.

TravisCarden’s picture

Issue summary: View changes

unassigned sphism from profiles directory (not sure what else I can do on it)

TravisCarden’s picture

Issue summary: View changes

Added link to instructions for silencing excluded errors.

TravisCarden’s picture

Assigned: andypost » Unassigned
Status: Active » Postponed
Issue tags: -CodeSprintCIS +Novice
FileSize
683 bytes

Edit: Please disregard in favor of #69.

TravisCarden’s picture

Issue summary: View changes

Updated issue summary.

TravisCarden’s picture

Issue summary: View changes

Added "Pro Tips" for dealing with classes.

xjm’s picture

@TravisCarden, is there an issue in the dcs queue for that? If not we should file one, even if it's a wontfix.

TravisCarden’s picture

@xjm, well the patch disables valid rules that belong in the sniffer. I just thought it would help people to disable them for this particular initiative. In other words, the patch should never be committed to the drupalcs project. Do I understand your question correctly?

jhodgdon’s picture

It looks like the patch just disables checking for param and return types, which are excluded from this initiative.

TravisCarden’s picture

That is correct, @jhodgdon.

TravisCarden’s picture

Issue summary: View changes

Added another reference to the new drupalcs patch.

TravisCarden’s picture

Issue summary: View changes

Assigned dashboard module to dinathecool.

TravisCarden’s picture

Issue summary: View changes

Added new core/*.* issue.
Removed reference to non-existent database and filetransfer subdirectories.

TravisCarden’s picture

Issue summary: View changes

Claimed overlay module.

TravisCarden’s picture

Issue summary: View changes

Updated issue assignments.

TravisCarden’s picture

Issue summary: View changes

Un-assigned dashboard module.

TravisCarden’s picture

Issue summary: View changes

Added xmlrpc module.

Tor Arne Thune’s picture

Issue summary: View changes

Assigning update.module to myself.

fotuzlab’s picture

Issue summary: View changes

Assigning xml-rpc module to self

TravisCarden’s picture

Issue summary: View changes

Removed dashboard module (removed from core: http://drupal.org/node/1697190).

TravisCarden’s picture

Issue summary: View changes

Added "Related issues" section.

jhodgdon’s picture

I have reviewed a couple of attempts at coder cleanup on the sub-issues of this meta-issue recently, and I'm seeing these problems:

a) People are adding docs headers to the getInfo() and/or setUp() methods in tests, which our coding standards currently do not support.

b) People are breaking up code lines longer than 80 characters, which our coding standards do not currently require.

c) People are adding public/protected to test methods, but not in a consistent way, and I don't think our coding standards currently require all methods to be designated as public/private/protected do they?

I'm gathering that DrupalCS is flagging these as errors even though they are not... any ideas?

douggreen’s picture

I started a 7.x-2.x version of coder, preparing for the 8.x version. See #1745068: Coder Review Drupal 8.x Battleplan

My comments/concerns with this effort:

1. I have integrated DCS with coder, so you can get the results of both with a single review. But this is still in only 7.x-2.x branch. I will have an 8.x release out as soon as I feel the 7.x-2.x new features are complete enough and stable, hopefully by Oct 1. So once this new release is out, you'll be able to just run coder with the "style" review and the "sniffer" review. The "sniffer" review will return the same results as DCS, just integrated into the coder output. This works today.

2. IMHO DCS reports too many false positives beyond our current style standards. @jhodgdon mentions a few of them. I think that the code sniffs are valuable and moving in a good direction, but they haven't yet received the community review to call them our gold standard. You can run DCS on core, but it takes a somewhat knowledgable person about our coding standards to apply the right filter on what is required and what isn't.

3. I can fix all of these problems reported by coder style review in a single patch for all of core, just like I did for 6.x #134493: Make Drupal Core Coding Standards compliant. This gets a bit more complicated when there are missing comments though. If I were to do it en masse, I'd certainly just add @todo comments. Would people prefer I take a first pass of a big patch, or continue with these smaller patches.

4. I have proposed an ignore system in #1772676: How to "ignore" warnings but need way more community input on this before we make it the standard, and start infusing it into core.

jhodgdon’s picture

RE #42... Thanks!

2. - indeed, of course I agree. :)

3. Really big patches are hard to review and difficult to get committed, since they break other patches. That is why we are breaking these issues up. I would also prefer not to have /** @todo add doc comments */ sprinkled through the code. And actually, the docs comments are being addressed on another meta issue (see summary).

4. Looks like a good idea, commenting on the other issue.

TravisCarden’s picture

I suppose our options for proceeding with this issue are (roughly*) these:

  1. More systematically and aggressively identify and patch false positives in drupalcs. @klausi has been good about accepting paches so far. In some cases though, our coding standards may need to be clarified so as to be machine-enforcable. This may be slow moving path, and it would still require some education of participants in this issue: we'll have to make sure everyone's working from the latest git clone or dev release.
  2. Relax the requirements surrounding drupalcs so it may be treated more as a loose guide than a strict gate. It might be necessary to drop the "Novice" tag and require that participants have a deeper understanding of the standards if we choose to rely more on human judgment. (That might not be a bad thing anyway.)
  3. Abandon drupalcs altogether and use Coder Review exclusively.
  4. Postpone this issue and all those that belong to it until a more satisfactory situation is reached with Coder/drupalcs.

* I omit, of course, the option of paying @jhodgdon what she's worth to just do the job for us. ;-)

jhodgdon’s picture

I am coming more and more to the conclusion that this cannot (yet) be done by novices. The tools are not working for them, and they don't know enough to ignore the false positives that are not part of the coding standards.

jhodgdon’s picture

Issue summary: View changes

took ownership of contextual module issue

theduke’s picture

I will handle dblog and statistics modules.

theduke’s picture

Issue summary: View changes

theduke is taking care of statistics module

theduke’s picture

Issue summary: View changes

Assigning dblog module

FluxSauce’s picture

I am coming more and more to the conclusion that this cannot (yet) be done by novices. The tools are not working for them, and they don't know enough to ignore the false positives that are not part of the coding standards.

I'm sorry you feel that way.

Months and months ago, before this issue was postponed, I claimed multiple issues and submitted patches for each, based on the coding standards at the time. There were areas within the coding standard that were not aligned with some of the newer D8 structures that were debated, contradicted, and never really resolved. Hours of work waited in the queue, corrected multiple times (including by people making a minor change and resubmitting the patch), only for the changes never to be accepted due to what looks like heel dragging from an outside perspective. Then came the accusations that the patch wasn't up to date, which was accurate as the patch was up to date when when it was submitted.

This was my first, real focused attempt at contributing to core, and I am extremely disenfranchised by this experience. I'm not a hack, I'm experienced and willing.

Currently, this issue is still marked as postponed and I have removed my name from four modules that I "claimed" above. I will look to contribute in other, appreciated ways.

jhodgdon’s picture

RE #47 - That is precisely what I meant by my comment -- novices were steered towards these issues in error, when there was really no way for them to succeed because the tools and standards were not working to make resolving these issues even possible for more experienced Drupalists. I in no way meant that the attempts by the novice contributors were not appreciated -- sorry if it came across that way -- I meant that the whole process was flawed and this issue is inappropriate for novice contributors.

It's definitely too bad that so many novice contributors had bad experiences with these issues... hopefully you will continue to contribute in other ways!

FluxSauce’s picture

Something that I don't understand - "throwing the baby out with the bathwater" and the all-or-nothing approach. If a commenting standard isn't sorted, skip the piece in question and accept the remainder. That didn't happen. Instead, "novice contributors" are (paraphrased) over their heads.

I'm bothered by this a lot, on many different levels. I hope to someday prove that I am capable of indenting code and correcting comments.

jhodgdon’s picture

Sorry again... I didn't mean to insult novice contributors! It's just that the organizers of this effort have yet to spell out which outputs from the tools are to be followed and which are to be ignored, so it's not a typical novice issue that would be great for someone who is really and truly brand new to Drupal programming and just needs to learn the ropes of how to use git, make a patch, etc. (they really need issues where the fix is straightforward so they can concentrate on learning the tools and procedures).

I don't think you're really a novice at this point. :)

sphism’s picture

Hey FluxSauce, same here... This issue was my first really focused effort at core contrib and it's definitely put me off a bit... However, my faith in contributing came back in bounds after going to the drupalcon munich code sprint and meeting XJM and YesCT.

What really amazes me is how people find the time to work on this stuff, I try to find time but I guess wednesday afternoon just doesn't work great for me. But everyone working together at the same time makes things way more fun.

Don't feel disheartened by this, let's both try to find some new contrib work that we can do and maybe work on it together, or help each other out or something - seems like we're in the same boat, maybe jhodgdon can point us to something we could help with

As for the 'not suitable for novices' thing - I kinda see the novice tag as meaning someone who's not familiar with the contrib process, not necessarily someone who can't understand code. I write big suites of drupal modules, but haven't managed to contribute much because it's remarkably difficult to get into.

Personally I would like to see all the patches written for this initiative committed - since there's lots of great work in there. Then after code freeze we open them all up again and do another round.

jhodgdon’s picture

Committing patches from this initiative: I would be happy to commit patches if they stick to only things that are actually in our written coding standards (leaving out the "false positive" things that Drupal CS is flagging), and things that are within the scope of what's described in the issue summary here. So far I am not seeing patches that fit those criteria.

Finding other issues to work on for new contributors:
- Check out http://drupal.org/new-contributors
- Search for issues tagged "Novice" in the core issue queue (other than these "make xyz pass coder review" issues)... there are usually some available!
- Issues in the "documentation" component (even if not tagged "novice") are always appreciated by me. The ones not tagged "novice" may require some extra research or decision-making; the ones tagged "novice" are usually really straightforward (like fixing a typo).

jhodgdon’s picture

Issue summary: View changes

Removed FluxSauce from assignment.

infiniteluke’s picture

Issue summary: View changes

Removed name from entity module as this is not a novice issue any more.

Lars Toomre’s picture

I have opened up #1800046: [META] Add missing type hinting to core docblocks, fix Drupal.Commenting.FunctionComment.Missing* to move existing core code to compliance with the need for type hiting in @param and @return directives of docblocks. This Meta issue is addressing everything else from such a Coder Review, except for such type hinting.

Please remember that "type hinting only" patches are time consumming to review and commit, and hence tend to have a lower priority when there are competing issues that need attention. However, "type hinting only" patches are welcome as sub-issues under that meta issue.

Lars Toomre’s picture

Issue summary: View changes

Updated issue summary.

Lars Toomre’s picture

Lars Toomre’s picture

I have opened two issues and posted patches to both that are focused on all test files in core. They are a combination of documentation and coding style clean-up that when committed will reduce the number of things highlighted in each of these sub-issues.

The two issues are:
- #1803656: Improve Tests consistency to standards in modules A-M
- #1804522: Improve Tests consistency to standards in modules N-Z

Reviews and comments there are welcome as it will make this overall coding standards initiative easier to complete.

Lars Toomre’s picture

Issue tags: -Novice

Various sub-issues of #1326672: Clean up API docs for menu module are moving forward and getting to the point where we can start working on sub-issues of this initiative.

I am unable to generate a Coder report at present and hence, I cannot figure out what still needs to be done to make key modules like Node, Menu, Simpletest, System, and User pass a Coder review. Could I ask that someone in the community add a sniff result to each of the appropriate issues? (Of course, if one wanted to do so for any other modules, that too would be welcome!) Thanks in advance!!

Also, as pointed out in #54, reviews of those two issues will help reduce the number of items flagged in each of these sub-issues.

Lars Toomre’s picture

Issue summary: View changes

Updated issue summary.

Lars Toomre’s picture

Issue summary: View changes

Updated issue summary.

Lars Toomre’s picture

Issue summary: View changes

Added sub-issues for Action and Ban modules.

TravisCarden’s picture

Issue summary: View changes

Added issue for breakpoint module.

Lars Toomre’s picture

Slowly, there is some progress being made on this initiative in the Action, Ban and Breakpoint sub-issues. Also, a patch fixing the Help module was committed last week, which I think is the first in this initative. Yeah!!

As the various reviews are being performed, a number of items are coming up. Some are specific to that module. Others are about something that should not be part of a Coder Review report (based on Code Sniffer module), or alternatively, should be flagged.

As a result, I have updated the issue summary of the meta issue so that we can inform and track follow-up issues that may be of interest to people working on the other sub-issues of this initiative. Please feel free to update with other follow-up issues as they come up.

Lars Toomre’s picture

Issue summary: View changes

Updated issue summary.

Lars Toomre’s picture

Issue summary: View changes

Updated issue summary.

Lars Toomre’s picture

Lars Toomre’s picture

Issue summary: View changes

Updated issue summary.

Lars Toomre’s picture

Issue summary: View changes

Updated issue summary.

Lars Toomre’s picture

Issue summary: View changes

Updated issue summary.

Lars Toomre’s picture

Issue summary: View changes

Updated issue summary.

Lars Toomre’s picture

Issue summary: View changes

Added follow up issue #1820868.

Lars Toomre’s picture

Issue summary: View changes

Added follow-up issue #1821592.

Lars Toomre’s picture

Issue summary: View changes

Updated issue summary.

Lars Toomre’s picture

Issue summary: View changes

Added follow-up issue 1821634.

TravisCarden’s picture

Issue summary: View changes

Recast "Follow-up issues" as "Blockers".

TravisCarden’s picture

Issue summary: View changes

Removed non-blocking blockers. Let's not ADD work at this point! Our goal is to get core passing Coder. Pursuant to this we'll remove _false positives_ from Coder, but adding _new tests_ should not be part of this issue.

cosmicdreams’s picture

This sounds like a perfect issue to tackle after Feature Freeze. I intend to help in that timeframe.

cosmicdreams’s picture

Issue summary: View changes

Oops: revision spam! (Sorry.) Fixed bad markup.

TravisCarden’s picture

Agreed, @cosmicdreams. Let me make it official: I'm postponing this issue and all its sub-issues until feature freeze so as not to distract from or interfere with major feature development. In the meantime, we can turn our attention here to the blockers. Thanks, everyone!

YesCT’s picture

OK... so we might be able to ease into this. By taking a new lock at the blocking issues.

I dont think we need to postpone this on #1539712: [policy, no patch] Coding standards for breaking function calls and language constructs across lines. I suggest we exclude the check for those, and that the coding standard write a "may" wrap recommendation for them.

YesCT’s picture

Issue summary: View changes

Updated issue assignments.

YesCT’s picture

Project changes for
http://drupal.org/project/coder
and
http://drupal.org/project/drupalcs

mean that the issue summary needs updated.

YesCT’s picture

Issue summary: View changes

Updated issue summary to remove the core blocker.

alexpott’s picture

Issue summary: View changes

Removed Poll module

jhodgdon’s picture

Issue summary: View changes

Copy of the revision from January 8, 2013 - 21:43.

sphism’s picture

bump...

Long time no see, are we gonna crack on with all these changes or give up and do something else?

jhodgdon’s picture

If people want to get going on this now, it is past feature freeze and the issue could be un-postponed.

TravisCarden’s picture

Issue summary: View changes

Copy of the revision from February 20, 2013 - 03:19.

TravisCarden’s picture

Status: Postponed » Active

Well let's crack on, then, shall we? ;) Off the top of my head, it seems like what we need to do to move forward is to make sure the existing issues still cover the entire codebase and that the closed/fixed issues are still fixed (i.e., there haven't been standards violations committed since the original work was done). It might also be necessary to reevaluate blockers and update participation instructions. Am I missing anything?

sphism’s picture

Awesome news.

I feel pretty overwhelmed where to start, I guess we can make all the issues active again, i'll take another look at the includes files D-G that I was working on and see if anything I did before still applies.

I think the hardest thing will be drumming up the momentum again

sphism’s picture

Issue summary: View changes

Reformatted table.

sphism’s picture

Issue summary: View changes

All issues can be made active again

andypost’s picture

Assigned: Unassigned » andypost
Issue tags: +CodeSprintCIS

Awesome task set! Takin it for 10 august

sphism’s picture

@Andypost: that's great news, what time (and timezone) is the the code sprint?

TravisCarden’s picture

Issue summary: View changes

set up form.inc as it's own issue

TravisCarden’s picture

Edit: Please disregard in favor of #69.

sphism’s picture

Thanks for that travis. Will try it out on Monday.

TravisCarden’s picture

You know what? Until we get resolution on #1539712: [policy, no patch] Coding standards for breaking function calls and language constructs across lines, let's exclude that array sniff, too. Again...

Here's a new patch for Coder (Sniffer) module to temporarilly disable the sniffs that have been excluded from this initiative. (To reiterate from before: this is not intended to be committed to the project.) Run the following command from your Coder directory to apply it:

curl https://drupal.org/files/coder-issue-1518116-reduced-do-not-test_0.patch | git apply

(If you don't have curl or you didn't check out Coder from Git, you can still download and apply the patch in the usual way.)

TravisCarden’s picture

Issue summary: View changes

Updated issue summary.

TravisCarden’s picture

Status: Postponed » Active
Issue tags: -Novice +CodeSprintCIS

Added Pro Tips to the issue summary.

Also, how did the meta issue get assigned to someone? Unassigning.

TravisCarden’s picture

Issue summary: View changes

Fixed broken markup. (Something went wrong with the input format's handling of our "handy issue-creating link".

TravisCarden’s picture

Issue summary: View changes

Removed erroneous "pro tip" about {@inheritdoc}.

TravisCarden’s picture

Issue summary: View changes

Removed PHP module.

TravisCarden’s picture

Issue summary: View changes

Removed OpenID module, which has been removed from core: https://drupal.org/node/2116417.

TravisCarden’s picture

Issue summary: View changes

Updated issue summary.

TravisCarden’s picture

Let's see if we can make it a little easier to participate in this. I've forked the Coder module and disabled the sniffs we're omitting here so people don't need to mess with patching it themselves. There's no "code review" tab on qa.drupal.org anymore, so I removed that requirement, as well as the one to install Coder Review, since the patch no longer applies, and probably doesn't work anymore anyway. @jhodgdon, if any of those changes will make you less accepting of patches, please let me know. Otherwise, I figure that trimmer scope will make it easier for people to get started on this and make progress.

jhodgdon’s picture

@TravisCarden/#71 - If patches come in that make Core more compliant with our coding standards, it's good. If patches come in with changes that have nothing to do with our coding standards, it's bad. If you've forked Coder in a way that makes it easier to make good changes and omit bad changes, that has to be good. :)

One thing I noticed that is in the issue summary is a suggestion to make member items in classes "public" if they are not designated otherwise. That conflicts with https://drupal.org/node/608152#visibility -- which suggests a better way would be to make them "protected" and see if the tests pass when the patch is uploaded. If not, go back and fix the ones that are not working correctly. Thoughts?

TravisCarden’s picture

Issue summary: View changes

Removing Overlay module per #2088121: Remove Overlay.

TravisCarden’s picture

Issue summary: View changes
TravisCarden’s picture

Update: I've updated the Coder spork for upstream changes and a patch for a false positive discovered in #1533104: Make bartik theme pass Coder Review. (Review the patch at #2203627: "Implements hook_foo() for some-template.file" sniff doesn't support Twig template names.) Please update your clones: git checkout issue-1518116 && git fetch origin && git reset --hard origin/issue-1518116. Thanks!

TravisCarden’s picture

Notice: Our Coder fork has been patched per the following:

Please update (git reset --hard) your clones per step #6 in the summary up top. And if you have feedback on the issues, please pop into them to comment or review the patches. Thanks!

YesCT’s picture

TravisCarden’s picture

@YesCT: Sure. Everything should be submitted to the official coder queue on d.o. If you want something added to my spork in the meantime, you can submit a pull request there. Thanks for asking!

YesCT’s picture

ok. :)

TravisCarden’s picture

Note: The Coder spork has been updated with a fix for a false positive. Thanks, @YesCT for reporting and @klausi for fixing!

BiigNiick’s picture

i was looking at this at the DrupalConAustin sprint as a Novice with help from @scor, but i ran out of time before i had to go... maybe this isn't a Novice issue? not sure...

i had a great first DrupalCon! thanks everyone :-)

YesCT’s picture

How do I get the new commits from there?
I ran
composer global update
but it did not update phpcs

Looks like I was running the 7.x version.
I want 8.x for Drupal 8.. but the install instructions
http://cgit.drupalcode.org/coder/tree/coder_sniffer/README.txt?h=8.x-2.x
which say

Installation: Composer
----------------------

You can also use Coder Sniffer as a library with Composer:

    "require": {
        "drupal/coder": "*"
    }

dont seem to get me 8.x

[edit]
found #2278115: How to install and use Sniffer 8.x?

GaëlG’s picture

The Travis coder fork didn't work for me with last composer code sniffer version. I had to use 1.4.2 to get rid of this:
Class PHP_CodeSniffer_CommentParser_ClassCommentParser not found

GaëlG’s picture

I got many lowerCamel errors like this one:

FILE: ...pps/drupal/htdocs/core/modules/field/src/Entity/FieldInstanceConfig.php
--------------------------------------------------------------------------------
FOUND 6 ERROR(S) AFFECTING 6 LINE(S)
--------------------------------------------------------------------------------
  55 | ERROR | Class property $field_name should use lowerCamel naming without
     |       | underscores
  62 | ERROR | Class property $entity_type should use lowerCamel naming without
     |       | underscores
 153 | ERROR | Class property $default_value should use lowerCamel naming
     |       | without underscores
 171 | ERROR | Class property $default_value_function should use lowerCamel
     |       | naming without underscores
 200 | ERROR | Class property $bundle_rename_allowed should use lowerCamel
     |       | naming without underscores
 653 | ERROR | Missing function doc comment

I asked on IRC what to do (don't want to waste time on useless tasks breaking the whole core) :

gaelg: should I change all instances of $field_name to $fieldName in the core to respect coding standards? This looks a bit violent to me but it's what CodeSniffer tells me (I'm working on #1533232)
vijaycs85: may be not. we have those type of problem all over the core

Maybe there should be a notice about this in the issue summary? Or make the Coder fork ignore this?

geerlingguy’s picture

FYI, I've been tracking D8 with most of D7's coding standards (via Jenkins/Coder Sniffer) over the past few months (and plan on continuing to do so for some time), in case anyone finds it useful:

http://drupalci.midwesternmac.com/dashboard/index/779

I especially like seeing graphs showing improvement over time :)

xjm’s picture

Version: 8.0.x-dev » 8.1.x-dev
Issue summary: View changes
Status: Active » Postponed

Interesting work @geerlingguy!

Here is a reassessment of this meta issue for the beta phase:

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because these are code style cleanups.
Issue priority Normal because no functional code is impacted, but following our own code style standards consistently will reduce tedium and confusion for contributors. Individual child issues should be minor.
Prioritized changes This is not a prioritized change for the beta phase.
Disruption These changes are moderately disruptive for Drupal core because they alter unrelated lines across many files, and they require careful review to ensure functional code cleanups are not introducing additional bugs or technical debt. Code style cleanups will also cause core minor version branches to diverge if they are not backported.

For now, I've postponed this issue to 8.1.x. We did discuss allowing pure code style cleanups closer to 8.0.x's RC, and if we decide to do that, we can move this back. However, because we repeatedly introduce code style regressions, we agreed in discussions at DrupalCons Austin and Amsterdam that the best course of action would be:

  1. Wait until the new Drupal.org testbot infrastructure can support automated testing for just one coding standards rule.
  2. File a specific issue to ensure one rule is followed throughout core. I recommend we start with trailing whitespace since core currently complies with this and it is easy to patch and review.
  3. Enable testbot support for that one rule.
  4. Repeat for additional coding standards rules.

I think we probably want to create a new meta issue focusing on fixing core on a per-rule basis instead of a per-component basis, and close this as a duplicate. However, leaving this open for now so that contributors can see these updates.

xjm’s picture

Issue summary: View changes
Wim Leers’s picture

#85: wow, very nice, thanks for doing that!
#86: sounds awesome!

Mile23’s picture

Just wanted to point out a tool I made to do semi-automatic coding standards review in NetBeans: https://www.drupal.org/sandbox/mile23/2197899

TravisCarden’s picture

Issue summary: View changes
andypost’s picture

Category: Task » Plan
tatarbj’s picture

For now, I've postponed this issue to 8.1.x. We did discuss allowing pure code style cleanups closer to 8.0.x's RC, and if we decide to do that, we can move this back.

Can we reopen the discussion, because we are very close to RC.
I'd like to work on it, i have enough time to clean up the core modules like i started to do it with this issue: https://www.drupal.org/node/1816682 but this is because it's based on 8.1.x the tests were failed, so we should put this issue and all of the related ones back to 8.0.x, am i right? This branch is not maintained very well (last commit is older than 1 year!) so most probably this is one cause why my patch was failed.
I'll go to Barcelona and work very hard on it, till that time i have like 1-2 hours per day to clean those modules up, there will be more.

pfrenssen’s picture

Status: Postponed » Active

Reopening for discussion as per #92.

I think it would be appropriate to allow coding standards patches from the moment 8.0.x-RC1 is actually released. That would give a very concrete starting point, and would allow people to start working on it today, knowing that the work can be accepted in a number of weeks.

The 8.1.x branch is not up-to-date, but I guess this will not be formally opened until we have a real 8.0.0 release, so we could instead move those issues back to 8.0.x.

@xjm proposed in #86 to start off this initiative with an automatic coding standards check on the test bot. It was proposed to only allow a single sniff (trailing whitespace) and gradually increase the automatic coverage. This will cut up the work in somewhat manageable chunks, but there will still be some sniffs that will need gigantic patches to fix across many different modules. Also, working on a sniff-by-sniff basis would always require people to work across the entire code base, this would be much less efficient than working on a single module (or even a part of a module). Also reviewing these patches would be harder, since the number of touched files might be quite large.

The codesniffer ruleset that is included in the Coder module is very good, I think we should start from this, rather than build up a new ruleset sniff by sniff.

Of course we need to reduce the impact as much as possible. We can do this by maintaining a blacklist of modules / classes / components that are not compliant. Initially we can add all modules and core components to the blacklist. Every time a module is fixed we can remove it from the blacklist, and from that point in time it will be fully covered by the automated test on the testbot, so new patches will have to be fully compliant.

We can include PHP CodeSniffer in core itself and supply a default phpcs.xml file, so that developers can trivially test their code for coding standards by executing:

$ cd core/
$ ./vendor/bin/phpcs

Here is an example phpcs.xml file with a blacklist of non-compliant modules:

<?xml version="1.0"?>
<!-- PHP_CodeSniffer standard for Drupal core. -->
<!-- See http://pear.php.net/manual/en/package.php.php-codesniffer.annotated-ruleset.php -->
<ruleset name="Drupal core">
    <description>PHP CodeSniffer coding standard for Drupal core</description>

    <rule ref="./vendor/drupal/coder/coder_sniffer/Drupal" />

    <arg name="extensions" value="php,inc,module,install,info,test,profile,theme,css,js"/>
    <arg name="report" value="full"/>

    <file>./*.php</file>
    <file>./includes</file>
    <file>./lib</file>
    <file>./modules</file>
    <file>./profiles</file>
    <file>./tests</file>
    <file>./themes</file>

    <!-- Minified files don't have to comply with coding standards. -->
    <exclude-pattern>*.min.css</exclude-pattern>
    <exclude-pattern>*.min.js</exclude-pattern>

    <!--
        Blacklist of modules that are not yet compliant with coding standards.
        Whenever a module is fixed it should be removed from this list so it
        can be tested automatically by the testbot.
     -->
    <exclude-pattern>core/modules/action</exclude-pattern>
    <exclude-pattern>core/modules/aggregator</exclude-pattern>
    <exclude-pattern>core/modules/ban</exclude-pattern>
    <exclude-pattern>core/modules/basic_auth</exclude-pattern>
    <exclude-pattern>...</exclude-pattern>
    <exclude-pattern>...</exclude-pattern>

</ruleset>
pfrenssen’s picture

Shall we plan a BoF at Drupalcon next week to discuss this?

pfrenssen’s picture

Status: Active » Closed (duplicate)

Closing this in favor of #2571965: [meta] Fix PHP coding standards in core which will deal with fixing the coding standards in a sniff-per-sniff basis instead of the module-per-module basis which was used here. Having a new issue will reduce confusion because of the large number of child issues that are associated with this issue and are now obsolete.