Skip to content

PHP 8.1: Added support for enum keyword #3478

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 16, 2022
Merged

PHP 8.1: Added support for enum keyword #3478

merged 1 commit into from
Jan 16, 2022

Conversation

@kukulich
Copy link
Contributor

@kukulich kukulich commented Nov 19, 2021 •
edited
Loading

Fixes #3474

@kukulich kukulich changed the title PHP 8.1: Added support for "enum" keyword PHP 8.1: Added support for enum keyword Nov 19, 2021
jrfnl
jrfnl reviewed Nov 19, 2021
View reviewed changes
Copy link
Contributor

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kukulich 👋🏻 Thank you for creating this PR. Looking good!

I've left some comments inline to think over. Hope it helps.

@kukulich kukulich force-pushed the php81-enum-keyword branch 2 times, most recently from a86c6de to d709d56 Compare November 19, 2021 20:58
jrfnl
jrfnl reviewed Nov 19, 2021
View reviewed changes
@jrfnl
Copy link
Contributor

jrfnl commented Nov 19, 2021

Thanks for the update @kukulich, left one more comment. Other that that, 👍🏻

jrfnl
jrfnl approved these changes Nov 19, 2021
View reviewed changes
Copy link
Contributor

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't verified against the RFC, I trust @kukulich will have done that, but PR-wise, this looks to cover all the bases 💯

@kukulich kukulich mentioned this pull request Nov 20, 2021
Closed
8 tasks
@kukulich kukulich force-pushed the php81-enum-keyword branch 2 times, most recently from 42551e5 to eb27ac3 Compare November 20, 2021 11:05
jrfnl
jrfnl requested changes Nov 20, 2021
View reviewed changes
Copy link
Contributor

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, still found some more issues to be addressed.

tests/Core/Tokenizer/EnumTest.php Outdated
*
* @return void
*/
public function testEnums($testMarker, $expectedScopeOpenerLine, $expectedScopeCloserLine)
Copy link
Contributor

@jrfnl jrfnl Nov 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering - why test the line and not the actual token position (in relation to the keyword token) ?
I realize the latter is more fiddly for setting up the tests, but it's also much more precise.

In other tests, this has been done by the data provider providing an offset and the token position then being calculated by $expectedOpener = $enum + $openerOffset;.

Copy link
Contributor Author

@kukulich kukulich Nov 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modified.

Copy link
Contributor

@jrfnl jrfnl Nov 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you modified it for scope opener, but removed the check for scope closer ?

Copy link
Contributor Author

@kukulich kukulich Nov 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added now.

@kukulich kukulich force-pushed the php81-enum-keyword branch 3 times, most recently from 3e94231 to bca788c Compare November 20, 2021 14:35
This was referenced Nov 20, 2021
Merged
Merged
@gsherwood gsherwood added this to the 3.7.0 milestone Nov 21, 2021
@kukulich kukulich mentioned this pull request Nov 21, 2021
Merged
3 tasks
@kukulich kukulich force-pushed the php81-enum-keyword branch 3 times, most recently from a7a0128 to 106eba3 Compare December 17, 2021 07:34
@tarlepp
Copy link

tarlepp commented Jan 9, 2022

What is the current state of this one?

@kukulich
Copy link
Contributor Author

kukulich commented Jan 12, 2022

Rebased on master.

gsherwood added a commit that referenced this pull request Jan 16, 2022
@gsherwood
Changelog for #3474 (ref #3478)
b7a931a
@gsherwood gsherwood merged commit f236c6a into squizlabs:master Jan 16, 2022
gsherwood
gsherwood reviewed Jan 16, 2022
View reviewed changes
tests/Core/Tokenizer/BackfillEnumTest.php
$tokens = self::$phpcsFile->getTokens();

$target = $this->getTargetToken($testMarker, [T_ENUM, T_STRING], $testContent);
$this->assertSame(T_STRING, $tokens[$target]['code']);
Copy link
Member

@gsherwood gsherwood Jan 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Due to the way namespaces are tokenized in 4.0 (they become T_NAME_RELATIVE or T_NAME_QUALIFIED) I had to reverse this logic in the 4.0 branch to use assertNotSame(T_ENUM...). I didn't make the same change in master.

@gsherwood
Copy link
Member

gsherwood commented Jan 16, 2022

Thanks a lot for this PR. I really appreciate the effort you're putting in to add support for 8.1 features to PHPCS.

@kukulich kukulich deleted the php81-enum-keyword branch January 17, 2022 23:52
@jrfnl jrfnl mentioned this pull request Jan 18, 2022
Merged
@wickedOne wickedOne mentioned this pull request Feb 8, 2022
Open
@jrfnl jrfnl mentioned this pull request Feb 10, 2022
Merged
@patrickomeara patrickomeara mentioned this pull request Feb 18, 2022
Closed
@jrfnl jrfnl mentioned this pull request Mar 31, 2022
Closed
@jacekwilczynski jacekwilczynski mentioned this pull request May 30, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gsherwood gsherwood gsherwood left review comments

@jrfnl jrfnl jrfnl requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

3.7.0

Development

Successfully merging this pull request may close these issues.

PHP 8.1 enum not recognized

4 participants

@kukulich @jrfnl @tarlepp @gsherwood