Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
The larger effort to get all of Drupal core into compliance with Drupal's coding standards has been broken into the following issues:
Comment | File | Size | Author |
---|---|---|---|
#79 | spartaaaa.png | 26.02 KB | oriol_e9g |
#74 | drupal-1109202-74.patch | 38.17 KB | TravisCarden |
#71 | drupal-1109202-71.patch | 39.09 KB | TravisCarden |
#59 | 1109202-59-coding-standards.patch | 158.2 KB | oriol_e9g |
#55 | 1109202-55-coding-standards.patch | 158.73 KB | oriol_e9g |
Comments
Comment #1
oriol_e9gMore fixes, more coding standard love.
Comment #2
scor CreditAttribution: scor commentedyou are introducing trailing whitespaces in your else statements changes.
Comment #3
oriol_e9gok, trying again.
Comment #4
bfroehle CreditAttribution: bfroehle commentedPatch in #3 looks good. How did you generate a list of these coding style bugs?
Comment #5
scor CreditAttribution: scor commentedlooks good now
Comment #6
oriol_e9g@bfroehle with coder module: http://drupal.org/project/coder
Comment #7
droplet CreditAttribution: droplet commentedit has 2 spaces between .= and t('After.....
Comment #8
oriol_e9gComment #9
oriol_e9gThe two spaces was a mistake before & after de patch. I have found other double spaces around the code and I fixed it with a new patch.
Comment #10
oriol_e9gOps! One more.
Comment #11
oriol_e9gMore fixes comming
Comment #12
oriol_e9gEvery loop it's a bigger patch :S
Comment #13
oriol_e9gComment #14
droplet CreditAttribution: droplet commentedcatch few more SPACE issue.
Comment #15
oriol_e9gTests passes and patch #14 cleanly apply
Comment #16
rfayD8 first. Let's get these fixed on D8 and into D7.
Comment #17
oriol_e9gOk. Trying again, D8 patch.
Comment #18
Lars Toomre CreditAttribution: Lars Toomre commentedThis should definitely go in soon assuming that it is still passing tests for D8.
Seeing this issue makes me wonder whether we should create a new issue to add 'tough love' coder SimpleTests to the standard core testing suite. Even if we were to start simple, we could automatically highlight the extra white-space errors, the missing blank line at the end of a file and other such common errors that seem to frequently pop up in the patch review and approval process.
Some of the other tests that could be easily include are tests for spacing around dot operators, space before opening brace of a function and double spaces in doc block comments, all of which seem to be the main items being caught here.
Comment #19
droplet CreditAttribution: droplet commentedrename for testbots.
It's same as #17, so RTBC by me :)
it's a bit crazy to review it, hoping commit to core immediately :)
Comment #21
droplet CreditAttribution: droplet commentedComment #22
droplet CreditAttribution: droplet commented#19, I uploaded a wrong file.
#21 & #17 exactly same patch with rename, so RTBC.
Comment #23
tstoecklerNote the last concatenation is missing a space.
There is stray whitespace after "$this->searchExpression".
I am leaving at RTBC though, because the 3 cases above aren't regressions, they are just additional coding standards problems unveiled by the patch
...and because this fixes about a bazillion coding standards problems in Drupal core
(and it passes tests and I double-checked that there were no code changes.)
EDIT: Fix formatting
Comment #24
droplet CreditAttribution: droplet commentedokay. again.
Comment #26
droplet CreditAttribution: droplet commentedComment #28
droplet CreditAttribution: droplet commentedstrange, the file is different than I upload... what's wrong..
Comment #29
oriol_e9g#28: newwwwww_one.patch queued for re-testing.
Comment #31
oriol_e9gAgain. Infinite perseverance.
Comment #32
droplet CreditAttribution: droplet commentedmissed this one :)
Comment #33
wojtha CreditAttribution: wojtha commentedWhen patching the coding the standards, it will be nice to name the patches according to Drupal standards or at least recommandations by inserting the issue number and comment number to the beginning of patch filename.
If I post the follow-up patch in this comment, I will use the name 1109202-33_coding_standards.patch or similar.
Nice catches BTW.
Comment #34
oriol_e9gOnly file name change.
Comment #35
wojtha CreditAttribution: wojtha commentedCool :-) the rename wasn't really necessary it is just very recommended to name the patches like this.
But you can't change the state to RTBC if you are the author or one of the authors of the patch... you need to wait until someone review it indepedently.
Comment #36
aspilicious CreditAttribution: aspilicious commentedYou can't edit system.tar.inc because its not rly drupalcode. Its an extension someone outside drupal wrote. See the information on the top of this file.
If that part gets removed this is rdy to go (man thats a big patch...)
Powered by Dreditor.
Comment #37
droplet CreditAttribution: droplet commentedOkay. regenerate again:
git apply --exclude=modules/system/system.tar.inc
git diff > 1109202-37_coding_standards.patch
Comment #38
jbrown CreditAttribution: jbrown commentedLooks good!
Comment #39
webchickMarking this postponed until the # of criticals for D7 & D8 gets < 15.
Comment #40
oriol_e9g#37: 1109202-37_coding_standards.patch queued for re-testing.
Comment #42
oriol_e9gDrupal 7.4 released
D7 & D8 criticals < 15
Try again
Comment #43
oriol_e9gNo reviews?
Comment #44
oriol_e9gNo reviews?
Comment #45
droplet CreditAttribution: droplet commentedI'm back :)
Comment #46
droplet CreditAttribution: droplet commented@oriol_e9g,
see #36 and #37
Comment #47
jbrown CreditAttribution: jbrown commented#42: 1109202-42_coding_standards.patch queued for re-testing.
Comment #49
klausiRejected files:
I could fix all hunks manually, only ajax.js does not contain the white space issues anymore (was probably fixed elsewhere).
Comment #50
solotandem CreditAttribution: solotandem commentedAre you generating these patches by making manual changes to core? Has anyone run core through the Grammar Parser recently? (It has been used previously for just this purpose.) It should clean up all the white space issues and other coding standards. It won't fix all of the comment stuff or change the JS files, though.
Comment #51
oriol_e9gI use a combination of coder module, manual changes and regular expressions. I can roll a new patch one more time but it seems that this will never be committed.
We can try again but I do not know if we can attract the core commiters to commit this huge patch.
Comment #52
webchickIt has been impossible to commit a patch like this until recently, because we were above our issue queue thresholds. Now, I think the key is timing this properly, and announcing it ahead of time so that people know that all of their patches will break in one fell swoop.
Comment #53
cweagansxref: #1266446: Core-wide code style cleanup
Good job on this :)
Comment #54
oriol_e9gOk! A new updated re-rolled patch is comming! :D
Comment #55
oriol_e9gGo! ...and yes, system.tar.inc and LICENSE.txt excluded :D
Comment #56
aspilicious CreditAttribution: aspilicious commentedI gave this a quick review. Should be commited as fast as possible.
Comment #58
aspilicious CreditAttribution: aspilicious commentedAuwch, one test expects different output. Srry oriol_e9g
Comment #59
oriol_e9gAgain!
modules/filter/tests/filter.url-input.txt and modules/filter/tests/filter.url-output.txt excluded!
Comment #60
webchickEr. Wait. I didn't mean "Re-roll it right now." I meant "We should figure out an opportune time for this to go in, and re-roll it then."
Right now we're back above our issue thresholds again, for example. :(
Comment #61
droplet CreditAttribution: droplet commentedmaybe the day before this #22336: Move all core Drupal files under a /core folder to improve usability and upgrades
Comment #62
oriol_e9gNo problem @webchick :D We can see this like a training for a futur solid fast reroll .)
We are waiting and when you say: now! We can do the final re-roll :D
Comment #63
oriol_e9gNR for Testbot validation.
Remider: When we do the last re-roll exclude in the patch the files:
Comment #64
oriol_e9ghttp://drupal.org/node/22336 Will be released on 1st November.
We can do our last re-roll in October 31, November 1 or the day after Novembre 2.
I'm using regular expressions and coder module to roll this patch, so we can do the last re-roll with similar effort after and before.
It's a good idea commit both patches together and only break issue queue patches one time.
Comment #65
cweagansI'd recommend doing it before, unless you're coordinating with the Coder module maintainers to make sure Coder looks in the /core directory.
Comment #66
droplet CreditAttribution: droplet commentedYes. Doing it before. #22336: Move all core Drupal files under a /core folder to improve usability and upgrades, could rollback if that has any unknown error..
Comment #67
droplet CreditAttribution: droplet commentedis it the right time to reroll it ?
Comment #68
oriol_e9gYes, maybe we can return to this epic war.
Comment #69
oriol_e9gComment #70
TravisCarden CreditAttribution: TravisCarden commentedI'd like to help with this. @oriol_e9g, it sounds like you have a script to generate what you've done above. If you can regenerate it now that #22336: Move all core Drupal files under a /core folder to improve usability and upgrades is complete, I'll review the result with Drupal Code Sniffer to make any manual fixes necessary. (If you can explain how to apply the patch you've already submitted now that everything's in the
core/
directory, I'll just work with that, but I don't know how to apply it in light of that change.)Comment #71
TravisCarden CreditAttribution: TravisCarden commentedI think my intentions were a little more expansive than what's represented in the patch in #59. Attached, for example, is what I would do with aggregator module. I just worked through the output of the Drupal Code Sniffer. I think expanding the DockBlocks as it prescribes would be most helpful. Can anyone comment on my patch? Am I going in the right direction? Should I continue on to the other modules and includes?
Comment #72
TravisCarden CreditAttribution: TravisCarden commentedComment #73
Lars Toomre CreditAttribution: Lars Toomre commentedI commend you for taking this on. The only issue that I noticed in your patch is the addition of $form and $form_state @param directives. My understanding of the documentation standards is that these should be excluded. Once done, I will take a closer look at your patch.
Nota bene: Think about breaking this initiative up into chunks. I am not sure what the right size should be. You might want to check with Jennifer, Angie or Nat directly.
Comment #74
TravisCarden CreditAttribution: TravisCarden commentedThank you, @Lars Toomre. Here's an updated patch. I've filed a couple of issues for clarification of questions that have come up working on this:
Comment #75
oriol_e9gMy experience with this issue says that it's better to split this task in diferent issues (by module or by file).
It's too hard to commit a big patch that broken a lot of other patches in the queue.
Open a diferent issue by module and mantain here a list of issues for monitoring. I can help with patches and reviewing.
Comment #76
oriol_e9gComment #77
TravisCarden CreditAttribution: TravisCarden commentedCan do. See the updated description.
Comment #78
jhodgdonIf you're going to start a meta-issue, you should also start by listing out all the modules to be done. This is a huge effort to coordinate... See
#1310084: [meta] API documentation cleanup sprint
for an example.
Best of luck... but really IMO you would be better off working on automated tools to fix these problems rather than manual patches. That API docs cleanup has been going for months and we're getting close, but it has been a real battle...
Comment #79
oriol_e9gYes! This is big battle! Be sure you're ready for the challenge.
Comment #80
TravisCarden CreditAttribution: TravisCarden commentedDeprecated in favor of #1518116: [meta] Make Core pass Coder Review.
Comment #80.0
TravisCarden CreditAttribution: TravisCarden commentedUpdate issue description.