🌐 AI搜索 & 代理 主页
Skip to content

Conversation

@Nikschavan
Copy link
Contributor

Fixes #922

Copy link
Member

@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.

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.

Copy link
Member

@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.

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 ?

@Nikschavan
Copy link
Contributor Author

Nikschavan commented Nov 4, 2019

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.

Copy link
Member

@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.

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.

@Nikschavan
Copy link
Contributor Author

I must have probably missed that, I will quickly push another commit to add require as well.

@Nikschavan
Copy link
Contributor Author

I have added the missing require keyword as well, the PR should be good to merge now.

@jrfnl
Copy link
Member

jrfnl commented Nov 4, 2019

@Nikschavan Excellent. Thank you for that. Once the build has passed, I will merge this.

@jrfnl jrfnl merged commit f9e2c6e into PHPCompatibility:develop Nov 4, 2019
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.

Detect use of reserved keyword names as function/method names

2 participants