PHP 8.0: NewNestedStaticAccess: add support for class constant dereferencing #1262
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…rencing > ... class constant accesses are only array and object dereferencable. This > means that while `Foo::$bar::$baz` is legal, `Foo::BAR::$baz` is not. > > This RFC proposes to make class constant accesses static derefencable as > well, so that `Foo::BAR::$baz` and `Foo::BAR::BAZ` become legal. Ref: * https://wiki.php.net/rfc/variable_syntax_tweaks#class_constant_dereferencability * php/php-src#5061 * php/php-src@ab154b7 Includes unit tests. Relates to PHPCompatibility#809.
jrfnl
requested changes
Dec 28, 2020
Member
jrfnl
left a comment
jrfnl
left a comment
There was a problem hiding this comment.
Hi @elazar, thanks for this change!
Looking very good.
I only have a few small remarks, nothing major.
- Each error message should have a unique error code so error messages can be selectively ignored. As things are, they currently both have the same error code.
You can either just adjust the error code for the new error message or adjust both to made the distinction between the two clear.
As the next release will be a major, we don't have to worry too much about a change in the original error code being a BC break. - As this sniff now has checks related to different PHP versions, it would be a good idea to add a "no-violation on PHP 7.0" check to the
testNestedStaticAccess()method. - Aside from the existing "false positive" prevention checks, is there any other code pattern you can think of which we need to guard against specifically for the new class constant dereferencing ? I.e. are there new false positive code patterns which could be added in the test case file which aren't currently accounted for ? (if the answer is "no", that's perfectly fine, I just want to make sure this has been considered)
Non-blocking:
- The error message phrasing is inconsistent -
cannot be dereferenced in PHP 7.4 or earliervswas not supported in PHP 5.6 or earlier.
Not a blocker, but might be an idea to make the phasing more consistent. - Code-scouting: I realize the original
testNestedStaticAccess()test does not have a proper method description, but with an eye on the documentation clean up, it would be nice if the newly added test method has a proper description and if you feel like adding a proper description to thetestNestedStaticAccess()that would be awesome too! 😄
PHPCompatibility/Tests/Syntax/NewNestedStaticAccessUnitTest.inc
Outdated
Show resolved
Hide resolved
40 tasks
added 5 commits
December 28, 2020 17:02
Contributor
Author
Done in f4e4e84.
Done in bdc6c09.
None that I can think of, no.
Done in bb66569.
Done in d5c7e51. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ref:
Includes unit tests.
Relates to #809.