Enforce coding convention on "elseif"

you mean enforce "elseif" instead of "else if" :-)
Patch is :
- else if
+ elseif

Any reasoning behind enforcing "elseif" instead of "else if". Seems like a matter of taste thing that doesn't make anyone's life easier or harder. If we're going to enforce this convention, I feel like there should be a rationale. Note that the example does not mention anything about "elseif" -- it just so happens that the author probably prefers "elseif". I prefer "else if" -- but that's just a result of my first learn php book using "else if".

+1 elseif

From http://fr2.php.net/manual/en/control-structures.elseif.php

Note that elseif and else if will only be considered exactly the same when using curly brackets as in the above example. When using a colon to define your if/elseif conditions, you must not separate else if into two words, or PHP will fail with a parse error.

<?php

/* Incorrect Method: */
if($a > $b):
    echo $a." is greater than ".$b;
else if($a == $b): // Will not compile.
    echo "The above line causes a parse error.";
endif;

/* Correct Method: */
if($a > $b):
    echo $a." is greater than ".$b;
elseif($a == $b): // Note the combination of the words.
    echo $a." equals ".$b;
else:
    echo $a." is neither greater than or equal to ".$b;
endif;
?>

Besides the reason mentioned by lilou, the main advantage is consistency. Even if "elseif" had no advantages over "else if", having only one is better having than a mix of the two.

Here's my review of the patch:
* It does replace every "else if" in PHP files with "elseif" (given the method used, that's no surprise).
* It does not break anything (all tests run with 22 failures before and after patch).

This is ready for Drupal 7, I'm not sure we need to backport this to Drupal 6. The hardest part is going to get contributors to use "elseif" exclusively in their patches.

If this is part of the approved coding standards, then I can commit this patch for the code review module, which will aid getting contrib maintainers to use this syntax: http://drupal.org/node/282402 We might want to update the coding standards doc to make it clearer also. At the moment, it's just implied from the indentation example.

Cheers,
Stella

Subscribing.

Coding standards already recommend elseif (see the Control Stuctures section of http://drupal.org/coding-standards), though it isn't explicitly stated.

only enforcing the existing standard.

Document about coding standard is not a coding standard till it is not followed by core. Core coding style is the policy..

here is a very nice example: #271142: constant values in db_query

See also http://groups.drupal.org/node/13295

@Pasqualle: that's actually incorrect. The coding standard always win. Core is merely an implementation of the coding standard - patches are made to core's style /because/ of the coding standard, not in spite of it. Coder.module came into existence to route out errors in core's style when matched against the coding standard, not to find out what core asserts as policy and then modify the standard.

Marked http://drupal.org/node/53882 a duplicate of this bug, although mine was first. :D

+1. When I rolled a patch for this a long time ago, there was resistance to adding it since everyone would need to re-roll their patches. Dude. It's called code thaw. ;)

My rationale for this change is that I train a lot of new developers, and existing developers who are new to Drupal, and/or PHP. And the more consistent the code is, the less that new people need to think and wonder about stuff, and the more they can "tune out" and just focus on the problem at hand. For example, when all hooks look exactly like this:

/**
 * Implementation of hook_foo().
 */
function mymodule_foo(...) {
  ...
}

Once I learn that, I can tune out any more hook declarations. I know that when I see a comment like that, I'm looking at a hook.

When sometimes arrays look like this:

$array = array('blah' => 'blah', 'blee' => 'blee', 'bork' => 'bork');

and sometimes like this:

$array = array(
  'blah' => 'blah',
  'blee' => 'blee',
  'bork' => 'bork',
);

Now, it becomes an area of focus, and thus a road block in a new developers' learning. Which one is right? How do I know? How can I find out? Am I wrong, or is core wrong, or is the whole dang world wrong? I can't move beyond this, because I'm new and don't have a comfort level enough to know that this bit of formatting is not important, where other bits of formatting are. And once I do figure it out and I see inconsistency later, it's going to bother me and my eye will be bouncing to those lines instead of what I'm actually working on. etc.

If core conforms to a standard, contrib will (eventually, painfully...) follow. Since elseif is what's been documented for 7 years or so, and because of lilou's rationale in #4 (which is interesting and I didn't know!) I'm very +1 for this change.

Patch still applies with offset.

+1 as well, mainly because of lilou's findings.

However, let's wait until all current monster patches are in, ok? Because this patch can be re-rolled in no time. NG-DBL at least, more?

I'm going to postpone this one a bit in order not to break too many other patches -- such as the database patch. We'll come back to this one a bit later. Seems like this is an easy patch to re-generate thanks to the little script Damien wrote.

If I'm not mistaken, this could be re-rolled now.

totally don't see the purpose in this bit of the coding standards. The only place it makes any sense is the template files. As it is I prefer the readability of white space.

Patch in #20 break JavaScript files since elseif is not a valid statement. I personally think "else if" makes more sense for consistency with JavaScript, but since the coding standards currently say "elseif", here's the revised patch.

Slightly side-tracking this, sorry. The above discussion assumes that the coding standards doc says to use 'elseif' instead of 'else if'. While this can be implied from the first example in the doc, it's not explicitly stated anywhere. If this is going to be the standard, can we update the coding standards guidelines to make it clearer?

Cheers,
Stella

I know some of you are going to laugh at me and even call me stupid, but here's my two pesos worth.

Well, as someone who comes from a long background in IT (or DP as it was called before that), and has used more than 2 dozen languages in her career, I don't particularly like either "else if" or "elseif". Someone new to PHP can get very confused and lost in this code (been there, done that). I prefer a more structured approach and if I was teaching right now, it is what I would encourage my students to use.

  if (a == b) {
    do something...
  }
  else {
    if (c > d) {
      do something else...
    }
  }

The indentation is straightforward and consistent with other languages and, IMHO, structured programming practices. PHP is the first "web" language I have used - it is also the first language I have used that allows anything like "elseif."

@NancyDru: It is much more readable but the extra indentation makes it a code bloat..

The best thing would be to not use else, else if and elseif at all. :) That is the most readable code. Everyone should at least once try to write code without else.

@NancyDru: C, C++, C#, Perl, PHP, Python, JavaScript, Visual Basic, Java, Ruby. All of those have some form of elseif. As someone who has been raised on VB, Java, then C++, I find your approach terribly unreadable and more difficult for complex comparisons. But debating the use of elseif itself is getting off track.

Right now we really need someone higher up to make a decision on what we're going to enforce as a coding standard, so we can fix the 50/50 standard throughout core.

It seems like this entire thread boils down to three things:

1. The only compelling reason for elseif is that PHP will yield a parse error if you use <?php else if: ?> syntax, which we often do in themes. (#4)
2. The only compelling reason for else if is that JavaScript fails to parse elseif, so it provides consistency across languages. (#22)
3. Everything else is various peoples' personal preferences.

Since #1 is a legitimate PHP bug, and since elseif has been the one documented in the coding standards since time immemorial, let's go with elseif.

Patch rerolled and ready for 7.x. All tests pass 100% and with no changes other than coding standards, this should be RTBC.

So. I really really want to commit this patch. It's just a question of "when." Do we postpone until code freeze, and throw a mass of clean-up patches like this, whitespace fixes, etc. at the very end when major patches are on ice for several months? Do we do it right before (or after) the next UNSTABLE-X tag? Suggestions?

if you're going to do it, do it sooner. that way you can nit pick everyone to death now and clear out the RTBC queue ;) seriously on the sooner part though. if it's going to be the coding standard let everyone get used to it.

Personally it gets frustrating with these coding style patches. I use KomodoIDE that automatically corrects tabs vs spaces and removes trailing spaces, and so nearly every time I update HEAD and edit any code to roll patches, I keep finding more and more coding style errors. Now that I've got that off my chest, I'd like to see these kinds of issues fixed now in mass, so that people working on HEAD aren't confused up by the inconsistencies on core's coding style. Any remaining code style errors that happen to find their way in after that can be fixed during the code freeze.

Patch in #28 applied with some offsets, but I'm providing a revised patch with two more "else if"s found and cleaning the offsets.

Edit: Added D6 patch.

Ok. I'm planning on rolling UNSTABLE-2 on Sunday. I will make this the very last patch I commit, and add "else if" to my list of "do not commit patches" list. ;) Thanks for your perseverance. :)

+1 for postponing coding-style patches like this, and +1000 for a dedicated timeframe in front of releasing a new stable, so hundreds of developers can focus on large(r) coding-style patches.

Reattaching D7 patch to confirm that it applies cleanly in testing.d.org

Aha! Non-existant CVS file from other patch included. New patch with that file's change removed. Should apply cleanly. *crosses fingers*

There we go! Applied cleanly (http://testing.drupal.org/node/15107)

+1 to applying it sooner rather than later.

If we wait until code freeze, this'll sit in the queue for months and months (hopefully months and months!) distracting from all the other patches that are RTBC, so committing asap would be great.

Very minimal re-roll.

One final roll for UNSTABLE-2!

Ok. Committed!

/me ducks flying, rotten tomatoes. ;)