Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

4.0 | PEAR/PSR2/FunctionCallSignature: support anonymous classes #47

Open
wants to merge 2 commits into
base: 4.0
Choose a base branch
from

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Nov 9, 2023

Description

Recreation of upstream PR squizlabs/PHP_CodeSniffer#3636 + rebase for the 4.0 branch:

The function call spacing for anonymous class instantiations was so far not checked by these or any other PHPCS native sniffs.

In my opinion, object instantiations of anonymous classes should be treated the same an object instantiations of non-anonymous classes.

The PEAR.Functions.FunctionCallSignature and the PSR2.Methods.FunctionCallSignature sniffs check the object instantiation spacing for non-anonymous classes, so seem like the logical place to also check the spacing for anonymous class object instantiations.

To add this support, the T_ANON_CLASS token has been added to the Tokens::$functionNameTokens array.

Notes:

  • As PSR12 does not specify the spacing between the class keyword and the open parenthesis (or rather is unclear about it), I am explicitly excluding anonymous classes from the "space before open parenthesis" check.
    Related: Unexpected spacing for anonymous classes with constructor arguments squizlabs/PHP_CodeSniffer#3200
  • I have verified all other uses of the Tokens::$functionNameTokens array within PHPCS.
    • The Generic.WhiteSpace.ArbitraryParenthesesSpacing sniff is not affected by the change and already contains a test to verify this.
    • The Squiz.Operators.ComparisonOperatorUsage sniff also is not affected by the change. I have added tests to confirm this in a separate commit.
  • Obviously external standards using the token array may be affected by the change, but a scan of the most popular external standards showed me that the token array is rarely used and when it is used, is mostly used incorrectly.
    The only sniff using the array, which looks to be using it correctly and which may be affected, is the WebImpressCodingStandard.WhiteSpace.ScopeIndent sniff. Whether this is positive or negative is up to @michalbundyra to determine.

Includes unit tests for both the PEAR.Functions.FunctionCallSignature and the PSR2.Methods.FunctionCallSignature sniffs .

Other notes from the original PR:

Note: this does cause some overlap with the PSR12.Classes.AnonClassDeclaration sniff in the PSR12 standard. Not sure what to do about this, though it shouldn't be a cause for fixer conflicts (as the sniffs expect the same thing).

Suggested changelog entry

  • The Tokens::$functionNameTokens array now includes the T_ANON_CLASS token.
  • PEAR + PSR2 FunctionCallSignature sniff will now also examine anonymous class instantiations with parameters.

@michalbundyra
Copy link
Contributor

hi, thanks for head-up, didn't know squizlabs is abandoned

tried with this branch and have some issues with loading rules on branch 4, getting a lot of:

Referenced sniff "Squiz.WhiteSpace.LanguageConstructSpacing" does not exist

need to check more what has changed to find out what's the problem, as release 3.7.2 from this repo works well with couple changes - just pushed branch: https://github.com/webimpress/coding-standard/tree/feat/phpcsstandards if you have a sec to advise what I need to change to prepare compatibility with v4 and verify changes from this branch.

Thanks 🙏

@jrfnl
Copy link
Member Author

jrfnl commented Nov 9, 2023

hi, thanks for head-up, didn't know squizlabs is abandoned

@michalbundyra Expect an official announcement over the next few days. For now, I'm still scrambling to get this repo up & running properly. (The news is very fresh)

tried with this branch and have some issues with loading rules on branch 4, getting a lot of:

Referenced sniff "Squiz.WhiteSpace.LanguageConstructSpacing" does not exist

need to check more what has changed to find out what's the problem, as release 3.7.2 from this repo works well with couple changes - just pushed branch: https://github.com/webimpress/coding-standard/tree/feat/phpcsstandards if you have a sec to advise what I need to change to prepare compatibility with v4 and verify changes from this branch.

The 4.0 branch has a CHANGELOG file which contains a list of all the changes which are 4-specific under "[Unreleased 4.x]".

I originally had this PR against the 3.x branch, but when recreating it now, I decided to err on the side of caution as the PR changes one of the Tokens arrays, so I'll put in 4.0. If it helps, you can test the original PR against 3.x in the Squizlabs repo. I haven't closed it (yet).

