The standard for if () statements in template files is:

if ($foo):
 ....
endif;

The attached patch fixes this in node.tpl.php, though it results in more lines of code (for readability), which is partly why this was split out from the patch in #471142: coding style fixes (part 2)

CommentFileSizeAuthor
coding_style_endif.patch1.58 KBstella

Comments

damien tournoud’s picture

This "standard" seems in frontal opposition with our documented standards [1]. As far as I know, it was only one developer choice. Core was never consistent in that regard (see for example [2]) before all themes except Garland were removed, leaving this the de facto standard.

Can't we go back to something more standard?

[1] http://drupal.org/coding-standards
[2] http://cvs.drupal.org/viewvc.py/drupal/drupal/themes/bluemarine/page.tpl...

stella’s picture

Agreed, I don't believe it's documented (which I think is strange since it's in coder...). It does appear to be the de facto standard. I do remember some discussion about it before, and I think the consensus was that it would be easier for non-programmers to deal with. However, we should probably access this "de-facto" standard and decide what standard we want to use for template files, which is why I separated this patch out from the other changes.

dman’s picture

<opinion class="rant">
- endif makes me queasy, I personally wish it wasn't used anywhere. I don't believe it helps non-coders at all - it's just as likely to get broken by html edits as the other, and has no symmetry with any of the code you will find in the drupal.org docs. Kill it. Really.
</opinion>
Even so - this patch is using the if() : endif paradigm in a place where it is real code logic, not just an inline display switch. The line it stepped over is tiny, but IMO :
- conditionally initializing a string, then printing a string is code.
- conditionally printing a sting is embedded display logic.

IF endifs are acceptable for embedded display, that still doesn't mean they should be promoted up to being used inside code logic.
The approach this patch takes - by splitting out the class string - is an OK move. But using endif in real code - arrg.
... just opinion again :-}

lilou’s picture

Issue tags: +Coding standards

Add tag.

Status: Needs review » Needs work

The last submitted patch failed testing.

stella’s picture

Status: Needs work » Needs review
webchick’s picture

Status: Needs review » Closed (won't fix)

Hm. Sorry, but this is won't fix for me. I'd prefer to see this addressed by #306358: Add $classes to templates with new theme_process_hook functions which is a much more themer-friendly solution to the problem.