Using the Coder module to check any inconsistencies with the drupal coding standards. Here's what was thrown about the aggregator.module

severity: normal Line 313: Use "elseif" in place of "else if"

    else if ($op == 'configure') {

severity: normal Line 324: Use "elseif" in place of "else if"

    else if ($op == 'save') {

severity: normal Line 333: Use "elseif" in place of "else if"

    else if ($op == 'view') {

severity: normal Line 390: Use "elseif" in place of "else if"

  else if (!empty($edit['title'])) {

severity: normal Line 415: Use "elseif" in place of "else if"

  else if (!empty($edit['fid'])) {

severity: normal Line 429: Use "elseif" in place of "else if"

  else if (!empty($edit['title'])) {

severity: normal Line 761: Use "elseif" in place of "else if"

    else if (!empty($item['SUMMARY'])) {

severity: normal Line 764: Use "elseif" in place of "else if"

    else if (!empty($item['CONTENT'])) {

severity: normal Line 792: Use "elseif" in place of "else if"

    else if ($link && $link != $feed['link'] && $link != $feed['url']) {

severity: normal Line 830: Use "elseif" in place of "else if"

  else if ($edit['iid']) {

severity: normal Line 834: Use "elseif" in place of "else if"

  else if ($edit['title'] && $edit['link']) {

...thought I'd report it so it can be fixed in a next version of drupal.

CommentFileSizeAuthor
#17 850152_completely_reverted.patch35.52 KBavpaderno
#16 850152_reverted_patch.patch5.36 KBavpaderno
#5 850152_fix_elseif.patch16.69 KBavpaderno

Comments

klonos’s picture

klonos
he/him
Primary language English
Location 90% Melbourne, Australia - 10% Larissa, Greece
commented
Issue tags: +Coding standards

+tag

EvanDonovan’s picture

EvanDonovan commented
Version: 6.17 » 7.x-dev
Priority: Normal » Minor

Issues are fixed in the highest-numbered version of Drupal and then backported, if necessary.

klonos’s picture

klonos
he/him
Primary language English
Location 90% Melbourne, Australia - 10% Larissa, Greece
commented
Version: 7.x-dev » 6.17

I agree on setting these to minor Evan. As for the version, I am testing on D6 setups and I am not sure the files are the same in their corresponding D7 'brothers' (or if their D7 versions even exist). How about we let another person with D7 setups report these D7 coding standards errors (if any)?

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Version: 6.17 » 7.x-dev
Component: aggregator.module » other
Status: Active » Needs review
StatusFileSize
new 850152_fix_elseif.patch16.69 KB

There are many files that uses else if, instead of elseif; let's see if the patch works.

klonos’s picture

klonos
he/him
Primary language English
Location 90% Melbourne, Australia - 10% Larissa, Greece
commented
Title: Use "elseif" in place of "else if" » D7 Coding standards issues

...lets have all D7 Coding standards issues here then.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Title: D7 Coding standards issues » Use "elseif" in place of "else if"

The report would become a big mess. Let's use a single report for a single task.

klonos’s picture

klonos
he/him
Primary language English
Location 90% Melbourne, Australia - 10% Larissa, Greece
commented
Title: Use "elseif" in place of "else if" » D7 Coding standards issues

Fixing coding standards is a single issue!

Please take a look at my reply here: #850170: The $string argument to t() should not begin or end with a space.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Title: D7 Coding standards issues » Use "elseif" in place of "else if"

If you are going to provide a single patch for all the parts that do not respect the coding standards, then it would be fine. Until you don't provide such patch, please leave the title as it is now.

aspilicious’s picture

aspilicious commented

Don't use one patch to fix all these issues. Do it in smaller patches.
This one is fine.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs review » Reviewed & tested by the community 
+++ modules/system/system.tar.inc	10 Jul 2010 16:54:53 -0000
@@ -1834,11 +1834,11 @@ class Archive_Tar
-                else if (   ($v_list[$i] == '')
+                elseif (   ($v_list[$i] == '')
 				         && ($i!=(sizeof($v_list)-1))
 						 && ($i!=0)) {
                     // ----- Ignore only the double '//' in path,

Maybe since we're already touching those lines we could fix the spacing issues here. There are other spacing issues in this file and I wouldn't want to scope creep that into this patch, but it feels wrong to see such a line with a '+' in front of it.

Either way, this patch is RTBC. I explicitly checked that there are no code changes involved other than 'else if' -> 'elseif' and since the tests pass, this is good to go.

Powered by Dreditor.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Fixed » Needs work 
+++ modules/system/system.tar.inc	10 Jul 2010 16:54:53 -0000
@@ -133,7 +133,7 @@ class Archive_Tar
-            } else if ($p_compress == 'bz2') {
+            } elseif ($p_compress == 'bz2') {

Friends, watch out. You changed the Archive_Tar class here, which does not follow Drupal's coding standards at all, because it's an externally developed library.

I recommend to revert these changes, and, discuss in a separate issue whether we want to change foreign library files like system.tar.inc (but then do it completely, e.g., start by running coder_format on it), or whether we want to keep such files as unaltered as possible for easier comparison with the original code and future updating. And link to that issue here.

Powered by Dreditor.

klonos’s picture

klonos
he/him
Primary language English
Location 90% Melbourne, Australia - 10% Larissa, Greece
commented

Good point Daniel, but I cannot express any opinion regarding this.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented

I agree that we should not change the content of the system.tar.inc file as it contains an external library; I thought it was a modified version adapted for Drupal.

To which revision should the file be reverted? As far as I can see, the file has already had many changes, since it has been added in Drupal repository.
I can provide a patch to restore the file as it was in revision 1.6. Is that enough, or should not we restore the file as it was on revision 1.2?

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Needs work » Needs review
StatusFileSize
new 850152_reverted_patch.patch5.36 KB

The attached patch reverts the code to revision 1.6.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
StatusFileSize
new 850152_completely_reverted.patch35.52 KB

This patch reverts the code to revision 1.2, which is the one without any of the changes done to the library when it has been added to Drupal repository. There is actually a change, but it doesn't aim to fix the code format.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Fixed

Thanks! Hope you agree, since this was not the only cosmetic change, let's move the entire discussion and reversion into #870204: Revert coding style changes to system.tar.inc & other externally developed files

Status: Fixed » Closed (fixed)
Issue tags: -Coding standards

Automatically closed -- issue fixed for 2 weeks with no activity.

Issue tags: +Coding standards

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.