Seen in core:

      <?php if ($left): ?>
  <?php if ($source_url) : ?>

The first form, without the space after the closing ), is far more common, and is also seen at http://php.net/manual/en/control-structures.alternative-syntax.php

(Is this right component, btw? Can't find anything else that looks likely.)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon’s picture

Project: Documentation » Drupal core
Version: » 7.x-dev
Component: Missing documentation » documentation
Issue tags: +Coding standards

We usually discuss coding standards in the Drupal Core issue queue, I think.

sun’s picture

Title: define coding standard for colon on PHP control blocks in templates » Coding standards: Alternative control structure syntax in theme templates

The colon should directly follow the closing parenthesis.

In general, Drupal does not use nor follow the white-space-everywhere-for-visual-clarity that can be seen in a minority of projects, such as Joomla.

In this case, the lonely separated colon could be easily misunderstood as a colon belonging to a ternary operator.

xjm’s picture

Issue tags: +Novice

So we need to hunt down all the places in our templates where this happens. A grep for ') :' in template files would be a good start. Tagging.

xjm’s picture

[milankovitch:drupal | Mon 16:41:11] $ grep -rn ') : ?>' *
modules/aggregator/aggregator-item.tpl.php:27:  <?php if ($source_url) : ?>
modules/aggregator/aggregator-item.tpl.php:33:<?php if ($content) : ?>
modules/aggregator/aggregator-item.tpl.php:39:<?php if ($categories) : ?>
modules/aggregator/aggregator-summary-item.tpl.php:21:<?php if ($source_url) : ?>,
modules/book/book-all-books-block.tpl.php:14:<?php foreach ($book_menus as $book_id => $menu) : ?>
modules/book/book-export-html.tpl.php:43:    <?php for ($i = 1; $i < $depth; $i++) : ?>
modules/book/book-navigation.tpl.php:38:      <?php if ($prev_url) : ?>
modules/book/book-navigation.tpl.php:41:      <?php if ($parent_url) : ?>
modules/book/book-navigation.tpl.php:44:      <?php if ($next_url) : ?>
modules/field/theme/field.tpl.php:52:  <?php if (!$label_hidden) : ?>
modules/field/theme/field.tpl.php:56:    <?php foreach ($items as $delta => $item) : ?>
modules/search/search-block-form.tpl.php:33:  <?php if (empty($variables['form']['#block']->subject)) : ?>
modules/search/search-result.tpl.php:48: *   <?php if (isset($info_split['comment'])) : ?>
modules/search/search-result.tpl.php:72:    <?php if ($snippet) : ?>
modules/search/search-result.tpl.php:75:    <?php if ($info) : ?>
modules/search/search-results.tpl.php:24:<?php if ($search_results) : ?>
modules/user/user-profile-category.tpl.php:28:  <?php if ($title) : ?>
themes/garland/comment.tpl.php:9:  <?php if ($new) : ?>
Niklas Fiekas’s picture

Version: 7.x-dev » 8.x-dev
Status: Active » Needs review
Issue tags: +Needs backport to D7
FileSize
9.55 KB

Here is a patch against 8.x, but it seams to apply cleanly for 7.x too.

I also found that usually elements are indented between if and endif.

<?php if ($foo): ?>
  <!-- Elements here should be indented. -->
^^
<?php endif; ?>

Status: Needs review » Needs work

The last submitted patch, 999040-template-file-coding-style.patch, failed testing.

Niklas Fiekas’s picture

Status: Needs work » Needs review
FileSize
8.32 KB

Ieek, what's that? The patch came out of git-format-patch, git version 1.7.4.4.
Let's see if git diff works.

Status: Needs review » Needs work

The last submitted patch, 999040-template-file-coding-style-2.patch, failed testing.

Niklas Fiekas’s picture

Status: Needs work » Needs review
FileSize
8.21 KB

Next random try. Maybe no a/ b/ prefixes anymore?

Status: Needs review » Needs work

The last submitted patch, 999040-template-file-coding-style-3.patch, failed testing.

xjm’s picture

Hi Niklas,

Maybe try rolling the patch with git diff origin/8.x rather than git format-patch? Maybe it is failing on the php example in your commit message or something.

Niklas Fiekas’s picture

I thought that too, so I next (http://drupal.org/node/999040#comment-4957014 ) tried a git diff that has no commit message.

Edit: I just asked what to do in #drupal-gitsupport.

jhodgdon’s picture

The patch in #7 looks like the right formatting. Maybe you can try making a clean git repository for Drupal 8 and seeing if that patch will apply with:
patch -p1 < (filename)
from the Drupal root.

Looking at the error log (choose View Details next to the patch file line in #7), it looks like the patch failed to apply here:

Output: [error: modules/profile/profile-block.tpl.php: No such file or directory
error: modules/profile/profile-listing.tpl.php: No such file or directory].

So maybe these have been removed from Drupal 8? Did you patch against an up-to-date Drupal 8 git repository?

xjm’s picture

Yeah you need to do a git pull. Profile has been removed so no need to patch it. :)

Niklas Fiekas’s picture

Ok, thank you all. The repo was up to date, but it looks like I accedantely commited profile to 8.x some time ago ;)
Wow ... it says

This may be a -p0 (old style) patch, which is no longer supported by the testbots.

but I didn't think it would say that for all failing patches. I would have never figured that out.

Rerolled patch attached.

Niklas Fiekas’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

That text about it maybe being a -p0 patch is just something the test bot always says when it fails to apply a patch. Always look at "view details" to see what actually happened. :)

Anyway, I've reviewed this patch, and if the testbot is happy this time, I think it's good to go for 8.x. We will need a different patch for 7.x, since it has the Profile module still.

Setting to RTBC, pending test bot results.

Niklas Fiekas’s picture

Here is a D7 patch. Not sure if you should do anything with the issue tags, but I guess I shouldn't set it to 7.x-dev and needs review for the testbots until this is in 8.x.

jhodgdon’s picture

Correct. After this is committed to 8.x, we will probably need to re-upload the patch too, because the test bot ignores patches with -d7 suffixes on the name.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 7.x and 8.x. Thanks.

Status: Fixed » Closed (fixed)

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

jhodgdon’s picture