Squiz/NonExecutableCode: fix false positives when code goes in and out of PHP/HTML

[fredden]

The following code snippet is currently flagging as Squiz.PHP.NonExecutableCode.Unreachable, but probably shouldn't.

<?php if (true): ?>
    <?php foreach ([] as $item): ?>
        <?php continue; ?>
    <?php endforeach; ?>
<?php endif; ?>

This pull request adds a check to avoid this snippet from being flagged as problematic.

[jrfnl]

Not done a full review yet, but still noticed some things when taking a second look which I believe need to be addressed before this PR can progress.

Just thinking: PHP will automatically add a semi-colon if one is missing before a PHP close tag. Will this fix work correctly with code missing the semi-colon ?

<!-- no problem here -->
<?php if (true): ?>
    <?php foreach ([] as $item): ?>
        <?php continue ?>
    <?php endforeach; ?>
<?php endif; ?>

<!-- should detect an error here -->
<?php if (true): ?>
    <?php foreach ([] as $item): ?>
        <?php continue ?>
        <div>non-executable</div>
    <?php endforeach; ?>
<?php endif; ?>

[jrfnl]

What about if the "re-open tag" is a PHP open tag with echo <?= ?

Might be good to add some tests for that as well.

[fredden]

Yes, the sniff already handles this case correctly. I've added a test (in b055898) to confirm.

[jrfnl]

I've just tested this, but as far as I can see, the sniff does not handle this correctly, but the test used for this is flawed.

Try this:

<!-- no problem here -->
<?php if (true): ?>
    <?php foreach ([] as $item): ?>
        <?php continue; ?>
        <?= ?>
    <?php endforeach; ?>
<?php endif; ?>

<!-- should detect an error here -->
<?php if (true): ?>
    <?php foreach ([] as $item): ?>
        <?php continue; ?>
        <?=
        'unreachable'
        ?>
    <?php endforeach; ?>
<?php endif; ?>

While an empty <?= ?> isn't very useful, that's not the concern of the sniff.
If the <?= was handled correctly, the first and only error for the above code would be on the line containing the 'unreachable'. As things are, both lines containing a <?= also throw errors.

[fredden]

Thanks for the feedback. I've looked into both of these suggested test cases.

The sniff will complain about any line of code which is unreachable. (See

PHP_CodeSniffer/src/Standards/Squiz/Tests/PHP/NonExecutableCodeUnitTest.2.inc

Lines 1 to 12 in d148feb

	<?php
	exit();
	// Errors are thrown from here down from the exit() above.
	foreach ($vars as $var) {
	if ($something === TRUE) {
	break;
	break;
	}
	}

and

PHP_CodeSniffer/src/Standards/Squiz/Tests/PHP/NonExecutableCodeUnitTest.php

Lines 89 to 92 in d148feb

	7 => 1,
	8 => 1,
	9 => 1,
	10 => 2,

- one complaint for each of lines 7-10 from the exit on line 3, and an additional complaint for line 10 from the break on line 9.)

I didn't see an existing test showing the expected behaviour when there are multiple unreachable statements on one line. I've added a test showing the existing behaviour in 34ba861.

Looking at these two suggested sniffs:

• <?= ?> is a parse error, so I don't think it needs any special treatment in this sniff. https://3v4l.org/MXtbr
• One statement split across multiple lines also isn't covered by existing tests; I've added a test showing the existing behaviour in 2067e7c.
• We could add some logic to detect something like <?= ''; ?> which isn't a parse error, and will produce no output, but such a line of code seems like something that should be flagged to developers for review, so I'm comfortable with this sniff reporting that as unreachable. I don't think it's feasible for this sniff to detect output of a variable which happens to be empty and alert differently. A human can review the line of code in that case and make a sensible judgement.

@jrfnl do these cover the concerns raised here, or would you like me to add these specific tests as well?

[jrfnl]

Just to confirm - the tests added in the 2 case file just document existing behaviour. The behaviour regarding these is not changed by this PR. Correct ?

[fredden]

Yes, the changes in src/Standards/Squiz/Tests/PHP/NonExecutableCodeUnitTest.2.inc (and the corresponding additions in src/Standards/Squiz/Tests/PHP/NonExecutableCodeUnitTest.php) are documenting existing behaviour. I can move these to a separate pull request if you would prefer.

[fredden]

Will this fix work correctly with code missing the semi-colon ?

No, the sniff has a false-negative in this case. While technically out of scope for this pull request, I will investigate adding code to solve this.

Edit: Fixed in 01410b4

[jrfnl]

@fredden Reviewed again and left some comments.

While technically out of scope for this pull request, I will investigate adding code to solve this.

Thanks for looking into this. In my opinion, it's not actually out of scope as the situation you are trying to address - code moving in and out of PHP/HTML - is the only time this comes into play.

[jrfnl]

I've just tested this, but as far as I can see, the sniff does not handle this correctly, but the test used for this is flawed.

Try this:

<!-- no problem here -->
<?php if (true): ?>
    <?php foreach ([] as $item): ?>
        <?php continue; ?>
        <?= ?>
    <?php endforeach; ?>
<?php endif; ?>

<!-- should detect an error here -->
<?php if (true): ?>
    <?php foreach ([] as $item): ?>
        <?php continue; ?>
        <?=
        'unreachable'
        ?>
    <?php endforeach; ?>
<?php endif; ?>

While an empty <?= ?> isn't very useful, that's not the concern of the sniff.
If the <?= was handled correctly, the first and only error for the above code would be on the line containing the 'unreachable'. As things are, both lines containing a <?= also throw errors.

[jrfnl]

@fredden Sorry it took me a while to get back to this PR. I've done some extensive testing with it now and IMO this is good to go. Thank you for your work on this!

[jrfnl]

FYI: this fix is included in today's PHP_CodeSniffer 3.8.0 release.

As per #3932, development on PHP_CodeSniffer will continue in the PHPCSStandards/PHP_CodeSniffer repository. If you want to stay informed, you may want to start "watching" that repo (or watching releases from that repo).