Description

Recreation and further iteration on upstream PR squizlabs/PHP_CodeSniffer#3773 from @fredden:

Tokenizer/PHP: add some extra tests for the readonly keyword backfill

Includes some minor tweaks to pre-existing tests.

Fix mis-identification of 'readonly' keyword

PHP 8.2 introduces disjunctive normal form types, which use parentheses, which invalidates the previous "special casing" for function/method declarations and calls using the readonly keyword.

This commit fixes this.

Note: this does not (yet) add support for DNF types to the tokenizer or anywhere else in PHPCS, it only fixes the tokenization of readonly.

Includes additional tests.

Ref: php/php-src@08b7539

Co-authored-by: Dan Wallis dan@wallis.nz

Suggested changelog entry

Tokenizer/PHP: fixed the tokenization of the readonly keyword when used in combination with PHP 8.2 disjunctive normal types

Original PR description and further discussion:

@fredden wrote:

The test suite for this package is failing on PHP 8.2. This seems to be due to a mis-identification of the readonly keyword when used as a function name. For example, https://github.com/squizlabs/PHP_CodeSniffer/actions/runs/4318800044/jobs/7537540753.

This pull request fixes that test failure.

I have tested this with PHP version 8.2.3. I also noticed that the test case "testReadonlyUsedAsFunctionCallWithSpaceBetweenKeywordAndParens" seems to have been in the wrong category within the test suite.

@fredden wrote:

Using the following test code on https://onlinephp.io/

<?php

function readonly() {
	print "yes\n";
}

readonly();
readonly ();
readonly /* comment */ ();

I get expected results on PHP versions 8.2.3, 8.0.28, 7.4.33, 7.3.33, 7.2.34, 7.1.33, 7.0.33, 5.6.40, 5.5.38, 5.4.45, 5.3.29, 5.2.17, 5.1.6, 5.0.5, 4.4.9, 4.3.11, 4.2.3, 4.1.2, 4.0.6

However, all versions of PHP 8.1.x produce a parse error on the last line with this code sample.

Do we need to add any special handling for PHP 8.1.x to this readonly detection?

@fredden wrote:

@jrfnl as far as I know, this change should make all the automated tests pass. I expect that may help with the triage process for other pull requests. I don't know where this pull request fits in the list of what to review. I'm highlighting this to hopefully help, but please feel free to ignore this comment.

@jrfnl wrote:

@fredden and me looked at this PR in detail together and this needs more work.

Findings:

• The current fix will break on PHP 8.2 DNF.
• More tests needs to be added.

Basically, we need to make sure that all tests which are included in this PHP Core commit are also included in the PHPCS Tokenizer BackfillReadonlyTest file and that those tests pass correctly: php/php-src@08b7539

Also, in the original implementation of the backfill, the situation where a comment would be between the readonly and the ( for function declarations/calls was not taken into account.
This is not handled in PHP Core, however, the PHPCS token stream should still be consistent for this as it will otherwise break sniffs which specifically look for the T_READONLY token.

As for DNF: that doesn't need to be handled in this PR, but at the very least, the readonly keyword tokenization should not break on it, while it currently would.

Related: in squizlabs/PHP_CodeSniffer#3667 there are some comments/analysis about the change in the readonly tokenization in PHP Core and how it relates to DNF.

I look forward to a next iteration on this PR.

@jrfnl wrote:

For reference - these are some older PRs related to the tokenization backfill for readonly:

• PHP 8.1: Added support for readonly keyword squizlabs/PHP_CodeSniffer#3480
• PHP 8.1 | Tokenizer/PHP: readonly bug fix squizlabs/PHP_CodeSniffer#3507
• PHP 8.1: Resolve readonly earlier to fix some bugs squizlabs/PHP_CodeSniffer#3584
• And loosely related: Improved tokenizing of context sensitive keywords squizlabs/PHP_CodeSniffer#3484