phpcbf breaks php opening and closing tags adding spaces #2083

giorgosbotis · 2022-09-16T07:07:33Z

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.

The text was updated successfully, but these errors were encountered:

dingo-d · 2022-09-16T11:57:38Z

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 · 2022-09-17T08:09:47Z

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 · 2022-09-17T11:27:19Z

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 · 2022-09-17T15:13:33Z

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 · 2022-09-17T15:17:50Z

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 · 2022-09-17T15:25:16Z

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 · 2022-09-17T17:16:44Z

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

GaryJones · 2022-09-17T22:54:30Z

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 · 2022-09-17T23:24:13Z

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 · 2022-09-18T07:56:57Z

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 !

jrfnl · 2022-09-18T09:44:24Z

Thanks for confirming @giorgosbotis ! In that case, I'm closing the issue and we'll just have to wait for the PHPCS version in which my fix will be merged, to be released.

jrfnl · 2022-09-19T09:44:50Z

Apparently the OP also reported this issue upstream with a different description, so now my PR can be binned....
@giorgosbotis Next time, please link the issues together, so there aren't two people from two different repos working on the same fix.

giorgosbotis · 2022-09-19T10:15:51Z

@jrfnl I'm sorry but it was not the same issue.

The issue I reported on phpcs board was a runtime error I got using only PEAR standard. It had nothing to do with breaking php tags and it was reproducible without Wordpress standards.

jrfnl · 2022-09-19T10:18:55Z

@giorgosbotis Except they were the same issue.... See the fix committed and the fix I pulled. They are effectively the same.

giorgosbotis · 2022-09-19T10:23:46Z

@jrfnl If I knew I wouldn't report two different issues on two different boards, and I would probably propose a fix. I think it's quite obvious I had no idea those issues were connected.

jrfnl · 2022-09-19T13:17:52Z

@jrfnl If I knew I wouldn't report two different issues on two different boards, and I would probably propose a fix. I think it's quite obvious I had no idea those issues were connected.

I understand it can be hard to determine whether things are the same issue, but if it involves the same code snippet, chances are high that the issues are related, if not the same.
For a next time, at least please afford us the courtesy of linking the issues together as "possibly related" to safe us wasting time on something which was already being worked on.

GaryJones added the Type: Bug label Sep 17, 2022

jrfnl mentioned this issue Sep 17, 2022

PEAR/FunctionCallSignature: minor tweaks + extra test squizlabs/PHP_CodeSniffer#3667

Closed

jrfnl added Upstream: PHPCS Status: Awaiting feedback and removed Type: Bug labels Sep 17, 2022

jrfnl closed this as completed Sep 18, 2022

jrfnl removed the Status: Awaiting feedback label Sep 18, 2022

jrfnl mentioned this issue Sep 19, 2022

PEAR.Functions.FunctionCallSignature incorrect indent fix when checking mixed HTML/PHP files squizlabs/PHP_CodeSniffer#3666

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

phpcbf breaks php opening and closing tags adding spaces #2083

phpcbf breaks php opening and closing tags adding spaces #2083

giorgosbotis commented Sep 16, 2022

dingo-d commented Sep 16, 2022

giorgosbotis commented Sep 17, 2022

GaryJones commented Sep 17, 2022

jrfnl commented Sep 17, 2022

jrfnl commented Sep 17, 2022

jrfnl commented Sep 17, 2022 •

edited

Loading

jrfnl commented Sep 17, 2022

GaryJones commented Sep 17, 2022

jrfnl commented Sep 17, 2022 •

edited

Loading

giorgosbotis commented Sep 18, 2022

jrfnl commented Sep 18, 2022 •

edited

Loading

jrfnl commented Sep 19, 2022

giorgosbotis commented Sep 19, 2022

jrfnl commented Sep 19, 2022

giorgosbotis commented Sep 19, 2022

jrfnl commented Sep 19, 2022

phpcbf breaks php opening and closing tags adding spaces #2083

phpcbf breaks php opening and closing tags adding spaces #2083

Comments

giorgosbotis commented Sep 16, 2022

Bug Description

Minimal Code Snippet

Error Code

Custom ruleset

Environment

Additional Context (optional)

Tested Against develop branch?

dingo-d commented Sep 16, 2022

giorgosbotis commented Sep 17, 2022

GaryJones commented Sep 17, 2022

jrfnl commented Sep 17, 2022

jrfnl commented Sep 17, 2022

jrfnl commented Sep 17, 2022 • edited Loading

jrfnl commented Sep 17, 2022

GaryJones commented Sep 17, 2022

jrfnl commented Sep 17, 2022 • edited Loading

giorgosbotis commented Sep 18, 2022

jrfnl commented Sep 18, 2022 • edited Loading

jrfnl commented Sep 19, 2022

giorgosbotis commented Sep 19, 2022

jrfnl commented Sep 19, 2022

giorgosbotis commented Sep 19, 2022

jrfnl commented Sep 19, 2022

Tested Against `develop` branch?

jrfnl commented Sep 17, 2022 •

edited

Loading

jrfnl commented Sep 17, 2022 •

edited

Loading

jrfnl commented Sep 18, 2022 •

edited

Loading