strip empty starting/ending php tags #1329
Conversation
|
Thank you for working on it 👍 We'll need a test for this too to avoid regressions. |
|
@TomasVotruba where to put the test? I cannot use the regular fixture format because its important that the AST starts with a the error happens only in the 3rd and 4th phase of the rector run: I think I need some kind of end2end test |
|
There is e2e dir https://github.com/rectorphp/rector-src/tree/main/e2e You can add more subdir and register subdir to e2e workflow |
|
hey, thanks for the pointer. I just added a e2e test, but it does not reproduce the problem yet. in my example repo I can see rector running 4 times.. when doing the same with the very same files within the e2e dir, rector is only running 2 times and the error does not reproduce. my example repo: after I copied my repro data into the rector-src I get only so rector is only working in 2 phases but in my repo it works in 4? |
|
ohh I just found out, that its important to the repro that there is no newline at the end of the so my final question is: where/how to implement the assertion of the expected behaviour in the e2e test? |
|
No need to make assertion, just ensure that e2e dry run doesn't apply diff changes As you work on branch; you can update workflow to run on current with update command to something like: |
when changing the I need the view.php to look like <div>
<?php echo 1 ?>
</div>without the fix from this PR rector tries to insert some php starting/ending tags: +<?php
+
+?>
<div>
- <?php echo 1 ?>
- </div>
+ <?php
+echo 1 ?>
+
+?>
+ </div><?phpwith this PRs fix applied, rector only tries to mangle the whitespacing: <div>
- <?php echo 1 ?>
- </div>
+ <?php
+echo 1 ?>
+
+?>
+ </div>if I change the |
|
You can try change line: rector-src/.github/workflows/e2e.yaml Line 39 in adc2e37 to use existing bin/rector instead of by composer: run: ./../../bin/rector process src --dry-run --ansi |
I tried adding this change with d691594 the github actions are now running into errors, which look weird: |
|
Composer install in root rector-src seems required |
|
@samsonasik thank you for the support <3. just disabled the actual fix of the PR with 61bac73 to make sure the unit test is working as expected. when the fix is re-enabled with ac5017a we can see the unit test is working as expected final question: how to get a green build now, as we cannot change the |
|
It seems there is double ?> even with the fix |
very good point. when I run the current rector master across my input view, I already get +<?php
+
+?>
<div>
- <?php echo 1 ?>
- </div>
+ <?php
+echo 1 ?>
+
+?>
+ </div><?phpso it seems the error of adding 2 times any further hints, where this might come frome? |
|
that's probably internal php-parser bug, probably better to send failing test case there. |
|
btw: I just verified that this bug does not occur when working with plain nikic/php-parser: <?php
use PhpParser\ParserFactory;
require_once 'vendor/autoload.php';
$code = file_get_contents('view.php');
$parser = (new ParserFactory)->create(ParserFactory::PREFER_PHP7);
$stmts = $parser->parse($code);
$prettyPrinter = new PhpParser\PrettyPrinter\Standard;
$newCode = $prettyPrinter->prettyPrintFile($stmts);
var_dump($newCode);leads to $ php repro.php
string(36) "
<div>
<?php
echo 1;
?>
</div>" |
|
another investigation: after reducing the /**
* @see \Rector\Core\Tests\PhpParser\Printer\BetterStandardPrinterTest
*/
final class BetterStandardPrinter extends Standard
{
/**
* @param mixed[] $options
*/
public function __construct(
private IndentCharacterDetector $indentCharacterDetector,
private DocBlockUpdater $docBlockUpdater,
array $options = []
) {
parent::__construct($options);
}
/**
* @param Node|Node[]|null $node
*/
public function print(Node | array | null $node): string
{
if ($node === null) {
$node = [];
}
if (! is_array($node)) {
$node = [$node];
}
return $this->prettyPrint($node);
}
public function pFileWithoutNamespace(FileWithoutNamespace $fileWithoutNamespace): string
{
$content = $this->pStmts($fileWithoutNamespace->stmts, false);
return ltrim($content);
}
}the error is still reproducible on rector-master. |
|
hmm I think I am getting closer. when using |
Co-authored-by: Abdul Malik Ikhsan <samsonasik@gmail.com>
|
Thank you @staabm |
closes rectorphp/rector#6836