rectorphp / rector

Instant Upgrades and Automated Refactoring of any PHP 5.3+ code
https://getrector.com
MIT License
8.71k stars 687 forks source link

Warning message on PHP files consisting of only comments #1157

Closed fitztrev closed 5 years ago

fitztrev commented 5 years ago

When I run the following command, I get a warning message when it found a PHP file where the contents were only comments.

$ ./vendor/bin/rector process app --level laravel58

Contents of app/Test.php:

<?php

// A single-line comment

Output:

PHP Warning:  assert(): assert($itemStartPos >= 0 && $itemEndPos >= 0) failed in
vendor/nikic/php-parser/lib/PhpParser/PrettyPrinterAbstract.php on line 753

Ref nikic/PHP-Parser#589

TomasVotruba commented 5 years ago

It looks like it's another file. Running it on the app/Tests.php with provided contents works ok.

vendor/bin/rector process app/Test.php --level laravel58
fitztrev commented 5 years ago

It seems the issue is only on PHP 7.2. Just tried on 7.3 and the warning is not there. Thanks!

TomasVotruba commented 5 years ago

It should work on any PHP. There will be some bug with one of the rules.

If you can provide failing file, we could spare re-creating the issue in the future by someone else on PHP 7.2

fitztrev commented 5 years ago

@TomasVotruba Ok, see below:

$ php -v
// PHP 7.2.4

Steps to reproduce

mkdir rector-issue
cd rector-issue
composer require rector/rector -q

echo "<?php echo 'hello world';"> FileIsOk.php
vendor/bin/rector process FileIsOk.php --level laravel58
// ^^ All good, works as expected

echo "<?php // comment here"> FileIsNotOk.php
vendor/bin/rector process FileIsNotOk.php --level laravel58
// ^^ Issue
TomasVotruba commented 5 years ago

Thank you :+1:

What rule from laravel58.yaml (in vendor/rector/config/level/laravel/laravel58.yaml) is causing this? Try commenting out all and use one by one

(I'm affraid this is not related to any rule. This might be caused by printer.)

fitztrev commented 5 years ago

Seems to be all of them -- not related to any specific rule.

TomasVotruba commented 5 years ago

Thank you. Hm, then it's related to the printer. Any clue why PHP 7.2 vs 7.3 is different? Do you have installed same version of php-parser?

fitztrev commented 5 years ago

Any clue why PHP 7.2 vs 7.3 is different?

$itemEndPos = $origArrItem->getEndTokenPos();

This is int(-1) which is why the assertion fails.

Do you have installed same version of php-parser?

Yes, same version.

TomasVotruba commented 5 years ago

On which PHP version this is -1?

$itemEndPos = $origArrItem->getEndTokenPos();

Is is possible your assert.<option> is disabled on PHP 7.3? It just seems weird the same number fails.

fitztrev commented 5 years ago

Ok, that's not it. It's actually -1 in both environments. Result for both PHP 7.2 and 7.3:

var_dump($itemEndPos); // int(-1)
var_dump($itemStartPos >= 0 && $itemEndPos >= 0); // bool(false)

I just looked at the assert options. Here's the difference:

PHP 7.2 - zend.assertions => 1 => 1 PHP 7.3 - zend.assertions => -1 => -1

I guess that explains it?

TomasVotruba commented 5 years ago

Yes :+1: I think you have assertions silenced on 7.3.

I'll enable assertions and try it locally, thanks for debugging :heart:

TomasVotruba commented 5 years ago

Closing as failing test is required and unable to replicate