PHP 8.0 | New `PHPCompatibility.Syntax.InterpolatedStringDereferencing` sniff by elazar · Pull Request #1242 · PHPCompatibility/PHPCompatibility

@elazar Hi Matthew! Awesome to see your first PHPCompatibility sniff !

As I told you when we spoke before, I'm a critical reviewer, so please don't take the dissecting below personally.

Heredocs

Based on https://3v4l.org/VdSfi, it looks like the PHP change does not apply to heredoc syntax.

And based on https://3v4l.org/tlYsX, dereferencing of heredocs without interpolated variables is not supported either.

That means that all sniff code + tests related to heredocs can be removed.

Pro-tip: always run all test code through a tool like 3v4l to verify that the interpretation of the PHP change is correct (and yes, I do this myself as well for most sniffs and often use 3v4l extensively to figure out what has actually changed as the descriptions in the PHP RFCs/changelogs can be quite obscure).

Multi-line text strings

Next up: multi-line strings are not handled by the sniff.

$a = "foo
$bar
more foo"->baz;

The above example would currently not give an error, while it is valid PHP and should throw an error if PHP < 8.0 should be supported by the codebase being examined.

Adding the token code for the current $stackPtr to the $skipTokens array would solve this, however, you will then run into the next issue:

$a = "foo $lines
$bar
more foo"[0];

If the current token code would be added to $skipTokens, the above code sample would now generate two errors, instead of one.

You may want to have a look at the PHPCSUtils TextStrings::getCompleteTextString() method.

Using that method would allow you to check for interpolated variables in a complete text string in one go, instead of line by line.

You'd still need to use something like $phpcsFile->findNext(\T_DOUBLE_QUOTED_STRING, ($stackPtr + 1), null, true) to find the next token after the end of the double quoted string.
Combine that with Tokens::$emptyTokens to get the next non-empty token after the text string.

You'll probably also want to return the text string "end pointer" when bowing out for "no issue" and at the end of the function. This will make sure that PHPCS will not call the sniff again until it reaches that pointer and will prevent duplicate errors being thrown for multi-line text strings with interpolated variables on multiple lines and/or unnecessary logic being executed for multi-line text strings.

As per the documentation of the Sniff interface:

Optionally return a stack pointer. The sniff will not be called again on the current file until the returned stack pointer is reached. Return (count($tokens) + 1) to skip the rest of the file.

Also see: https://pear.php.net/package/PHP_CodeSniffer/docs/3.5.0/apidoc/PHP_CodeSniffer/Sniff.html#methodprocess

Little aside (not relevant anymore for this sniff, but may be useful for the future):
To get to the text string "end" token for heredocs, you can find the T_END_HEREDOC pointer in the scope_closer array index of the T_START_HEREDOC token.

Minor other notes

  • Feel free to use blank lines liberally in the test case file to make it easier to distinguish the different test cases.
    Might not be that relevant anymore once the heredocs are removed, but I, for one, prefer readability of the test code over compactness.
  • We've recently made an effort to improve the documentation of the codebase. The test method documentation is the last bit which still needs further improvement, but it would be great if new tests being added already contained descriptive documentation and proper punctuation.
    I've left inline suggestions to fix this up for the test file for this sniff.
  • Once the sniff is fully ready, you may want to squash the individual commits into one commit so as to keep the codebase history clean.