Posted by oriol_e9g on March 29, 2011 at 12:16pm
16 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | base system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (duplicate) |
| Issue tags: | coding standards |
Issue Summary
The larger effort to get all of Drupal core into compliance with Drupal's coding standards has been broken into the following issues:
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| drupal-conding-standards.patch | 6.18 KB | Idle | PASSED: [[SimpleTest]]: [MySQL] 31,544 pass(es). | View details | Re-test |
Comments
#1
More fixes, more coding standard love.
#2
+++ b/includes/filetransfer/ssh.inc@@ -72,7 +72,8 @@ class FileTransferSSH extends FileTransfer implements FileTransferChmodInterface
- } else {
+ } ¶
+ else {
you are introducing trailing whitespaces in your else statements changes.
#3
ok, trying again.
#4
Patch in #3 looks good. How did you generate a list of these coding style bugs?
#5
looks good now
#6
@bfroehle with coder module: http://drupal.org/project/coder
#7
it has 2 spaces between .= and t('After.....
$output .= t('After you create an image style, you can add effects: crop, scale, resize, rotate, and desaturate (other contributed modules provide additional effects). For example, by combining effects as crop, scale, and desaturate, you can create square, grayscale thumbnails.') . '<dd>';
#8
#9
The 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.
#10
Ops! One more.
#11
More fixes comming
#12
Every loop it's a bigger patch :S
#13
#14
catch few more SPACE issue.
#15
Tests passes and patch #14 cleanly apply
#16
D8 first. Let's get these fixed on D8 and into D7.
#17
Ok. Trying again, D8 patch.
#18
This 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.
#19
rename for testbots.
It's same as #17, so RTBC by me :)
it's a bit crazy to review it, hoping commit to core immediately :)
#20
The last submitted patch, drupal-coding-standards.patch, failed testing.
#21
#22
#19, I uploaded a wrong file.
#21 & #17 exactly same patch with rename, so RTBC.
#23
--- a/modules/field_ui/field_ui.test+++ b/modules/field_ui/field_ui.test
- $type_name = strtolower($this->randomName(8)) . '_' .'test';
+ $type_name = strtolower($this->randomName(8)) . '_' .'test';
...
- $type_name2 = strtolower($this->randomName(8)) . '_' .'test';
+ $type_name2 = strtolower($this->randomName(8)) . '_' .'test';
Note the last concatenation is missing a space.
--- a/modules/search/search-result.tpl.php+++ b/modules/search/search-result.tpl.php
- preg_match_all('/ (-?)("[^"]+"|[^" ]+)/i', ' ' . $this->searchExpression , $keywords, PREG_SET_ORDER);
+ preg_match_all('/ (-?)("[^"]+"|[^" ]+)/i', ' ' . $this->searchExpression , $keywords, PREG_SET_ORDER);
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
#24
okay. again.
#25
The last submitted patch, drupal-coding-standards_0.patch, failed testing.
#26
#27
The last submitted patch, drupal-coding-standards_0_0.patch, failed testing.
#28
strange, the file is different than I upload... what's wrong..
#29
#28: newwwwww_one.patch queued for re-testing.
#30
The last submitted patch, newwwwww_one.patch, failed testing.
#31
Again. Infinite perseverance.
#32
missed this one :)
- preg_match_all('/ (-?)("[^"]+"|[^" ]+)/i', ' ' . $this->searchExpression , $keywords, PREG_SET_ORDER);+ preg_match_all('/ (-?)("[^"]+"|[^" ]+)/i', ' ' . $this->searchExpression, $keywords, PREG_SET_ORDER);
#33
When 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.
#34
Only file name change.
#35
Cool :-) 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.
#36
+++ b/modules/system/system.tar.incundefined+++ b/modules/system/system.tar.incundefined
@@ -94,10 +94,10 @@ class Archive_Tar // extends PEAR
* If the compress argument is set the tar will be read or created as a
* gzip or bz2 compressed TAR file.
*
- * @param string $p_tarname The name of the tar archive to create
+ * @param string $p_tarname The name of the tar archive to create
* @param string $p_compress can be null, 'gz' or 'bz2'. This
* parameter indicates if gzip or bz2 compression
- * is required. For compatibility reason the
+ * is required. For compatibility reason the
* boolean value 'true' means 'gz'.
You 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.
#37
Okay. regenerate again:
git apply --exclude=modules/system/system.tar.inc
git diff > 1109202-37_coding_standards.patch
#38
Looks good!
#39
Marking this postponed until the # of criticals for D7 & D8 gets < 15.
#40
#37: 1109202-37_coding_standards.patch queued for re-testing.
#41
The last submitted patch, 1109202-37_coding_standards.patch, failed testing.
#42
Drupal 7.4 released
D7 & D8 criticals < 15
Try again
#43
No reviews?
#44
No reviews?
#45
I'm back :)
#46
@oriol_e9g,
see #36 and #37
#47
#42: 1109202-42_coding_standards.patch queued for re-testing.
#48
The last submitted patch, 1109202-42_coding_standards.patch, failed testing.
#49
git apply --exclude=modules/system/system.tar.inc --reject 1109202-42_coding_standards.patchRejected files:
# misc/ajax.js.rej# modules/field/field.form.inc.rej
# modules/field_ui/field_ui.test.rej
# modules/user/user.admin.inc.rej
I could fix all hunks manually, only ajax.js does not contain the white space issues anymore (was probably fixed elsewhere).
#50
Are 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.
#51
I 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.
#52
It 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.
#53
xref: #1266446: Core-wide code style cleanup
Good job on this :)
#54
Ok! A new updated re-rolled patch is comming! :D
#55
Go! ...and yes, system.tar.inc and LICENSE.txt excluded :D
#56
I gave this a quick review. Should be commited as fast as possible.
#57
The last submitted patch, 1109202-55-coding-standards.patch, failed testing.
#58
Auwch, one test expects different output. Srry oriol_e9g
#59
Again!
modules/filter/tests/filter.url-input.txt and modules/filter/tests/filter.url-output.txt excluded!
#60
Er. 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. :(
#61
maybe the day before this #22336: Move all core Drupal files under a /core folder to improve usability and upgrades
#62
No 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
#63
NR for Testbot validation.
Remider: When we do the last re-roll exclude in the patch the files:
modules/system/system.tar.incLICENSE.txt
modules/filter/tests/filter.url-input.txt
modules/filter/tests/filter.url-output.txt
#64
http://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.
#65
I'd recommend doing it before, unless you're coordinating with the Coder module maintainers to make sure Coder looks in the /core directory.
#66
Yes. 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..
#67
is it the right time to reroll it ?
#68
Yes, maybe we can return to this epic war.
#69
#70
I'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.)#71
I 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?
#72
#73
I 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.
#74
Thank 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:
#75
My 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.
#76
#77
Can do. See the updated description.
#78
If 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...
#79
Yes! This is big battle! Be sure you're ready for the challenge.
#80
Deprecated in favor of #1518116: [meta] Make Core pass Coder Review.