PHP 8.1: Added `enum` support to sniffs #3482

kukulich · 2021-11-20T18:35:17Z

Based on #3478

kukulich · 2022-01-17T08:05:25Z

Rebased on master and ready for review.

gsherwood · 2022-01-17T21:57:15Z

First, this is terrific. Thank you.

Curious as to any opinions on treating enums with the exact same sniff rules as classes, especially PSR2 and PSR12 standards.

I'm in 2 minds. Given they are basically classes, treating them in the same way makes sense to me. But the PSR standards obviously don't mention enums, so it's quite possible someone reports a bug that PHPCS is enforcing rules that are not stated in the PSR.

I think there is an argument to say they are implied in the PSR, given the line :

The term "class" refers to all classes, interfaces, and traits.

and the contents of the RFC itself:

Enumerations are built on top of classes and objects. That means, except where otherwise noted, “how would Enums behave in situation X” can be answered “the same as any other object instance.”

But keen for other opinions.

jrfnl · 2022-01-17T22:37:25Z

Just my two cents:
Treating enums like classes in every single way but the "signature" (which would need special handling) and the "case" statements (which should probably be treated similar to class constants), is the logical thing to do and is how I would expect sniffs to behave.

FYI: I'm hoping to review most of the work in this PR tomorrow (if still needed by then).

src/Standards/Squiz/Tests/PHP/NonExecutableCodeUnitTest.2.inc

jrfnl · 2022-01-18T16:15:23Z

@kukulich I've started reviewing - shall I pull a PR with various tweaks to your branch to get through the small stuff ?

kukulich · 2022-01-18T16:16:54Z

@jrfnl Feel free :)

jrfnl · 2022-01-18T22:52:30Z

PR kukulich#1 has been pulled with some small improvements.

Notes:

Most are just small test improvements to add additional safeguards.

The new test in Squiz.Commenting.InlineComment was breaking a test which had to be at the end of the file. Fixed by moving the new test up.

The new test in Squiz.WhiteSpace.ControlStructureSpacing wasn't actually testing anything. Fixed by wrapping it in a control structure.

The test improvements involving "valid name" sniffs will, in most cases, now cause the tests to fail. This is due to bug Name of typed enum tokenized as T_GOTO_LABEL #3534, which was discovered during this review. The tests will pass once the bugfix PR has been merged.

The Squiz.Classes.ValidClassName sniff needed an additional fix to be able to handle backed enums.

That concludes part 1 of the review (= review current fixes).

For part 2, I'm now going to look at an additional set of results from token searches to see if any of the additional sniffs flagged need tests/sniff fixes for enum support.

jrfnl · 2022-01-19T02:04:33Z

Not 100% done yet, but here are my preliminary findings:

Additional sniffs for which tests/support should be added (I've prepared the commits for a few already):

Note sure about the Squiz.Classes.ClassDeclaration sniff. What do you think ?

kukulich · 2022-02-14T22:41:31Z

Rebased and green.

jaroslavlibal · 2022-02-22T13:54:49Z

I'm not sure if this is the right place to mention this, so apologies if not.

If I have an enum with some methods, like this:

<?php declare(strict_types = 1);

namespace Model\Language;

enum Language: string
{

    case CS = 'cs';
    case EN = 'en';

    /** @return string[] */
        private static function getNames(): array
        {
            return [
            self::CS->value => 'language.name.cs',
            self::EN->value => 'language.name.en',
            ];
        }
}

Then firstly phpcbf breaks the formatting (in the example, the output after it has been "fixed") and secondly phpcs reports a problem on the sniff PSR1.Files.SideEffects.FoundWithSymbols.

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.

* Test case file 1 should contain both a duplicate and a non-duplicate enum. * Test case file 2 should contain an enum which was already declared in file 1 to verify cross-file detection.

Ensure that all aspects of the sniff are tested for enums. While these tests pass, if you look at the actual error message output, you will see some weirdness (declaration name for the `enum` set as `string`). This should be fixed once PR 3534 has been merged.

Ensure that backed enum signatures are included in the tests. Note: the adjusted tests _without a space between the enum name and the colon_ will fail until PR 3534 has been merged.

Test more aspects of the sniff are applied correctly to enums.

The enum test was placed _below_ a test which was marked as testing a specific situation at the end of a file, which invalidated the "end of file" test. Fixed by moving the new test up.

Add a _valid_ test case for a backed enum. Note: this new test will fail until PR 3534 has been merged.

Let's also test the implements keyword when used with an enum.

Ensure that backed enum signatures are included in the tests. As this sniff retrieves the class name itself instead of using the `File::getDeclarationName()` method, these tests _will_ find the name correctly (not mistake the type for the name), but _may_ include the colon in the name as the "name end" determination checks on scope opener and whitespace and doesn't take the colon for the type declaration into account.

Only use tabs when in the test cases when they serve a specific purpose.

Let's also test that `$this` in a non-static enum method doesn't get flagged.

The test as-it-was, wasn't actually testing anything. As the sniff is about control structures, the `enum` needs to be nested in one before the test would fail without the fix,

rrajkomar · 2022-03-15T09:10:43Z

Hello,
Is there any ETA for this merge ? Currently php enums are unusable with PHPCS which is delaying any migration to PHP8.1 with Enums (at least without disabling analysis on those files).
Also is there any estimation on the 3.7.0 release date ?
Thanks.

gsherwood · 2022-03-15T09:17:19Z

Hello, Is there any ETA for this merge ? Also is there any estimation on the 3.7.0 release date ?
There is no ETA. I've been exceptionally busy and haven't been able to review the changes since I last approved.

gsherwood · 2022-03-18T03:38:32Z

@jrfnl @kukulich I'm keen to get a release out with at least some ENUM support.

I think what's here already is great. While more support could be added, I'd like to know if either of you see any blockers to getting this merged in. I know I'm not going to have time to work on any of the additional sniff changes, but I can at least get this merged and released to partial support is available.

kukulich · 2022-03-18T05:42:02Z

I would be happy with release with partial enum support :)

