[Hotfix] PHP Tokenizer: improve arrow function backfill consistency

[jrfnl]

This commit implements the proposal as outlined in #2859 (comment).

The fn keyword will now only be tokenized as T_FN if it really is an arrow function, in which case it will also have the additional indexes set in the token array.

In all other cases - including when the keyword is used in a function declaration for a named function or method -, it will be tokenized as T_STRING.

Includes numerous additional unit tests.

Includes removing the changes made to the File::getDeclarationName() function and the AbstractPatternSniff in commit 37dda44 as those are no longer necessary.

Fixes #2859

Related to #2523

[gsherwood]

Thanks a lot for submitting this PR. Just a couple of notes on the tests.

I added the missing tests to the data provider and changing the method to use it correctly, but I then received failures, which is to be expected given that the token should be T_STRING and not T_FN:

1) PHP_CodeSniffer\Tests\Core\Tokenizer\BackfillFnTokenTest::testNotAnArrowFunction with data set #1 ('/* testConstantDeclaration */')
Failed to find test target token for comment string: /* testConstantDeclaration */ With token content: fn
Failed asserting that true is false.

2) PHP_CodeSniffer\Tests\Core\Tokenizer\BackfillFnTokenTest::testNotAnArrowFunction with data set #6 ('/* testNonArrowStaticConstant */')
Failed to find test target token for comment string: /* testNonArrowStaticConstant */ With token content: fn
Failed asserting that true is false.

3) PHP_CodeSniffer\Tests\Core\Tokenizer\BackfillFnTokenTest::testNotAnArrowFunction with data set #7 ('/* testNonArrowStaticConstantDeref */')
Failed to find test target token for comment string: /* testNonArrowStaticConstantDeref */ With token content: fn
Failed asserting that true is false.

As of now, I can remove the test entirely , or replace the fake arrow function with a real one, and the tests still pass. This may require a new method to ensure a T_FN token is not found for these text cases.

[jrfnl]

Saw your remarks and was just fixing up the PR, but I see you've now already merged it.

I added the missing tests to the data provider and changing the method to use it correctly, but I then received failures, which is to be expected given that the token should be T_STRING and not T_FN:

Actually, that wasn't the problem. The test was already looking for both token types. The problem was that the $tokenContent was hard-coded as lowercase fn, while the tests related to constants were using uppercase,

This may require a new method to ensure a T_FN token is not found for these text cases.

That is already being tested in the testNotAnArrowFunction() method.

Re the:

This new test was added but is not checked anywhere. Should it be added to dataNotAnArrowFunction()?

Sorry about that. I was working on both PHPCS as well as PHPCSUtils at the same time and must have not kept them well enough in sync.

I'd be happy to send in the fixes I just committed locally, but as you already merged this now, I presume that's no longer needed ?

[gsherwood]

I'd be happy to send in the fixes I just committed locally, but as you already merged this now, I presume that's no longer needed ?

Sorry, was in the middle of a few things and pushed this without thinking. Have corrected the tests as per your advice and all is working well now. Thanks.

[jrfnl]

@gsherwood Looks like we did the same thing. I saw the tests hadn't been edited and just pulled #2863. I suppose that can be closed now ?

[gsherwood]

Looks like we did the same thing

I cheated and just made the code lowercase :)

[jrfnl]

Looks like we solved it slightly differently. Before I do anything else - should I update/rebase #2863 ? Or do you want to leave it as-is now ?

[gsherwood]

Or do you want to leave it as-is now ?

I'm happy to leave it if you are

[jrfnl]

Looks like we did the same thing

I cheated and just made the code lowercase :)

I just saw. And yes, that's cheating 😝

[jrfnl]

Or do you want to leave it as-is now ?

I'm happy to leave it if you are

As long as it's being tested we should be good, though the uppercase did add an extra safeguard to the test of course.

[gsherwood]

As long as it's being tested we should be good, though the uppercase did add an extra safeguard to the test of course.

I guess it technically needs both lower and upper (and mixed?) to account for different ways someone might define a constant, but I don't actually think that's going to impact the code here.

But your fix is still better and more flexible, so if you take the time to rebase it, I'll merge it in and add additional lower/mixed tests for it.

[jrfnl]

@gsherwood Ok, will do.

[jrfnl]

@gsherwood PR has been updated.

[jrfnl]

@gsherwood Just a thought: what with this change to how arrow functions are now handled/tokenized, I think it may now be safe to add them to the Tokens::$scopeOpeners and the Tokens::$parenthesisOpeners arrays. What do you think ?