phpcbf breaks php opening and closing tags adding spaces

[giorgosbotis]

Bug Description

Using phpcbf on a .php file with many opening and closing php tags both inline and multiline breaks the file completely. The result has < ? php with the spaces and missing closing tags.

Minimal Code Snippet

The issue happens when running this command:

phpcbf --standard=Wordpress-Core myfile.php

... over a file containing this code:

<?php

if (have_rows('page_block')): ?>
  <?php while (have_rows('page_block')):
    the_row(); ?>
    <?php if (get_row_layout() == 'my_block'): ?>
      <?php include get_theme_file_path(
        'my_block.php'
      ); ?>               
    <?php endif; ?>
  <?php endwhile; ?>
<?php endif; ?>

The file was auto-fixed via phpcbf to:

<?php

if ( have_rows( 'page_block' ) ) : ?>
	<?php
	while ( have_rows( 'page_block' ) ) :
		the_row();
		?>
		<?php
		if ( get_row_layout() == 'my_block' ) :
			< ? php
			include get_theme_file_path(
				'my_block.php'
			);
			?>
					 
	<?php endif; ?>
	<?php endwhile; ?>
	<?php
endif;

... while I expected the code to be fixed to:

<?php

if ( have_rows( 'page_block' ) ) {
  while ( have_rows( 'page_block' ) ) {
    the_row();
    if ( get_row_layout() == 'my_block' ) {
      include get_theme_file_path('/theme_extra/theme_blocks/page/timer_suppliers_block.php');
    }
  }
}

Error Code

Custom ruleset

Environment

Question	Answer
PHP version	8.0.19
PHP_CodeSniffer version	3.7.1
WPCS version	dev
WPCS install type	composer local
IDE (if relevant)	-

Additional Context (optional)

Tested Against develop branch?

• I have verified the issue still exists in the develop branch of WPCS.

[dingo-d]

I can confirm that with the PHPCS 3.6.2, and WPCS 2.3.0 on PHP 7.4.26 I got the same result. So this is a bug (at least as far as the opening tag being broken).

As far as changing the alternative syntax of block structures to braces ones, I don't think phpcbf will do that when core standards are applied, because the alternative syntax (if: ... endif; or while: ... endwhile;) is allowed in the WP core.

I think you should look into DisallowAlternativeSyntaxSniff in the PHPCSExtra ruleset.

If you add it to your phpcs.xml.dist like:

<?xml version="1.0"?>
<ruleset name="Test">
	<description>Test ruleset</description>

	<rule ref="WordPress-Core" />

	<!-- Additional arguments. -->
	<arg value="sp"/>
	<arg name="cache"/>
	<arg name="basepath" value="."/>
	<arg name="parallel" value="8"/>
	<arg name="extensions" value="php"/>

	<rule ref="Universal.ControlStructures.DisallowAlternativeSyntax" />

</ruleset>

The fix will produce:

<?php

if ( have_rows( 'page_block' ) ) { ?>
	<?php
	while ( have_rows( 'page_block' ) ) {
		the_row();
		?>
		<?php
		if ( get_row_layout() == 'my_block' ) {
			< ? php
			include get_theme_file_path(
				'my_block.php'
			);
			?>
					 
	<?php } ?>
	<?php } ?>
	<?php
}

You'd still need to check the extra unnecessary PHP tags (there could probably be some setting or sniff for that as well,didn't poke too deep into it 🤷🏼‍♂️).

[giorgosbotis]

I didn't really expect to change the alternative syntax (exaggerated with that expected code block 😁 )but "DisallowAlternativeSyntaxSniff" is really interesting and thanks for that!

I tried to figure out what triggers the bug with the opening php tag but had no luck. It seems that removing <rule ref="PEAR.Functions.FunctionCallSignature"> helps but that does not make any sense to me.

[GaryJones]

If the starting snippet uses tabs consistently, then the fixer doesn't break that opening tag. It seems to treat the control syntax alternative syntax as a ternary condition.

In pass 1:

Generic.WhiteSpace.ScopeIndent:1530 replaced token 48 (T_INLINE_HTML on line 7) "······<?php·" => "\t\t\t<?php·"
...
=> Changeset started by Squiz.PHP.EmbeddedPhp:108
		Q: Squiz.PHP.EmbeddedPhp:109 replaced token 49 (T_OPEN_TAG on line 7) "<?php·" => "<?php·\n"
		Q: Squiz.PHP.EmbeddedPhp:110 replaced token 49 (T_OPEN_TAG on line 7) "<?php·" => "<?php·\n······"
		A: Squiz.PHP.EmbeddedPhp:111 replaced token 49 (T_OPEN_TAG on line 7) "<?php·" => "<?php·\n······"