jrfnl · 2022-03-18T09:39:39Z

@jrfnl @kukulich I'm keen to get a release out with at least some ENUM support.

I think what's here already is great. While more support could be added, I'd like to know if either of you see any blockers to getting this merged in. I know I'm not going to have time to work on any of the additional sniff changes, but I can at least get this merged and released to partial support is available.

Happy for this to be merged.

I could pull the extra changes which I worked on already either to this PR or separately, whichever is preferred.

Not all sniffs having support is IMO not a blocker for the release.

However, I do think it would be good if the below three from my previous comment got sorted before the release:

File::getMemberProperties() - Should this get an exception for enums, along the same lines as for interfaces ?

File::findImplementedInterfaceNames() - Should support finding the interfaces implemented by enums too

File::findEndOfStatement() - Needs tests.

For the first I'd like a second opinion before actioning this.
I can work on getting the second sorted.
Anyone else up for looking at the third ?

Other than that for a release to go out, please be aware that IMO supporting intersection types will require the introduction of a new token (similar to what was done for union types), so that would mean that support for intersection types would need to wait until 3.8.0.

janedbal · 2022-03-21T14:46:02Z

Happy for this to be merged.

Great! We are really looking forward to this! Can we expect new version this week?

acidjazz · 2022-03-23T21:11:34Z

Almost 5 months since the release of PHP 8.1 Enums - lets merge and release!

gsherwood · 2022-03-31T22:30:58Z

This is now merged. Thanks all.

kukulich mentioned this pull request Nov 20, 2021

PHP 8.1 support #3479

Closed

8 tasks

kukulich marked this pull request as draft November 20, 2021 18:36

kukulich changed the title ~~PHP 8.1: Added enum support to valid sniffs~~ PHP 8.1: Added enum support to sniffs Nov 20, 2021

gsherwood added this to Idea Bank in PHPCS v3 Development via automation Nov 21, 2021

gsherwood added this to the 3.7.0 milestone Nov 21, 2021

kukulich force-pushed the php81-enum branch 2 times, most recently from 6d83d31 to 7fa55f9 Compare December 17, 2021 07:36

kukulich force-pushed the php81-enum branch 2 times, most recently from 8f70e33 to 22375be Compare January 17, 2022 08:01

kukulich marked this pull request as ready for review January 17, 2022 08:05

gsherwood requested changes Jan 17, 2022

View reviewed changes

src/Standards/Squiz/Tests/PHP/NonExecutableCodeUnitTest.2.inc Outdated Show resolved Hide resolved

kukulich force-pushed the php81-enum branch from 22375be to 3295e62 Compare January 17, 2022 22:45

kukulich requested a review from gsherwood January 17, 2022 22:50

gsherwood approved these changes Jan 17, 2022

View reviewed changes

jrfnl mentioned this pull request Jan 18, 2022

Name of typed enum tokenized as T_GOTO_LABEL #3534

Merged

jrfnl mentioned this pull request Jan 18, 2022

Various updates to enum sniff fixes kukulich/PHP_CodeSniffer#1

Merged

kukulich force-pushed the php81-enum branch 2 times, most recently from fadddfc to aadc30c Compare February 14, 2022 22:37

faytekin approved these changes Mar 2, 2022

View reviewed changes

kukulich added 2 commits March 13, 2022 21:36

PHP 8.1: Squiz/ValidVariableName - Added tests for enums support

93e703a

PHP 8.1: Zend/ValidVariableName - Added tests for enums support

be50d27

kukulich and others added 14 commits March 13, 2022 21:37

PHP 8.1: Squiz/StaticThisUsage - Added support for enums

b520d4b

File::getDeclarationName(): tweak method description

fc2c525

Generic/DuplicateClassName: improve enum tests

