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.
In #652650: Change 'elseif' standard to 'else if' we briefly discussed changing the Drupal coding standards from "elseif" to "else if", but quickly realized the standard is the best choice. However, there is some code in both D8 and D7 that uses "else if", so we need to enforce this. Simple patch, just replace every occurrence of "else if" to "elseif".
Comment | File | Size | Author |
---|---|---|---|
#30 | drupal-1509838-30.patch | 1.22 KB | tim.plunkett |
#26 | 1509838_26_elseif.patch | 1.85 KB | cosmicdreams |
#14 | 1509838-elseif-everywhere.patch | 3.23 KB | klausi |
#11 | 1509838_11_elseif.patch | 9.71 KB | cosmicdreams |
#1 | 1509838-elseif-everywhere.patch | 3.23 KB | klausi |
Comments
Comment #1
klausiHere is a simple find and replace command to track down all occurences of "else if" excluding Javascript files (execute in a Unix-style environment in your Drupal 8 root folder):
find . -path './.git' -prune -o -not \( -name '*.js' \) -type f -exec sed -i -e "s/else if/elseif/g" {} \;
This also changes core/lib/Drupal/Component/Archiver/ArchiveTar.php, which is a third party library. You can revert that change with
git checkout core/lib/Drupal/Component/Archiver/ArchiveTar.php
Patch attached.
And I think it is not necessary to backport this minor change to Drupal 7, so removing tag.
Comment #2
klausiAdding "find and replace" tag, so that I remember where I put those useful find&sed commands. It can be quite tedious to get them actually right.
Comment #3
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedLooks pretty straight forward.
Comment #4
jhodgdonWow, really, only 6 places in all of Core? So why were we thinking of going in the other direction? :)
Comment #5
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedYep, that seams to be all ;)
(Except tons of javascript that needs to stay
else if
and third party PHP code.)Comment #6
droplet CreditAttribution: droplet commentedupdate version of Archive_Tar #1414508: update system.tar.inc to latest Archive_Tar (Pear)
Comment #7
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedWow, excellent!
Comment #8
jhodgdonRE #6 -- um, why is that relevant here?
Comment #9
droplet CreditAttribution: droplet commented@jhodgdon
the 3rd party script contains "else if" (comment #5)
Comment #10
cosmicdreams CreditAttribution: cosmicdreams commentedThe new ArchiveTar.php in /core/lib/Drupal/Component/Archiver adds 20 more instances of this issue. Preparing a patch.
Comment #11
cosmicdreams CreditAttribution: cosmicdreams commentedHere's a patch that includes those changes. In reading previous comments it seems that these changes were intended to be included but were somehow.
Comment #12
jhodgdonWe should not be doing anything to third-party scripts at all?
Comment #13
cosmicdreams CreditAttribution: cosmicdreams commentedAlso, isn't it a standard to scope elseif's no matter what?
for example:
Should the previous code be this instead?
Comment #14
klausiDo not try to "fix" external third party files, because this will be overwritten anytime the file is updated from upstream.
Reuploading patch from #1.
Comment #15
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedRTBC seconded. #14 is exactly what we need.
Comment #16
webchickTossing to Jennifer since it's coding standards related.
Comment #17
jhodgdonThe patch seems to be fine, although I haven't verified that it catches all the spots...
But I also don't see anywhere on http://drupal.org/coding-standards where it says we should be using elseif as opposed to else if. So I think we need to draft some wording, probably for the section "control structures", before we do this patch?
Comment #18
webchickHm. Is that not already documented by virtue of the code sample directly beneath the "Coding standards" heading? I'm not sure we need to spell it out more than that, do we?
Comment #19
jhodgdonI think it should be spelled out in a note below, if it is a standard, especially given that PHP language supports both.
Comment #20
jhodgdonIf everyone already agrees that it's a de-facto standard (as evidenced by most of core following it), then I can put a note there. Does it need to be in Coder too?
Comment #21
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedCoder already has Use "elseif" in place of "else if".
Comment #22
franz+1 for the note.
Comment #23
jhodgdonI added a brief note to the coding standards. I'll get the patch committed after I verify independently that it gets all the else if spots (except in 3rd-party libraries that we aren't modifying).
Comment #24
jhodgdonPatch above needs a reroll -- someone has already removed the else if lines from locale.admin.inc.
I have also just verified (I think) that the only places outside of Archiver and .js files that have elseif are:
using
Comment #25
jhodgdonUnassigning in hopes that someone will reroll this patch.
Comment #26
cosmicdreams CreditAttribution: cosmicdreams commentedHere's a the reroll
Comment #27
cosmicdreams CreditAttribution: cosmicdreams commentedComment #28
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedThank you, cosmicdreams. Assuming tests pass.
Comment #29
jhodgdonCommitted to 8.x, thanks!
Patch needs reroll for 7.x, where the following command
says there are only two places to patch:
Note that system.tar.inc in d7 I think is the same as Archiver.inc in D8 -- it's a 3rd-party file so we leave it alone.
Comment #30
tim.plunkettOkay.
Comment #31
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedThanks!
Comment #32
jhodgdonCommitted to 7.x. Thanks!
Comment #33.0
(not verified) CreditAttribution: commentedtypo