=> Changeset ended: 1 changes applied

By pass 2, the <?php isn't recognised as a T_OPEN_TAG, so it gets split up:

WordPress.WhiteSpace.DisallowInlineTabs:89 replaced token 59 (T_WHITESPACE on line 8) "·····\t\t\t<" => "···············<"
...
=> Changeset started by Squiz.WhiteSpace.OperatorSpacing:241
		Q: Squiz.WhiteSpace.OperatorSpacing:250 replaced token 59 (T_WHITESPACE on line 8) "···············<" => "·<"
		* token 59 has already been modified, skipping *
=> Changeset failed to apply
...
Squiz.WhiteSpace.OperatorSpacing:273 replaced token 60 (T_LESS_THAN on line 8) "<" => "<·"
	Squiz.WhiteSpace.OperatorSpacing:215 replaced token 61 (T_INLINE_THEN on line 8) "?" => "·?"

The ··? gets changed to ·? in Pass 3:

=> Changeset started by Squiz.ControlStructures.ControlSignature:219
		Q: Squiz.ControlStructures.ControlSignature:226 replaced token 60 (T_WHITESPACE on line 9) "···············<" => "<"
		Q: Squiz.ControlStructures.ControlSignature:229 replaced token 59 (T_COLON on line 9) ":" => ":\n"
		A: Squiz.ControlStructures.ControlSignature:230 replaced token 60 (T_WHITESPACE on line 9) "···············<" => "<"
		A: Squiz.ControlStructures.ControlSignature:230 replaced token 59 (T_COLON on line 9) ":" => ":\n"
=> Changeset ended: 2 changes applied
...
Squiz.WhiteSpace.OperatorSpacing:307 replaced token 62 (T_WHITESPACE on line 9) "··?" => "·?"
...
Squiz.WhiteSpace.OperatorSpacing:273 replaced token 63 (T_INLINE_THEN on line 9) "?" => "?·"

Pass 4:

Generic.WhiteSpace.ScopeIndent:1527 replaced token 63 (T_LESS_THAN on line 10) "<" => "\t\t\t<"

[jrfnl]

Nice debugging @GaryJones ! Are you working on this or would you like me to pick this up ? Sounds like a tokenizer bug in PHPCS, but I'd need to investigate to be sure.

[jrfnl]

Smaller code sample to reproduce this with (and still reproducible):

<?php if (get_row_layout() == 'my_block'): ?>
  <?php include get_theme_file_path(
    'my_block.php'
  ); ?>
<?php endif; ?>

[jrfnl]

Actually not a tokenizer issue, but as @giorgosbotis already surmised in #2083 (comment), a problem with the PEAR.Functions.FunctionCallSignature sniff, which removes the close tag, which then makes the code snippet invalid PHP, which is why the next PHP open tag doesn't get tokenized as an open tag for the second fixer round, which then causes incorrect fixes to be made.

 PEAR.Functions.FunctionCallSignature:396 replaced token 14 (T_CLOSE_TAG on line 1) "?>\n" => ""

[jrfnl]

Upstream PR squizlabs/PHP_CodeSniffer#3667 should fix this. Testing appreciated.

[GaryJones]

Tested by changing my composer.json to include:

"squizlabs/php_codesniffer": "dev-feature/pear-functioncallsignature-prevent-fixer-creating-parse-error as 3.9",

and:

    "repositories": [
        {
            "type": "vcs",
            "url": "https://github.com/jrfnl/php_codesniffer"
        }
    ]

And the test file now correctly gets fixed to:

<?php

if ( have_rows( 'page_block' ) ) : ?>
	<?php
	while ( have_rows( 'page_block' ) ) :
		the_row();
		?>
		<?php if ( get_row_layout() == 'my_block' ) : ?>
			<?php
			include get_theme_file_path(
				'my_block.php'
			);
			?>
					 
	<?php endif; ?>
	<?php endwhile; ?>
	<?php
endif;

[jrfnl]

Thanks @GaryJones ! As this is an upstream issue, I think we should close the issue here as there is nothing which we can do in WPCS about this. Agreed ?

[giorgosbotis]

Tested the commit as well. I can also confirm it fixes the issue and it works just fine, even with multiple similar if/else statements. Thank you @jrfnl !