024c47d

* Test case file 1 should contain both a duplicate and a non-duplicate enum. * Test case file 2 should contain an enum which was already declared in file 1 to verify cross-file detection.

PEAR/ValidClassName: improve enum tests

2632099

Ensure that backed enum signatures are included in the tests. Note: the adjusted tests _without a space between the enum name and the colon_ will fail until PR 3534 has been merged.

PSR12/UseDeclaration: improve enum tests

37bc4e0

Test more aspects of the sniff are applied correctly to enums.

Squiz/InlineComment: fix enum test

19b5775

The enum test was placed _below_ a test which was marked as testing a specific situation at the end of a file, which invalidated the "end of file" test. Fixed by moving the new test up.

Squiz/ClassFileName: improve enum tests

b1ae480

Add a _valid_ test case for a backed enum. Note: this new test will fail until PR 3534 has been merged.

Squiz/LowercaseClassKeywords: improve enum tests

7494c3d

Let's also test the implements keyword when used with an enum.

Squiz/NonExecutableCode: minor test tweak

b0b4add

Only use tabs when in the test cases when they serve a specific purpose.

Squiz/StaticThisUsage: improve enum tests

5629985

Let's also test that `$this` in a non-static enum method doesn't get flagged.

Squiz/ControlStructureSpacing: fix the enum test

15a0b4e

The test as-it-was, wasn't actually testing anything. As the sniff is about control structures, the `enum` needs to be nested in one before the test would fail without the fix,

kukulich force-pushed the php81-enum branch from aadc30c to 15a0b4e Compare March 13, 2022 20:38

dereuromark mentioned this pull request Mar 28, 2022

Add SameSiteEnum. cakephp/cakephp#16404

Merged

jrfnl mentioned this pull request Mar 31, 2022

ScopeIndent reports wrong indentation in enums #3572

Closed

gsherwood merged commit d73a063 into squizlabs:master Mar 31, 2022

PHPCS v3 Development automation moved this from Idea Bank to Ready for Release Mar 31, 2022

jrfnl mentioned this pull request Mar 31, 2022

PHP 8.1 | Add enum support to various additional sniffs #3573

Merged

kukulich deleted the php81-enum branch April 19, 2022 14:19

jrfnl mentioned this pull request Oct 15, 2022

PHP 8.1: add support for enums PHPCSStandards/PHPCSUtils#358

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PHP 8.1: Added `enum` support to sniffs #3482

PHP 8.1: Added `enum` support to sniffs #3482

kukulich commented Nov 20, 2021 •

edited

Loading

kukulich commented Jan 17, 2022

gsherwood commented Jan 17, 2022 •

edited

Loading

jrfnl commented Jan 17, 2022

jrfnl commented Jan 18, 2022

kukulich commented Jan 18, 2022

jrfnl commented Jan 18, 2022 •

edited

Loading

jrfnl commented Jan 19, 2022 •

edited

Loading

kukulich commented Feb 14, 2022

jaroslavlibal commented Feb 22, 2022 •

edited

Loading

rrajkomar commented Mar 15, 2022 •

edited

Loading

gsherwood commented Mar 15, 2022

gsherwood commented Mar 18, 2022

kukulich commented Mar 18, 2022

jrfnl commented Mar 18, 2022

janedbal commented Mar 21, 2022

acidjazz commented Mar 23, 2022

gsherwood commented Mar 31, 2022

PHP 8.1: Added enum support to sniffs #3482

PHP 8.1: Added enum support to sniffs #3482

Conversation

kukulich commented Nov 20, 2021 • edited Loading

kukulich commented Jan 17, 2022

gsherwood commented Jan 17, 2022 • edited Loading

jrfnl commented Jan 17, 2022

jrfnl commented Jan 18, 2022

kukulich commented Jan 18, 2022

jrfnl commented Jan 18, 2022 • edited Loading

jrfnl commented Jan 19, 2022 • edited Loading

kukulich commented Feb 14, 2022

jaroslavlibal commented Feb 22, 2022 • edited Loading

rrajkomar commented Mar 15, 2022 • edited Loading

gsherwood commented Mar 15, 2022

gsherwood commented Mar 18, 2022

kukulich commented Mar 18, 2022

jrfnl commented Mar 18, 2022

janedbal commented Mar 21, 2022

acidjazz commented Mar 23, 2022

gsherwood commented Mar 31, 2022

PHP 8.1: Added `enum` support to sniffs #3482

PHP 8.1: Added `enum` support to sniffs #3482

kukulich commented Nov 20, 2021 •

edited

Loading

gsherwood commented Jan 17, 2022 •

edited

Loading

jrfnl commented Jan 18, 2022 •

edited

Loading

jrfnl commented Jan 19, 2022 •

edited

Loading

jaroslavlibal commented Feb 22, 2022 •

edited

Loading

rrajkomar commented Mar 15, 2022 •

edited

Loading