Plugin-php: Inline groups break prematurely

Created on 22 Jun 2018 · 14Comments · Source: prettier/plugin-php

input/output

<?php declare(strict_types=1); ?>
<html>
    <body>
        <?php
          $a = 1;
        ?>
    </body>
</html>

Expected

<?php declare(strict_types=1); ?>
<html>
    <body>
        <?php $a = 1; ?>
    </body>
</html>

cc @evilebottnawi

bug inline

Source

mgrip

All 14 comments

@mgrip also
Input:

<?php declare(strict_types=1); ?>
<html>
    <body>
        <?php $a = 1; $b = 1; ?>
    </body>
</html>

Expected:

<?php declare(strict_types=1); ?>
<html>
    <body>
        <?php 
        $a = 1;
        $b = 1; 
        ?>
    </body>
</html>

alexander-akait on 22 Jun 2018

👍1

I _think_ now that we're using the regular group() we should get that for free, but agreed need to add a test for it 👍

mgrip on 22 Jun 2018

👍1

@mgrip any ideas how we can fix it (include second example)? I have spent around 5 hours and don't find solution :disappointed:

@vjeux Maybe you can help with this? We should attach <?php and ?>, but we don't have any information about this in ast. Maybe prettier have feature to attach information to built doc (we can detach their on parser step)? Or you have other ideas?

alexander-akait on 4 Jul 2018

@evilebottnawi I don't have enough context, what exactly is the issue?

vjeux on 5 Jul 2018

@vjeux
Input:

<html>
    <body>
        <?php $a = 1; ?>
    </body>
</html>

https://astexplorer.net/#/gist/e809179f28533199e74f5fdba8034353/f2e931e931b67991cbd50634181635d193057914

We print inline as concat(["?>", "inline_content", "<?php"]), but it break group:

<html>
    <body>
        <?php
        $a = 1;
        ?>
    </body>
</html>

Expected:

<html>
    <body>
        <?php $a = 1; ?>
    </body>
</html>

Also have problem with indent inside <?php.

Input:

<div>
    <div>
        <?php
        echo 'foo';
        echo 'foo';
        ?>
    </div>
</div>

Output:

<div>
    <div>
        <?php echo 'foo';
echo 'foo';
 ?>
    </div>
</div>

Looks we should increase indent based on indent <?php

Expected:

<div>
    <div>
        <?php 
        echo 'foo';
        echo 'foo';
        ?>
    </div>
</div>

Maybe you can give some advices how we can implement this?

alexander-akait on 5 Jul 2018

Ya this is the problem I was struggling with as well 🙁

@evilebottnawi for fully nested nodes I believe we actually print them as

concat([
  "inline_content", 
  group(concat(["<?php", print(nestedNode), "?>"])),
  "inline_content"
]);

The problem is that because we print the raw inline string, I think Prettier doesn't realize there were line breaks, so if the first inline content is

<html>
    <body>

Prettier thinks thats 26 characters on this line, rather than 8. That means the groups for the nested nodes will break prematurely (thats why it works sometimes, but not all the time). Ideally we'd have a way to tell Prettier what the current line index is or something. I think the other alternative is to _not_ just print the raw inline text, and instead do this https://github.com/prettier/plugin-php/blob/master/src/printer.js#L1113 - but that breaks other things around how we print inline nodes mixed into php nodes.

mgrip on 5 Jul 2018

@mgrip

I think the other alternative is to not just print the raw inline text, and instead do this https://github.com/prettier/plugin-php/blob/master/src/printer.js#L1113 - but that breaks other things around how we print inline nodes mixed into php nodes.

It break inline (example html) in many cases, we should find way how tell prettier doesn't think thats 26 characters on this line.

Also now algorithm is very bad, i try to run this on some code bases there a lot of php + html cases and got a lot of problem :disappointed: I try to refactor and add new tests.

alexander-akait on 5 Jul 2018

👍1

What we do for comments is to split all the lines and send them one by one to the printer deliminated by hardline()

This way the indent is not messed up.

vjeux on 6 Jul 2018

@vjeux thanks for advice, need try this

/cc @mgrip

alexander-akait on 6 Jul 2018

I think thats what I was suggesting? But it breaks a lot of other inline formatting (at least the way we have it set up now)

mgrip on 7 Jul 2018

I try again solve this problem, we should refactor all logic around inline, because it is impossible read and logic very very very complex

alexander-akait on 18 Jul 2018

fine by me, I just don't know what the alternative is. Any ideas for a good approach to a refactor? I'd be worried investing a lot of time in a re-write just to end up with something similarly complex

mgrip on 18 Jul 2018

I'm looking for a solution can't solve, spend around 2 last days, apparently this does not solve :disappointed: And this does not allow me to integrate php plugin in my development environment and i do not see sense to contribute if I can not apply it :disappointed: Plugin was good idea. I'll try again tomorrow, this will be the last attempt, I have no more ideas, apparently prettier does not have enough flexibility to form such things, sorry

alexander-akait on 19 Jul 2018

😕1

Thanks for looking - I know the inline stuff gets crazy complicated...

mgrip on 20 Jul 2018

Was this page helpful?

0 / 5 - 0 ratings