I'm hoping to release 3.8.0, including nearly all of my previously open PRs (with the exception of the 4.x specific ones) over the next few weeks.
Once I've released that, Packagist should be able to start serving from this repo.

@michalbundyra
Copy link
Contributor

@jrfnl thanks, just tested with feature/pear-psr2-psr12-functioncallsignature-check-anon-classes from [email protected]:jrfnl/PHP_CodeSniffer.git and all my tests seems to be fine. But yeah, if it changes tokenizer probably better for next major. Thanks!

PS. Do you think would be ok for me to release new minor (1.4.0) with switch to this library (and probably pin v3.8 when released)? Or I would need a new major? Thanks :)

@jrfnl
Copy link
Member Author

jrfnl commented Nov 9, 2023

@jrfnl thanks, just tested with feature/pear-psr2-psr12-functioncallsignature-check-anon-classes from [email protected]:jrfnl/PHP_CodeSniffer.git and all my tests seems to be fine. But yeah, if it changes tokenizer probably better for next major. Thanks!

Thanks for testing!

PS. Do you think would be ok for me to release new minor (1.4.0) with switch to this library (and probably pin v3.8 when released)? Or I would need a new major? Thanks :)

That should be fine as far as I can see, though I suggest only doing so after I've tagged 3.8.0 and setting the version constraints to ^3.8 to be on the safe side.

@michalbundyra
Copy link
Contributor

Cool, thanks again, I'll subscribe on the repo to be up to date.

Once it's stabilised I might be bringing couple of mine old PRs from squizlabs as well :)

@jrfnl
Copy link
Member Author

jrfnl commented Nov 9, 2023

Cool, thanks again, I'll subscribe on the repo to be up to date.

Once it's stabilised I might be bringing couple of mine old PRs from squizlabs as well :)

Sounds like a plan ;-)

@jrfnl jrfnl force-pushed the feature/pear-psr2-psr12-functioncallsignature-check-anon-classes branch from d17d642 to 7bbac11 Compare November 11, 2023 04:10
@jrfnl jrfnl changed the title PEAR/PSR2/FunctionCallSignature: support anonymous classes 4.0 | PEAR/PSR2/FunctionCallSignature: support anonymous classes Dec 2, 2023
@jrfnl jrfnl force-pushed the feature/pear-psr2-psr12-functioncallsignature-check-anon-classes branch from 7bbac11 to 1505104 Compare December 6, 2023 00:36
.. to document that the sniff also handles comparisons passed in the instantiation of an anonymous class.
The function call spacing for anonymous class instantiations was so far not checked by these or any other PHPCS native sniffs.

In my opinion, object instantiations of anonymous classes should be treated the same an object instantiations of non-anonymous classes.

The `PEAR.Functions.FunctionCallSignature` and the `PSR2.Methods.FunctionCallSignature` sniffs check the object instantiation spacing for non-anonymous classes, so seem like the logical place to also check the spacing for anonymous class object instantiations.

To add this support, the `T_ANON_CLASS` token has been added to the `Tokens::$functionNameTokens` array.

Notes:
* As PSR12 does not specify the spacing between the `class` keyword and the open parenthesis (or rather is unclear about it), I am explicitly excluding anonymous classes from the "space before open parenthesis" check.
    Related: squizlabs/PHP_CodeSniffer 3200
* I have verified all other uses of the `Tokens::$functionNameTokens` array within PHPCS.
    - The `Generic.WhiteSpace.ArbitraryParenthesesSpacing` sniff is not affected by the change and already contains a test to verify this.
    - The `Squiz.Operators.ComparisonOperatorUsage` sniff also is not affected by the change. I have added tests to confirm this in a separate commit.
* Obviously external standards using the token array _may_ be affected by the change, but a scan of the most popular external standards showed me that the token array is rarely used and when it is used, is mostly used incorrectly.
    The only sniff using the array, which looks to be using it correctly and which may be affected, is the `WebImpressCodingStandard.WhiteSpace.ScopeIndent` sniff. Whether this is positive or negative is up to michalbundyra to determine.

Includes unit tests for both the `PEAR.Functions.FunctionCallSignature` and the `PSR2.Methods.FunctionCallSignature` sniffs .
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants