Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Newline required after opening brace triggered for allowed loop syntax #1085

Open
desrosj opened this issue Aug 4, 2017 · 18 comments
Open

Comments

@desrosj
Copy link
Contributor

desrosj commented Aug 4, 2017

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();
?>
@GaryJones
Copy link
Member

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.

@desrosj
Copy link
Contributor Author

desrosj commented Aug 4, 2017

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 desrosj changed the title Alternative Control Structure Syntax Should Be Allowed Newline required after opening brace triggered for allowed loop syntax Aug 4, 2017
@desrosj
Copy link
Contributor Author

desrosj commented Aug 4, 2017

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

@jrfnl
Copy link
Member

jrfnl commented Aug 4, 2017

This was previously discussed in #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".

@GaryJones
Copy link
Member

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; ?>.

@JDGrimes
Copy link
Contributor

JDGrimes commented Aug 4, 2017

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.

@jrfnl
Copy link
Member

jrfnl commented Aug 4, 2017

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".

@GaryJones
Copy link
Member

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

@desrosj
Copy link
Contributor Author

desrosj commented Aug 4, 2017

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.

@JDGrimes
Copy link
Contributor

JDGrimes commented Aug 4, 2017

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.

@jrfnl
Copy link
Member

jrfnl commented Aug 4, 2017

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

@jrfnl
Copy link
Member

jrfnl commented Oct 24, 2017

Ping @pento @ntwb ⬆️

@ntwb
Copy link
Member

ntwb commented Oct 24, 2017

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.

@GaryJones
Copy link
Member

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?

@Rarst
Copy link
Contributor

Rarst commented Dec 12, 2017

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.

@mundschenk-at
Copy link

mundschenk-at commented Feb 10, 2018

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.

@joiglifberg
Copy link

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

@ghost
Copy link

ghost commented Jan 14, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants