"Unexpected end of token stream" with nested if in alternative PHP syntax

[mk-mxp]

• PHPMD version: 2.10.1
• PHP Version: 7.3.27
• Installation type: composer
• Operating System / Distribution & Version: Ubuntu 20.04 LTS

Current Behavior

phpmd stops with Unexpected end of token stream in file ...

Expected Behavior

Successfully parses the file

Steps To Reproduce:

Create a file test.php with this valid PHP code:

<?php if (true): ?>
  <?php if (true): ?>
  <?php endif ?>
<?php else: ?>
<?php endif ?>

Run vendor/bin/phpmd test.php text ruleset.xml (format and ruleset don't matter). If I add another empty <?php ?> to the end of the file, the file is checked as expected. It doesn't matter if there is any real condition in the if clauses or if there is other PHP code inside the conditional branches.

Checks before submitting

• Be sure that there isn't already an issue about this. See: Issues list
• Be sure that there isn't already a pull request about this. See: Pull requests
• I have added every step to reproduce the bug.
• If possible I added relevant code examples.
• This issue is about 1 bug and nothing more.
• The issue has a descriptive title. For example: "JSON rendering failed on Windows for filenames with space".

[kylekatarnls]

Hello 👋 properly formatted statements in PHP should end with ; which mean using <?php endif; ?> instead of <?php endif ?> and indeed this syntax would properly work with PHPMD.

It's sure we can better handle this syntax and eventually replace it with a non-fatal error like "Always end statement with a semi-colon" but I would encourage you anyway to follow this rule.

Always having a semi-colon at the end of a statement can save you trouble.

See this example:

<?php foo() ?><?php ($a == 1) ? 'ok' : 'KO' ?>

Some smart dev comes here and want to cleanup the template file removing all ?><?php because indeed in 99% in templates it's fine to remove (such as in <?php if (...) { ?>ok<?php } ?><?php else { ?>KO<?php } ?>).

But there in the example above, it's not heuristic and it won't neither gives you a syntax error as the result would be valid code:

<?php foo()($a == 1) ? 'ok' : 'KO' ?>

Let's say foo() returns an object that have both __toString() and __invoke() and both cases will run without errors but output completely different results.

So I advice to put ; after every statement even if followed by ?> to be on the safe side.

[mk-mxp]

@kylekatarnls As much as I do stand with You for ; being best practice and all, PHP docs say it's allowed to omit ; just before the closing ?>. And as it is allowed PHP itself will never complain about it.

I would appreciate if PHPMD contained a rule to find those places where ; is missing, and not just error out completely and give an unhelpful message.

[kylekatarnls]

and not just error out completely and give an unhelpful message

As I said, we should warn with "Always end statement with a semi-colon", that's why I sorted this issue as a bug to be fixed.

I'm pretty aware the code is valid for PHP. But everything PHP allow you to do is not the wisest or safest choice, that's actually the whole point of using code smell detector like PHPMD. This may explain why we never had request to support this syntax before.

PHPMD has its own parser: PDepend, so syntax rules have to be explicitly implemented in it to be supported.

We would gladly merge a PR for this if anyone wants to give it a try.

[rafael-neris]

@kylekatarnls Hello Kyle, I would like to help in this issue, can you help me giving detailing of what we need to do in this issue, please?

[kylekatarnls]

Hello @rafael-neris thanks for investigating this.

I prepared a branch you can clone with a test that reproduce this issue: https://github.com/pdepend/pdepend/tree/fix/optional-semi-colon-before-php-end-tag

After installing dependencies locally with composer update --ignore-platform-reqs you can run this test alone using:

./vendor/bin/phpunit --filter "/PHPParserGenericTest::testOptionalSemiColon/" --no-coverage

(.\vendor\bin\phpunit on Windows)

It will gives the full stack trace of the error:

src/main/php/PDepend/Source/Language/PHP/AbstractPHPParser.php:7368
src/main/php/PDepend/Source/Language/PHP/AbstractPHPParser.php:6175
src/main/php/PDepend/Source/Language/PHP/AbstractPHPParser.php:3249
src/main/php/PDepend/Source/Language/PHP/AbstractPHPParser.php:6161
src/main/php/PDepend/Source/Language/PHP/AbstractPHPParser.php:2586
src/main/php/PDepend/Source/Language/PHP/AbstractPHPParser.php:2570
src/main/php/PDepend/Source/Language/PHP/AbstractPHPParser.php:2532
src/main/php/PDepend/Source/Language/PHP/AbstractPHPParser.php:3480
src/main/php/PDepend/Source/Language/PHP/AbstractPHPParser.php:6066
src/main/php/PDepend/Source/Language/PHP/AbstractPHPParser.php:397
src/test/php/PDepend/AbstractTest.php:929
src/test/php/PDepend/AbstractTest.php:853
src/test/php/PDepend/Source/Language/PHP/PHPParserGenericTest.php:778
src/test/php/PDepend/AbstractTest.php:142

From that stack, I think parseStatementTermination() is the point to inspect. When we check for semicolon:

if ($this->tokenizer->peek() === Tokens::T_SEMICOLON) {
    $this->consumeToken(Tokens::T_SEMICOLON);
} else {
    $this->parseNonePhpCode();
}

Here we should also check if next token is a Tokens::T_CLOSE_TAG and if so consider the statement is also terminated as if we would have Tokens::T_SEMICOLON then Tokens::T_CLOSE_TAG.

Thanks.

[kylekatarnls]

Hello @rafael-neris, I started a fix on #553 (still the same branch: fix/optional-semi-colon-before-php-end-tag) and it might be more difficult than I expected (it broke other tests) but you can still work on it if you want, I would close my PR if you create a new one that pass tests.