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
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:
- 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!
- Next, choose an unclaimed issue from the Remaining Tasks section below.
- 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)
- Edit this issue summary here, and add your issue to the Remaining Tasks section below.
- 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.
- 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. - Run Coder Sniffer against the relevant directory or files, e.g.,
drush dcs includes/
. - 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.
- 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".
- Note in your issue summary any failures that were not corrected, and why.
- Start over at the first step if desired!
- 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.)
* Except files subdirectory. We don't want to alter the files in the simpletest/files directory, since they are used in tests
Related issues
Comment | File | Size | Author |
---|
Comments
Comment #0.0
TravisCarden CreditAttribution: TravisCarden commentedUpdated references to this issue's node ID.
Comment #1
webchickAwesome initiative! :D
Comment #2
xjmBefore you do anything, stop! Heads up!
See also:
Help with the above would be greatly appreciated. :)
Comment #3
xjmMarked #1266446: Core-wide code style cleanup as duplicate.
Comment #4
xjmI reopened #1358940: Remove Coder_Tough_Love dependency from PIFR_Coder. I recommend postponing this initiative until that is fixed.
Comment #5
TravisCarden CreditAttribution: TravisCarden commentedMarked as duplicates:
Comment #6
TravisCarden CreditAttribution: TravisCarden commentedPostponing per comments #2 and #4.
Comment #6.0
TravisCarden CreditAttribution: TravisCarden commentedFixed a little formatting in the "Remaining tasks" table.
Comment #7
TravisCarden CreditAttribution: TravisCarden commentedAdding tag.
Comment #7.0
TravisCarden CreditAttribution: TravisCarden commentedFixed a couple of mistakes in issue template code.
Comment #7.1
TravisCarden CreditAttribution: TravisCarden commentedAdded "coding standards" tag to new issue template and quick create link.
Comment #7.2
xjm(xjm) Fix link.
Comment #8
cosmicdreams CreditAttribution: cosmicdreams commentedWhich version of coder should we use to check work as we go? The 7.x version? http://drupal.org/node/105916/release
Comment #9
TravisCarden CreditAttribution: TravisCarden commented@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.
Comment #10
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedXJM,
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.
Comment #10.0
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedClaimed block module.
Comment #10.1
TravisCarden CreditAttribution: TravisCarden commentedClaimed book module.
Comment #10.2
TravisCarden CreditAttribution: TravisCarden commentedClaimed Color module.
Comment #10.3
TravisCarden CreditAttribution: TravisCarden commentedClaimed Comment module.
Comment #10.4
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedUpdated issue summary.
Comment #10.5
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedUpdated issue summary.
Comment #10.6
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedAdded additional issues
Comment #10.7
TravisCarden CreditAttribution: TravisCarden commentedAdded Config module.
Comment #10.8
TravisCarden CreditAttribution: TravisCarden commentedClaimed Contact module.
Comment #10.9
TravisCarden CreditAttribution: TravisCarden commentedRemoved listings for non-existent includes sub-directories. Removed note about config module already passing.
Comment #10.10
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedAdded additional issues
Comment #10.11
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedUpdated issue summary.
Comment #10.12
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedUpdated issue summary.
Comment #10.13
sphism CreditAttribution: sphism commentedadded link to php module
Comment #10.14
sphism CreditAttribution: sphism commentedpoll module
Comment #10.15
sphism CreditAttribution: sphism commentedrdf module
Comment #10.16
sphism CreditAttribution: sphism commentedsearch module
Comment #10.17
sphism CreditAttribution: sphism commentedshortcut
Comment #10.18
sphism CreditAttribution: sphism commentedstatistics module
Comment #10.19
sphism CreditAttribution: sphism commentedsyslog module
Comment #10.20
sphism CreditAttribution: sphism commentedsystem module
Comment #10.21
sphism CreditAttribution: sphism commentedtaxonomy module
Comment #10.22
sphism CreditAttribution: sphism commentedtoolbar
Comment #10.23
sphism CreditAttribution: sphism commentedtracker module
Comment #10.24
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedFinished adding issues
Comment #11
xjmThe latter two links were for reference and advertising. Thence "see also." :)
Comment #11.0
xjmRemoved non-existant Trigger module.
Comment #12
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedAs 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.
Comment #12.0
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedUpdated issue summary.
Comment #12.1
sphism CreditAttribution: sphism commentedassigning sphism to Includes D-G
Comment #12.2
eiriksmAssigned tracker issue to myself
Comment #13
xjmThis is a good point. In fact, it might be wise to postpone the issue here on its API docs counterpart.
Comment #13.0
xjmAssigning taxonomy to myself. (xjm)
Comment #14
lotyrin CreditAttribution: lotyrin commentedI'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.
Comment #16
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedI 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.
Comment #17
jhodgdonI'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?
Comment #18
TravisCarden CreditAttribution: TravisCarden commentedI 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.
Comment #19
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedI 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.
Comment #20
TravisCarden CreditAttribution: TravisCarden commenteddrupalcs 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.
Comment #21
xjmHere is the workflow I'd suggest:
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!
Comment #21.0
xjmUpdated participation instructions to recommend use Drupal Code Sniffer until Coder module can be used on d8.
Comment #22
xjmI added a step to the summary asking that participants document the results of each tool in their issues.
Comment #22.0
xjm(xjm) Added section on documenting what tools were used and what failures were corrected.
Comment #23
jhodgdon+1 on #21.
Comment #23.0
jhodgdonUpdated issue summary.
Comment #23.1
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedUpdated assigned modules
Comment #23.2
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedUpdated summary
Comment #23.3
TravisCarden CreditAttribution: TravisCarden commentedReformatted the remaining tasks table a little bit. Broke themes directory into separate issues per theme. Assigned user module to lotyrin.
Comment #24
lotyrin CreditAttribution: lotyrin commented@xjm I've started the sandbox: http://drupal.org/node/1539824
Comment #24.0
lotyrin CreditAttribution: lotyrin commentedUpdated issue summary.
Comment #24.1
FluxSauce CreditAttribution: FluxSauce commentedClaiming toolbar.
Comment #24.2
FluxSauce CreditAttribution: FluxSauce commentedClaiming syslog
Comment #24.3
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedUpdated issue summary.
Comment #25
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedUpdating for #1299424: Allow one module per directory and move system tests to core/modules/system
Comment #25.0
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedUpdating for #1299424: Allow one module per directory and move system tests to core/modules/system
Comment #25.1
FluxSauce CreditAttribution: FluxSauce commentedAccepting Translation
Comment #26
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedAs requested in #1512434-30: Make Aggregator module pass Coder Review we will not be adding param/return types in this sprint.
Comment #26.0
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedUpdated issue summary.
Comment #27
infiniteluke CreditAttribution: infiniteluke commentedFYI
I added myself into the table above for the entity module.
Comment #27.0
infiniteluke CreditAttribution: infiniteluke commentedAdding myself as working on entity module
Comment #27.1
scottalan CreditAttribution: scottalan commentedUpdated issue summary.
Comment #28
scottalan CreditAttribution: scottalan commentedI 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.
Comment #29
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedAll 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.
Comment #29.0
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedUpdated issue summary.
Comment #29.1
sphism CreditAttribution: sphism commentedadded sphism to profiles directoy
Comment #29.2
TravisCarden CreditAttribution: TravisCarden commentedUnassigned rdf module.
Comment #29.3
TravisCarden CreditAttribution: TravisCarden commentedUnassigned user module.
Comment #29.4
TravisCarden CreditAttribution: TravisCarden commentedUnassigned includes directory, files starting with A-C.
Comment #30
sphism CreditAttribution: sphism commentedYeah 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.
Comment #31
sphism CreditAttribution: sphism commentedshould this:
in file.inc
be this
the error is : 1516 | ERROR | Line indented incorrectly; expected at least 6 spaces, found 4 (line 1516 is // Unknown error.)
Comment #32
sphism CreditAttribution: sphism commentedin 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 ???
Comment #33
sphism CreditAttribution: sphism commentedjust 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:
Comment #34
xjm@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.
Comment #35
TravisCarden CreditAttribution: TravisCarden commentedPostponing 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.
Comment #35.0
TravisCarden CreditAttribution: TravisCarden commentedunassigned sphism from profiles directory (not sure what else I can do on it)
Comment #35.1
TravisCarden CreditAttribution: TravisCarden commentedAdded link to instructions for silencing excluded errors.
Comment #36
TravisCarden CreditAttribution: TravisCarden commentedEdit: Please disregard in favor of #69.
Comment #36.0
TravisCarden CreditAttribution: TravisCarden commentedUpdated issue summary.
Comment #36.1
TravisCarden CreditAttribution: TravisCarden commentedAdded "Pro Tips" for dealing with classes.
Comment #37
xjm@TravisCarden, is there an issue in the dcs queue for that? If not we should file one, even if it's a wontfix.
Comment #38
TravisCarden CreditAttribution: TravisCarden commented@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?
Comment #39
jhodgdonIt looks like the patch just disables checking for param and return types, which are excluded from this initiative.
Comment #40
TravisCarden CreditAttribution: TravisCarden commentedThat is correct, @jhodgdon.
Comment #40.0
TravisCarden CreditAttribution: TravisCarden commentedAdded another reference to the new drupalcs patch.
Comment #40.1
TravisCarden CreditAttribution: TravisCarden commentedAssigned dashboard module to dinathecool.
Comment #40.2
TravisCarden CreditAttribution: TravisCarden commentedAdded new core/*.* issue.
Removed reference to non-existent database and filetransfer subdirectories.
Comment #40.3
TravisCarden CreditAttribution: TravisCarden commentedClaimed overlay module.
Comment #40.4
TravisCarden CreditAttribution: TravisCarden commentedUpdated issue assignments.
Comment #40.5
TravisCarden CreditAttribution: TravisCarden commentedUn-assigned dashboard module.
Comment #40.6
TravisCarden CreditAttribution: TravisCarden commentedAdded xmlrpc module.
Comment #40.7
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedAssigning update.module to myself.
Comment #40.8
fotuzlab CreditAttribution: fotuzlab commentedAssigning xml-rpc module to self
Comment #40.9
TravisCarden CreditAttribution: TravisCarden commentedRemoved dashboard module (removed from core: http://drupal.org/node/1697190).
Comment #40.10
TravisCarden CreditAttribution: TravisCarden commentedAdded "Related issues" section.
Comment #41
jhodgdonI 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?
Comment #42
douggreen CreditAttribution: douggreen commentedI 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.
Comment #43
jhodgdonRE #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.
Comment #44
TravisCarden CreditAttribution: TravisCarden commentedI suppose our options for proceeding with this issue are (roughly*) these:
* I omit, of course, the option of paying @jhodgdon what she's worth to just do the job for us. ;-)
Comment #45
jhodgdonI 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.
Comment #45.0
jhodgdontook ownership of contextual module issue
Comment #46
theduke CreditAttribution: theduke commentedI will handle dblog and statistics modules.
Comment #46.0
theduke CreditAttribution: theduke commentedtheduke is taking care of statistics module
Comment #46.1
theduke CreditAttribution: theduke commentedAssigning dblog module
Comment #47
FluxSauce CreditAttribution: FluxSauce commentedI'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.
Comment #48
jhodgdonRE #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!
Comment #49
FluxSauce CreditAttribution: FluxSauce commentedSomething 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.
Comment #50
jhodgdonSorry 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. :)
Comment #51
sphism CreditAttribution: sphism commentedHey 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.
Comment #52
jhodgdonCommitting 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).
Comment #52.0
jhodgdonRemoved FluxSauce from assignment.
Comment #52.1
infiniteluke CreditAttribution: infiniteluke commentedRemoved name from entity module as this is not a novice issue any more.
Comment #53
Lars Toomre CreditAttribution: Lars Toomre commentedI 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.
Comment #53.0
Lars Toomre CreditAttribution: Lars Toomre commentedUpdated issue summary.
Comment #53.1
Lars Toomre CreditAttribution: Lars Toomre commentedAdd link to #1800046: [META] Add missing type hinting to core docblocks, fix Drupal.Commenting.FunctionComment.Missing* in related issues section.
Comment #54
Lars Toomre CreditAttribution: Lars Toomre commentedI 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.
Comment #55
Lars Toomre CreditAttribution: Lars Toomre commentedVarious 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.
Comment #55.0
Lars Toomre CreditAttribution: Lars Toomre commentedUpdated issue summary.
Comment #55.1
Lars Toomre CreditAttribution: Lars Toomre commentedUpdated issue summary.
Comment #55.2
Lars Toomre CreditAttribution: Lars Toomre commentedAdded sub-issues for Action and Ban modules.
Comment #55.3
TravisCarden CreditAttribution: TravisCarden commentedAdded issue for breakpoint module.
Comment #56
Lars Toomre CreditAttribution: Lars Toomre commentedSlowly, 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.
Comment #56.0
Lars Toomre CreditAttribution: Lars Toomre commentedUpdated issue summary.
Comment #56.1
Lars Toomre CreditAttribution: Lars Toomre commentedUpdated issue summary.
Comment #56.2
Lars Toomre CreditAttribution: Lars Toomre commentedAdded follow up issue #1820104: Ensure @param object type used in function definition (as appropriate).
Comment #56.3
Lars Toomre CreditAttribution: Lars Toomre commentedUpdated issue summary.
Comment #56.4
Lars Toomre CreditAttribution: Lars Toomre commentedUpdated issue summary.
Comment #56.5
Lars Toomre CreditAttribution: Lars Toomre commentedUpdated issue summary.
Comment #56.6
Lars Toomre CreditAttribution: Lars Toomre commentedUpdated issue summary.
Comment #56.7
Lars Toomre CreditAttribution: Lars Toomre commentedAdded follow up issue #1820868.
Comment #56.8
Lars Toomre CreditAttribution: Lars Toomre commentedAdded follow-up issue #1821592.
Comment #56.9
Lars Toomre CreditAttribution: Lars Toomre commentedUpdated issue summary.
Comment #56.10
Lars Toomre CreditAttribution: Lars Toomre commentedAdded follow-up issue 1821634.
Comment #56.11
TravisCarden CreditAttribution: TravisCarden commentedRecast "Follow-up issues" as "Blockers".
Comment #56.12
TravisCarden CreditAttribution: TravisCarden commentedRemoved 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.
Comment #57
cosmicdreams CreditAttribution: cosmicdreams commentedThis sounds like a perfect issue to tackle after Feature Freeze. I intend to help in that timeframe.
Comment #57.0
cosmicdreams CreditAttribution: cosmicdreams commentedOops: revision spam! (Sorry.) Fixed bad markup.
Comment #58
TravisCarden CreditAttribution: TravisCarden commentedAgreed, @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!
Comment #59
YesCT CreditAttribution: YesCT commentedOK... 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.
Comment #59.0
YesCT CreditAttribution: YesCT commentedUpdated issue assignments.
Comment #60
YesCT CreditAttribution: YesCT commentedProject changes for
http://drupal.org/project/coder
and
http://drupal.org/project/drupalcs
mean that the issue summary needs updated.
Comment #60.0
YesCT CreditAttribution: YesCT commentedUpdated issue summary to remove the core blocker.
Comment #60.1
alexpottRemoved Poll module
Comment #60.2
jhodgdonCopy of the revision from January 8, 2013 - 21:43.
Comment #61
sphism CreditAttribution: sphism commentedbump...
Long time no see, are we gonna crack on with all these changes or give up and do something else?
Comment #62
jhodgdonIf people want to get going on this now, it is past feature freeze and the issue could be un-postponed.
Comment #62.0
TravisCarden CreditAttribution: TravisCarden commentedCopy of the revision from February 20, 2013 - 03:19.
Comment #63
TravisCarden CreditAttribution: TravisCarden commentedWell 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?
Comment #64
sphism CreditAttribution: sphism commentedAwesome 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
Comment #64.0
sphism CreditAttribution: sphism commentedReformatted table.
Comment #64.1
sphism CreditAttribution: sphism commentedAll issues can be made active again
Comment #65
andypostAwesome task set! Takin it for 10 august
Comment #66
sphism CreditAttribution: sphism commented@Andypost: that's great news, what time (and timezone) is the the code sprint?
Comment #66.0
TravisCarden CreditAttribution: TravisCarden commentedset up form.inc as it's own issue
Comment #67
TravisCarden CreditAttribution: TravisCarden commentedEdit: Please disregard in favor of #69.
Comment #68
sphism CreditAttribution: sphism commentedThanks for that travis. Will try it out on Monday.
Comment #69
TravisCarden CreditAttribution: TravisCarden commentedYou 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.)
Comment #69.0
TravisCarden CreditAttribution: TravisCarden commentedUpdated issue summary.
Comment #70
TravisCarden CreditAttribution: TravisCarden commentedAdded Pro Tips to the issue summary.
Also, how did the meta issue get assigned to someone? Unassigning.
Comment #70.0
TravisCarden CreditAttribution: TravisCarden commentedFixed broken markup. (Something went wrong with the input format's handling of our "handy issue-creating link".
Comment #70.1
TravisCarden CreditAttribution: TravisCarden commentedRemoved erroneous "pro tip" about {@inheritdoc}.
Comment #70.2
TravisCarden CreditAttribution: TravisCarden commentedRemoved PHP module.
Comment #70.3
TravisCarden CreditAttribution: TravisCarden commentedRemoved OpenID module, which has been removed from core: https://drupal.org/node/2116417.
Comment #70.4
TravisCarden CreditAttribution: TravisCarden commentedUpdated issue summary.
Comment #71
TravisCarden CreditAttribution: TravisCarden commentedLet'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.
Comment #72
jhodgdon@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?
Comment #73
TravisCarden CreditAttribution: TravisCarden commentedRemoving Overlay module per #2088121: Remove Overlay.
Comment #74
TravisCarden CreditAttribution: TravisCarden commentedComment #75
TravisCarden CreditAttribution: TravisCarden commentedUpdate: 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!Comment #76
TravisCarden CreditAttribution: TravisCarden commentedNotice: 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!Comment #77
YesCT CreditAttribution: YesCT commented@TravisCarden where should I open issues like those found in #1533250: Many coding standards clean-ups in locale module
in https://github.com/TravisCarden/coder ?
and in https://drupal.org/project/issues/coder ?
Comment #78
TravisCarden CreditAttribution: TravisCarden commented@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!
Comment #79
YesCT CreditAttribution: YesCT commentedok. :)
Comment #80
TravisCarden CreditAttribution: TravisCarden commentedNote: The Coder spork has been updated with a fix for a false positive. Thanks, @YesCT for reporting and @klausi for fixing!
Comment #81
BiigNiick CreditAttribution: BiigNiick commentedi 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 :-)
Comment #82
YesCT CreditAttribution: YesCT commentedHow 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
dont seem to get me 8.x
[edit]
found #2278115: How to install and use Sniffer 8.x?
Comment #83
GaëlGThe 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
Comment #84
GaëlGI got many lowerCamel errors like this one:
I asked on IRC what to do (don't want to waste time on useless tasks breaking the whole core) :
Maybe there should be a notice about this in the issue summary? Or make the Coder fork ignore this?
Comment #85
geerlingguy CreditAttribution: geerlingguy commentedFYI, 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 :)
Comment #86
xjmInteresting work @geerlingguy!
Here is a reassessment of this meta issue for the beta phase:
Beta phase evaluation
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:
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.
Comment #87
xjmComment #88
Wim Leers#85: wow, very nice, thanks for doing that!
#86: sounds awesome!
Comment #89
Mile23Just wanted to point out a tool I made to do semi-automatic coding standards review in NetBeans: https://www.drupal.org/sandbox/mile23/2197899
Comment #90
TravisCarden CreditAttribution: TravisCarden commentedComment #91
andypostComment #92
tatarbjCan 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.
Comment #93
pfrenssenReopening 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:Here is an example
phpcs.xml
file with a blacklist of non-compliant modules:Comment #94
pfrenssenShall we plan a BoF at Drupalcon next week to discuss this?
Comment #95
pfrenssenClosing 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.