WordPress-Coding-Standards icon indicating copy to clipboard operation
WordPress-Coding-Standards copied to clipboard

Newline required after opening brace triggered for allowed loop syntax

Open desrosj opened this issue 7 years ago • 18 comments

The WordPress coding standards state:

You are free to use the alternative syntax for control structures (e.g. if/endif, while/endwhile)—especially in your templates where PHP code is embedded within HTML, for instance:

<?php if ( have_posts() ) : ?>
    <div class="hfeed">
        <?php while ( have_posts() ) : the_post(); ?>
            <article id="post-<?php the_ID() ?>" class="<?php post_class() ?>">
                <!-- ... -->
            </article>
        <?php endwhile; ?>
    </div>
<?php endif; ?>

The current version of the standard does not allow this though. Newline required after opening brace is thrown for the following code snippets:

  • <?php while ( have_posts() ) : the_post(); ?>
  • <?php if ( have_posts() ) : while ( have_posts() ) : the_post(); ?>

Running phpcbf on these two examples will produce the following, respectively:

<?php
	while ( have_posts() ) :
		the_post();
?>
<?php
	if ( have_posts() ) :
		while ( have_posts() ) :
			the_post();
?>

desrosj avatar Aug 04 '17 13:08 desrosj

Where you put the <?php delimiter, is unrelated to the use of alternative control structure syntax.

<?php
if ( have_posts() ) :
	?>
	<div class="hfeed">
		<?php
		while ( have_posts() ) :
			the_post();
			?>
			<article id="post-<?php the_ID() ?>" class="<?php post_class() ?>">
				<!-- ... -->
			</article>
			<?php
		endwhile;
		?>
	</div>
	<?php
endif;
?>

Would likely pass.

GaryJones avatar Aug 04 '17 13:08 GaryJones

Maybe I mislabeled the ticket.

<?php if ( have_posts() ) : while ( have_posts() ) : the_post(); ?> <?php if ( have_posts() ) : the_post(); ?> and <?php while ( have_posts() ) : the_post(); ?> are very common, have been for a long time, and are documented as allowed in the handbook.

My opinion is that new lines should not be required for these cases.

desrosj avatar Aug 04 '17 17:08 desrosj

Updated the ticket title (the first title I wrote before diving deeper into the issue to debug)

desrosj avatar Aug 04 '17 17:08 desrosj

This was previously discussed in https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/issues/1000 and the requirement for PHP open/close tags to be on their own lines for multi-statement embedded PHP has been added explicitly to the WP Core Handbook: https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/#opening-and-closing-php-tags

So, no, this is not "allowed in the handbook".

jrfnl avatar Aug 04 '17 17:08 jrfnl

The reference @jrfnl gave leads to:

When embedding multi-line PHP snippets within a HTML block, the PHP open and close tags must be on a line by themselves.

Though, to be fair, that's a little confusing when it relates to, for instance <?php endwhile; ?>.

GaryJones avatar Aug 04 '17 18:08 GaryJones

Though, to be fair, that's a little confusing when it relates to, for instance <?php endwhile; ?>.

Yeah, they are allowed to both be on the same line, but if only one of them is on the line, then it has to be by itself without other code.

Which is why "multi-line" is the important word there.


But this isn't about the tags, it is about brace style. And the handbook does indeed include this example:

<?php if ( have_posts() ) : ?>
    <div class="hfeed">
        <?php while ( have_posts() ) : the_post(); ?>
            <article id="post-<?php the_ID() ?>" class="<?php post_class() ?>">
                <!-- ... -->
            </article>
        <?php endwhile; ?>
    </div>
<?php endif; ?>

https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/#brace-style

The third line (<?php while ( have_posts() ) : the_post(); ?>) puts code on the line after the "brace."

It could be changed to this:

 <?php while ( have_posts() ) : ?>
     <?php the_post(); ?>

And that would also pass.

But I think this pattern is common enough that we might want to consider making a special allowance for it.

JDGrimes avatar Aug 04 '17 18:08 JDGrimes

Which is why "multi-line" is the important word there.

Actually, it's not about "multi-line", but about "multi-statement". This is not clear enough in the handbook and I think it would be good to clarify this there.

Regarding the brace style example - IMHO it would be better to change that code snippet in the handbook to the below:

<?php
while ( have_posts() ) :
     the_post();
     ?>

But I think this pattern is common enough that we might want to consider making a special allowance for it.

The reason the handbook was adjusted and this sniff was added to the Core ruleset is that without it we get into trouble with fixer conflicts.

As it's been decided to add it, I would very much be in favour of applying it consistently and not having semi-random "exceptions".

jrfnl avatar Aug 04 '17 19:08 jrfnl

I'm in agreement here - just the wording / example in the Handbook needs tweaking to make it clearer.

GaryJones avatar Aug 04 '17 19:08 GaryJones

For some more context, Codex actually displays this loop format as well (acknowledging that parts of the Codex are outdated).

My point was only that this is very common out in the wild, and many learning resources I have seen people use have taught this style. Not against change, but just wanted to raise that this is very common, and may be worth looking at further.

desrosj avatar Aug 04 '17 19:08 desrosj

I'm fine with updating the handbook and codex, I just wanted to make sure that we're all on the same page with regard to the fact that this has basically been the de facto standard up to now. Personally, I think that one statement per line is far more readable; it's easy to miss the second statement. So I'm happy to just say this isn't really feasible technically to add an exception for, and move on.

JDGrimes avatar Aug 04 '17 20:08 JDGrimes

Pinging @pento @ntwb - if you agree with the above, could either of you adjust the handbook accordingly ?

jrfnl avatar Aug 04 '17 20:08 jrfnl

Ping @pento @ntwb :arrow_up:

jrfnl avatar Oct 24 '17 03:10 jrfnl

The Loop is a special thing in WordPress, I think exceptions should be made for The Loop

A rudimentary search here on GitHub for while ( have_posts() ) : the_post(); in PHP files resulted in 68 of the 100 results I looked at (the first 10 pages) to use that snippet on a single line.

Here's ~10 codex pages that reference that same single line snippet

https://codex.wordpress.org/The_Loop https://codex.wordpress.org/The_Loop_in_Action https://codex.wordpress.org/Class_Reference/WP_Query https://codex.wordpress.org/Function_Reference/have_posts https://codex.wordpress.org/Function_Reference/wp_reset_query https://codex.wordpress.org/Pagination https://codex.wordpress.org/Function_Reference/rewind_posts

And not just in English: https://codex.wordpress.org/fr:La_Boucle https://codex.wordpress.org/it:Il_Loop https://codex.wordpress.org/pt-br:O_Loop https://codex.wordpress.org/zh-cn:%E5%BE%AA%E7%8E%AF

It pretty much is a defacto standard IMHO and I would favor an exception being added for these to be honest.

ntwb avatar Oct 24 '17 04:10 ntwb

Could it be a warning if the second statement on the same line is the_post();, but an error otherwise? So yes, a semi-exception, but enough to draw attention to it, without it being a run failure?

GaryJones avatar Oct 24 '17 08:10 GaryJones

I disagree about this being specific to loop or needing an exception for it. In my opinion rule that is specifically about braces formatting is irrelevant to alternative syntax.

Alternative syntax is better suited for being mixed in HTML in some cases, exactly because braces and rigid formatting can get in the way of producing neat meaningful readable template.

I think this rule should just stay away from alternative if syntax altogether, neither the change in current coding standard in regard to it is needed.

Rarst avatar Dec 12 '17 11:12 Rarst

I agree with @Rarst here. With HTML templating, you sometimes need to avoid extra whitespace. While you can get around this sniff by echoing prepared strings, the resulting template is far less readable.

mundschenk-at avatar Feb 10 '18 09:02 mundschenk-at

I too agree with @ntwb and @Rarst and believe that an exception should be added for The Loop.

joiglifberg avatar Apr 19 '18 06:04 joiglifberg

Any progress on this? I agree that an exception should be added. The Loop is such a fundamental part of Wordpress development that an exception makes sense.

ghost avatar Jan 14 '19 19:01 ghost