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

PHP 8.1: New PHPCompatibility.Classes.NewFinalClassConstants sniff #1317

Merged
merged 1 commit into from
Apr 14, 2022

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Mar 9, 2022

Added support for the final modifier for class constants. Also, interface constants become overridable by default.

New sniff to detect this new feature.

Note: the sniff does not do anything special for non-final interface constants which previously were not overridable.

Includes unit tests.
Includes docs.

Refs:

Related to #1299

> Added support for the final modifier for class constants. Also, interface constants become overridable by default.

New sniff to detect this new feature.

Note: the sniff does not do anything special for non-final interface constants which previously were not overridable.

Includes unit tests.
Includes docs.

Refs:
* https://wiki.php.net/rfc/final_class_const
* https://www.php.net/manual/en/migration81.new-features.php#migration81.new-features.core.final-constants
* https://www.php.net/manual/en/language.oop5.final.php#language.oop5.final.example.php81
* php/php-src#6878
* php/php-src@a5360e8
@jrfnl jrfnl added enhancement PR: quick merge PR only contains relatively simple changes PR: ready for review labels Mar 9, 2022
@jrfnl jrfnl added this to the 10.0.0 milestone Mar 9, 2022
@jrfnl jrfnl requested a review from wimg March 9, 2022 00:26
@jrfnl
Copy link
Member Author

jrfnl commented Mar 9, 2022

@afilina And another one.

<code title="PHP >= 8.1: using the final modifier.">
<![CDATA[
class Foo {
<em>final</em> const BAR = 10;
Copy link
Contributor

Choose a reason for hiding this comment

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

The HTML tag appears literally in markdown docs. I recommend avoiding markdown in code samples. Other than that, everything looks good on the docs side.

Suggested change
<em>final</em> const BAR = 10;
final const BAR = 10;

Copy link
Member Author

Choose a reason for hiding this comment

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

The <em> markers are intended to highlight the "issue" in the HTML view.
In the Text and the MarkDown view, they are automatically removed when the report is generated.

How did you generate the report to get them to show ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just checked the source code of the generators which are included with PHPCS and the above is correct. Also tested with the docs for another sniff I was just working on.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the generator code I mean: https://github.com/squizlabs/PHP_CodeSniffer/blob/51335eb46b2b940b6c429643fe96f514d4a4e4a1/src/Generators/Markdown.php#L131-L141

If the code in the phpcs docs repo does not line up with that, it will need to be adjusted.

@wimg wimg merged commit c23e20c into develop Apr 14, 2022
@wimg wimg deleted the php-8.1/new-newfinalconstants-sniff branch April 14, 2022 19:54
@jrfnl jrfnl removed PR: quick merge PR only contains relatively simple changes PR: ready for review labels Sep 4, 2022
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.

None yet

3 participants