ForbiddenNames: Add missing reserved keywords to ForbiddenNames sniff#923
ForbiddenNames: Add missing reserved keywords to ForbiddenNames sniff#923jrfnl merged 7 commits intoPHPCompatibility:developfrom
Conversation
jrfnl
left a comment
There was a problem hiding this comment.
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.
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.
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