Support from Acquia helps fund testing for Drupal Acquia logo

Comments

oriol_e9g’s picture

FileSize
11.5 KB

More fixes, more coding standard love.

scor’s picture

Status: Needs review » Needs work
+++ 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.

oriol_e9g’s picture

Status: Needs work » Needs review
FileSize
11.5 KB

ok, trying again.

bfroehle’s picture

Patch in #3 looks good. How did you generate a list of these coding style bugs?

scor’s picture

Status: Needs review » Reviewed & tested by the community

looks good now

oriol_e9g’s picture

@bfroehle with coder module: http://drupal.org/project/coder

droplet’s picture

Status: Reviewed & tested by the community » Needs work

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>';
 
oriol_e9g’s picture

Assigned: Unassigned » oriol_e9g
oriol_e9g’s picture

Status: Needs work » Needs review
FileSize
37.77 KB

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.

oriol_e9g’s picture

Ops! One more.

oriol_e9g’s picture

Status: Needs review » Needs work

More fixes comming

oriol_e9g’s picture

FileSize
67.28 KB

Every loop it's a bigger patch :S

oriol_e9g’s picture

Status: Needs work » Needs review
FileSize
67.28 KB
droplet’s picture

FileSize
69.86 KB

catch few more SPACE issue.

oriol_e9g’s picture

Status: Needs review » Reviewed & tested by the community

Tests passes and patch #14 cleanly apply

rfay’s picture

Version: 7.x-dev » 8.x-dev
Status: Reviewed & tested by the community » Needs review

D8 first. Let's get these fixed on D8 and into D7.

oriol_e9g’s picture

Ok. Trying again, D8 patch.

Lars Toomre’s picture

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.

droplet’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
90.59 KB

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, drupal-coding-standards.patch, failed testing.

droplet’s picture

Status: Needs work » Needs review
FileSize
90.59 KB
droplet’s picture

Status: Needs review » Reviewed & tested by the community

#19, I uploaded a wrong file.

#21 & #17 exactly same patch with rename, so RTBC.

tstoeckler’s picture

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

droplet’s picture

okay. again.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, drupal-coding-standards_0.patch, failed testing.

droplet’s picture

Status: Needs work » Needs review
FileSize
90.59 KB

Status: Needs review » Needs work

The last submitted patch, drupal-coding-standards_0_0.patch, failed testing.

droplet’s picture

Status: Needs work » Needs review
FileSize
90.59 KB

strange, the file is different than I upload... what's wrong..

oriol_e9g’s picture

#28: newwwwww_one.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, newwwwww_one.patch, failed testing.

oriol_e9g’s picture

Status: Needs work » Needs review
FileSize
130.38 KB

Again. Infinite perseverance.

droplet’s picture

FileSize
130.6 KB

missed this one :)

-    preg_match_all('/ (-?)("[^"]+"|[^" ]+)/i', ' ' .  $this->searchExpression , $keywords, PREG_SET_ORDER);
+    preg_match_all('/ (-?)("[^"]+"|[^" ]+)/i', ' ' . $this->searchExpression, $keywords, PREG_SET_ORDER);
wojtha’s picture

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.

oriol_e9g’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
130.6 KB

Only file name change.

wojtha’s picture

Status: Reviewed & tested by the community » Needs review

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.

aspilicious’s picture

Status: Needs review » Needs work
+++ 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.

droplet’s picture

Status: Needs work » Needs review
FileSize
129.76 KB

Okay. regenerate again:

git apply --exclude=modules/system/system.tar.inc
git diff > 1109202-37_coding_standards.patch

jbrown’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Coding standards

Looks good!

webchick’s picture

Status: Reviewed & tested by the community » Postponed

Marking this postponed until the # of criticals for D7 & D8 gets < 15.

oriol_e9g’s picture

Status: Postponed » Needs review
Issue tags: -Coding standards

#37: 1109202-37_coding_standards.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Coding standards

The last submitted patch, 1109202-37_coding_standards.patch, failed testing.

oriol_e9g’s picture

Status: Needs work » Needs review
FileSize
136.92 KB

Drupal 7.4 released
D7 & D8 criticals < 15
Try again

oriol_e9g’s picture

No reviews?

oriol_e9g’s picture

No reviews?

droplet’s picture

Status: Needs review » Reviewed & tested by the community

I'm back :)

droplet’s picture

Status: Reviewed & tested by the community » Needs work

@oriol_e9g,

see #36 and #37

jbrown’s picture

Status: Needs work » Needs review
Issue tags: -Coding standards

#42: 1109202-42_coding_standards.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Coding standards

The last submitted patch, 1109202-42_coding_standards.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review
FileSize
134.69 KB
git apply --exclude=modules/system/system.tar.inc --reject 1109202-42_coding_standards.patch

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

solotandem’s picture

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.

oriol_e9g’s picture

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.

webchick’s picture

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.

cweagans’s picture

xref: #1266446: Core-wide code style cleanup

Good job on this :)

oriol_e9g’s picture

Ok! A new updated re-rolled patch is comming! :D

oriol_e9g’s picture

Go! ...and yes, system.tar.inc and LICENSE.txt excluded :D

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

I gave this a quick review. Should be commited as fast as possible.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1109202-55-coding-standards.patch, failed testing.

aspilicious’s picture

Auwch, one test expects different output. Srry oriol_e9g

oriol_e9g’s picture

Again!
modules/filter/tests/filter.url-input.txt and modules/filter/tests/filter.url-output.txt excluded!

webchick’s picture

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. :(

droplet’s picture

oriol_e9g’s picture

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

oriol_e9g’s picture

Status: Needs work » Needs review

NR for Testbot validation.

Remider: When we do the last re-roll exclude in the patch the files:

modules/system/system.tar.inc
LICENSE.txt
modules/filter/tests/filter.url-input.txt
modules/filter/tests/filter.url-output.txt
oriol_e9g’s picture

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.

cweagans’s picture

I'd recommend doing it before, unless you're coordinating with the Coder module maintainers to make sure Coder looks in the /core directory.

droplet’s picture

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

droplet’s picture

is it the right time to reroll it ?

oriol_e9g’s picture

Yes, maybe we can return to this epic war.

oriol_e9g’s picture

Status: Needs review » Needs work
TravisCarden’s picture

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

TravisCarden’s picture

FileSize
39.09 KB

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?

TravisCarden’s picture

Status: Needs work » Needs review
Lars Toomre’s picture

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.

TravisCarden’s picture

Assigned: oriol_e9g » Unassigned
FileSize
38.17 KB

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:

oriol_e9g’s picture

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.

oriol_e9g’s picture

Status: Needs review » Active
TravisCarden’s picture

Title: Drupal coding standards fixes » Fix coding standards violations across Drupal core

Can do. See the updated description.

jhodgdon’s picture

Title: Fix coding standards violations across Drupal core » [meta] Fix coding standards violations across Drupal core

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

oriol_e9g’s picture

FileSize
26.02 KB

Yes! This is big battle! Be sure you're ready for the challenge.

TravisCarden’s picture

Status: Active » Closed (duplicate)
TravisCarden’s picture

Issue summary: View changes

Update issue description.