-
-
Notifications
You must be signed in to change notification settings - Fork 204
ForbiddenNames: Add missing reserved keywords to ForbiddenNames sniff #923
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
Conversation
jrfnl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Nikschavan Thanks for this PR and thanks also for adding the additional missing keywords.
Looks good and I currently can't think of any code patterns which would cause false positives for these, though no doubt, we'll get reports if they exist 😃
The only thing I'd like to point out is that the original list was in alphabetic order which makes future maintenance of the list easier.
It would be great if you would be willing to update the PR to make the lists alphabetic again.
jrfnl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops... and I forgot to mention in the issue that it would also be good if you could add some dummy code with valid use of the keywords to the https://github.com/PHPCompatibility/PHPCompatibility/blob/develop/PHPCompatibility/Tests/Keywords/ForbiddenNamesUnitTest.inc file.
That file basically tests that the sniff does not report false positives when the keyword is used correctly.
So for list, the example code could be:
list( $a, $b, $c ) = $array;And for include, the example code could be:
include __DIR__ . '/path/to/file.php';
include( $file );A small snippet of dummy code should be added for valid use of each of the added keywords.
Does that make sense ?
|
Thank you for the feedback @jrfnl, I have organized the new keys alphabetically and added some dummy use cases for the keywords, let me know if this needs any improvement. |
jrfnl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Nikschavan Thank you for that update. All looks good to me.
The only question I still have is whether there was any particular reason to include require_once, but not require.
Not a blocker for merging this PR as the keyword can still be added later in a separate PR.
|
I must have probably missed that, I will quickly push another commit to add |
|
I have added the missing |
|
@Nikschavan Excellent. Thank you for that. Once the build has passed, I will merge this. |
Fixes #922