plugin-php icon indicating copy to clipboard operation
plugin-php copied to clipboard

Invalid output caused by combo of if statement, inline node, and comment

Open ionrocks opened this issue 7 years ago • 12 comments

Input:

<?php
if (true) {
    ?>inline<?php
    // comment
}

Output:

<?php
if (true) { ?>inline<?php
    // comment
    // comment
    ?>}

Note the extra closing tag that has been added, which makes the code invalid, and that the comment is duplicated, which is unexpected.

If you comment out this line, it doesn't print out the closing tag that breaks the code, but it also removes the closing tag from the test file inline3.php.

If you comment out the line above, prettier throws the following error, I think about inline3.php again.

Comment "// test" was not printed. Please report this error!

I've tried to debug, but the printing system is quite large. I'll still keep looking, but might as well put it out to the community too!

Problem aslo with

<!DOCTYPE html>
<html lang="de">
<head>
    <?php /* ups */
    if (1==1)
    {
        echo 'FOO';
    } ?>
</head>
<body>
</body>
</html>

ionrocks avatar Aug 09 '18 04:08 ionrocks

We're currently changing on the printing of only-comment code blocks by manually adding new AST nodes that represent them in https://github.com/prettier/plugin-php/pull/513. On that branch your example is formatted as

<?php
if (true) { ?>inline<?php }
    // comment

which is not pretty, but at least seems to be valid PHP. Ultimately, I think we'll need the fix suggested in https://github.com/glayzzle/php-parser/issues/170 to fix this for good.

czosel avatar Aug 09 '18 06:08 czosel

/cc @czosel can we solve this problem without https://github.com/prettier/plugin-php/pull/513? IF yes we should do this asap and do release, because it is break code and high priority (sorry for duplicate)

alexander-akait avatar Aug 10 '18 13:08 alexander-akait

@evilebottnawi I’d recommend to merge #513, because it makes comment handling much simpler on the printing side. Let me know if there’s anything you’d still like to change in #513.

czosel avatar Aug 10 '18 15:08 czosel

@evilebottnawi The only way to fix this seems to be to add support for "only-comment" AST nodes in the parser. Do you agree?

czosel avatar Aug 10 '18 19:08 czosel

@czosel need investigate, feel free to experiment. Inline nodes is the most difficult part, comments also, together is incredibly difficult.

alexander-akait avatar Aug 11 '18 14:08 alexander-akait

Hello, I took a stab at this and I was able to remove the double comment and the extra closing tag. https://github.com/nikolasmatt/plugin-php/commit/6b30d2f0c6ea05979c000285ad301bbd2b3c581f I've got it to the point where the output is

<?php
if (true) { ?>inline<?php
    // comment
   }

This is ugly but non-breaking.

I've hit a wall on how to make the block break appropriately. Any guidance would be much appreciated.

nikolasmatt avatar Oct 14 '18 01:10 nikolasmatt

I think I ran into this today, as well. Here is the example that bit me. (Sorry if this is a different problem; I can open a new issue if so.)

Input

<?php $totalRows_rsUsers = 0; ?>
<?php if ($totalRows_rsUsers == 0) { // Show if recordset empty ?>
No results.
<?php } else { ?>
Found <?php echo $totalRows_rsUsers; ?> users.
<?php } ?>

Output (note the extra <?php tag before the comment)

<?php $totalRows_rsUsers = 0; ?>
<?php if ($totalRows_rsUsers == 0) {<?php
    // Show if recordset empty
    ?>
No results.
<?php } else { ?>
Found <?php echo $totalRows_rsUsers; ?> users.
<?php } ?>

Which of course produces an error:

$ php test3.php
PHP Parse error:  syntax error, unexpected '<' in /Users/crcarter/Software/Other/prettier/test3.php on line 2

Parse error: syntax error, unexpected '<' in /Users/crcarter/Software/Other/prettier/test3.php on line 2

This was with Prettier 1.15.2 and plugin-php 0.9.0.

claytonrcarter avatar Nov 26 '18 20:11 claytonrcarter

@claytonrcarter yep, it is bug, need fix

alexander-akait avatar Nov 26 '18 20:11 alexander-akait

@czosel I will spend the whole weekend to come up with an algorithm for the best way to print them, but i have some examples where don't have idea how better print.

Examples:

<?php if (
    $veryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLongVariable ||
    $veryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLongVariable
) { ?>
  <div>Test</div>
<?php } ?>

Oh my good:

<?php if (
    $veryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLongVariable ||
    $veryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLongVariable
) {
  if ($veryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLongVariable) { ?>
  <div>Test</div>
<?php } ?> test <?php } ?>

Any ideas, other examples?

It is last what we should do and we can release stable version, support php7.3 we can implement in 1.1 version.

alexander-akait avatar Feb 01 '19 20:02 alexander-akait

@evilebottnawi My feeling is that the whole inline topic is not something that can be solved in a "perfect" way - there is just too much freedom in mixing PHP with HTML. I'd suggest approaching the issue like this:

  1. Introduce AST node for <?php and ?>
  2. Try to format simple/typical examples as good as possible
  3. Wait and collect edge cases
  4. Decide which edge cases can be fixed

What do you think?

czosel avatar Feb 01 '19 20:02 czosel

Introduce AST node for

  1. It is decrease code of lines but not solve a lot of problems

Try to format simple/typical examples as good as possible

We already do this in many cases.

Wait and collect edge cases

Yep

Decide which edge cases can be fixed

Some edge cases already fixed, but i want implement algorithm what cover 90%-95% cases. I have some ideas. Printing inline is difficult only for control structure, in other places it is easy and/or easy to fix.

alexander-akait avatar Feb 01 '19 21:02 alexander-akait

I'm running into the same issue on the latest version (v0.10.2). Is there a recommended fix for this issue? I tried out the https://github.com/prettier/plugin-php/tree/issue-517 branch and it mostly works, except that running prettier --write followed by prettier -l returns the list of files that were affected by this bug on master.

Should we be watching https://github.com/prettier/plugin-php/issues/1053 instead for a resolution?

Thanks!

m-mitchell avatar May 24 '19 23:05 m-mitchell