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

Does not work with readonly classes introduced in PHP 8.2 #3727

Closed
siketyan opened this issue Dec 10, 2022 · 15 comments · Fixed by #3728
Closed

Does not work with readonly classes introduced in PHP 8.2 #3727

siketyan opened this issue Dec 10, 2022 · 15 comments · Fixed by #3728

Comments

@siketyan
Copy link

siketyan commented Dec 10, 2022

May be closed by #3686 #3728

Describe the bug
In PHP 8.2, "readonly class" feature was introduced.
Using readonly classes, we can shorten readonly property declarations as follows:

class Foo {
    public function __construct(
        private readonly string $foo,
        private readonly int $bar,
    ) {
    }
}

equals to:

readonly class Foo {
    public function __construct(
        private string $foo,
        private string $bar,
    ) {
    }
}

However, the latest version of phpcs v3.7.1 does not work on the classes which are marked as "readonly". (The phpcs error output is in "To reproduce" section)

Code sample

readonly class Foo {
    public function __construct(
        private string $foo,
        private string $bar,
    ) {
    }
}

Custom ruleset
N/A

To reproduce
Steps to reproduce the behavior:

  1. Run phpcs with the default ruleset, with the code sample above.
FILE: /project/Foo.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 1 | WARNING | A file should declare new symbols (classes, functions,
   |         | constants, etc.) and cause no other side effects, or
   |         | it should execute logic with side effects, but should
   |         | not do both. The first symbol is defined on line 13
   |         | and the first side effect is on line 13.
   |         | (PSR1.Files.SideEffects.FoundWithSymbols)
----------------------------------------------------------------------

Expected behavior
A clear and concise description of what you expected to happen.

Versions (please complete the following information):

  • OS: macOS Ventura 13.0.1 (Apple M1 Max; aarch64-apple-darwin)
  • PHP: 8.2.0
  • PHPCS: 3.7.1
  • Standard: PSR12

Additional context
N/A

@jrfnl
Copy link
Contributor

jrfnl commented Dec 10, 2022

Thanks for reporting this.

PR #3686 would not fix this, though I have a number of follow-up commits waiting on #3686 to be merged to add support for readonly classes to various sniffs. However, looks like I missed the PSR1.Files.SideEffects sniff and as the fix for that sniff does not depend on #3686, I have pulled PR #3728 now to fix this.

@siketyan
Copy link
Author

@jrfnl
Thank you for the update and your work!

asispts added a commit to asispts/ptscs that referenced this issue Jan 22, 2023
There is a bug when dealing with readonly class. See squizlabs/PHP_CodeSniffer#3727

This commit will temporary exclude PSR1.Files.SideEffects.FoundWithSymbols
@gsherwood gsherwood added this to the 3.8.0 milestone Feb 22, 2023
spaze added a commit to spaze/michalspacek.cz that referenced this issue Apr 7, 2023
This is to satisfy some scanners looking for one. I mean, it's Easter, after all.

The `FourOhFourButFound` class could be marked as `readonly` but the code sniffer doesn't yet support readonly classes, see squizlabs/PHP_CodeSniffer#3727.
@still-dreaming-1
Copy link

still-dreaming-1 commented Apr 30, 2023

PHP 8.2 beta came out July 21, 2022. The first RC came out on Sep 1. The official stable release was on Dec 8, 2022. And here we are, April 30, 2023, and we still have to choose between dropping PHP_CodeSniffer and actually making use of the readonly classes feature.

@jrfnl
Copy link
Contributor

jrfnl commented Apr 30, 2023

@still-dreaming-1 Have you tried contributing ?

@iquito
Copy link

iquito commented Apr 30, 2023

@jrfnl Would contributing help though? There are so many pull requests already waiting (you seem to have already fixed this issue for quite some time, for example, thanks for that!) and no avenue to help out or indication what would help. I would donate some money for the project or for a maintainer (as I do for other open source projects), but there is no information about that either. It does seem that this project is somehow stuck.

@jrfnl
Copy link
Contributor

jrfnl commented Apr 30, 2023

@iquito We're working on getting it unstuck, but comments like the one I responded to are not helpful and highly demotivating.

@jrfnl
Copy link
Contributor

jrfnl commented May 3, 2023

Another readonly classes issue was reported in #3799 (comment).
A fix for that one has been pulled in #3816

@gsherwood
Copy link
Member

I've merged the fix. Thanks for the bug report.

@KorvinSzanto
Copy link

@gsherwood Is this something that you could tag soon? It'd be nice to stop disabling the busted rule

@kasperhendriks
Copy link

@jrfnl @gsherwood any ETA on getting this out?

@kallesommernielsen
Copy link

Just ran into this myself; any chance for a new release of PHPCS? Thank you for considering!

@alamirault
Copy link

How can we help to tag this hotfix ?

@jrfnl
Copy link
Contributor

jrfnl commented Sep 25, 2023

@alamirault There's nothing anyone can do, but wait patiently for @gsherwood to have some time again. Also see: #3861

@kczx3
Copy link

kczx3 commented Oct 20, 2023

Tough to find time for development when you're a CTO now 😉 It certainly aligns with his activity on GitHub. Hopefully he hasn't forgotten his roots!
image

@jrfnl
Copy link
Contributor

jrfnl commented Dec 8, 2023

FYI: the fix for this issue 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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants