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.

Sign up for GitHub

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

Jump to bottom

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

Open
ionrocks opened this issue Aug 9, 2018 · 12 comments
Open

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

ionrocks opened this issue Aug 9, 2018 · 12 comments
Labels
bug high-pri inline
Milestone
1.0 - First stabl...

Comments

@ionrocks
Copy link
Contributor

ionrocks commented Aug 9, 2018 •
edited by alexander-akait
Loading

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>
@czosel
Copy link
Collaborator

czosel commented Aug 9, 2018

We're currently changing on the printing of only-comment code blocks by manually adding new AST nodes that represent them in #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 glayzzle/php-parser#170 to fix this for good.

@alexander-akait alexander-akait mentioned this issue Aug 9, 2018
Closed
@ntotten ntotten mentioned this issue Aug 10, 2018
Closed
@alexander-akait
Copy link
Member

/cc @czosel can we solve this problem without #513? IF yes we should do this asap and do release, because it is break code and high priority (sorry for duplicate)

@czosel
Copy link
Collaborator

czosel commented Aug 10, 2018

@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
Copy link
Collaborator

czosel commented Aug 10, 2018

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

@alexander-akait
Copy link
Member

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

@nikolasmatt
Copy link
Contributor

Hello, I took a stab at this and I was able to remove the double comment and the extra closing tag. nikolasmatt@6b30d2f
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.

@claytonrcarter
Copy link
Contributor

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.

@alexander-akait
Copy link
Member

@claytonrcarter yep, it is bug, need fix

@czosel czosel added the inline label Dec 1, 2018
@alexander-akait alexander-akait mentioned this issue Dec 12, 2018
Closed
@alexander-akait alexander-akait mentioned this issue Jan 25, 2019
Closed
@alexander-akait
Copy link
Member

alexander-akait commented Feb 1, 2019 •
edited
Loading

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

@czosel
Copy link
Collaborator

czosel commented Feb 1, 2019

@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?

@alexander-akait
Copy link
Member

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.

@ichiriac ichiriac mentioned this issue Feb 16, 2019
Open
@m-mitchell
Copy link

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 #1053 instead for a resolution?

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug high-pri inline
Projects
None yet
Milestone
1.0 - First stable release
Development

No branches or pull requests
6 participants
@m-mitchell @claytonrcarter @alexander-akait @czosel @nikolasmatt @ionrocks