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

Skip validating user defined functions which has similar name as file system function #65

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mgalang
Copy link

@mgalang mgalang commented Mar 2, 2020

Description

The Filesystem function sniff is also detecting and validating user defined functions even if the function is not a PHP native filesystem function.

Proposed fixed: Check if previous token is "function" to check if the function is user defined.

Steps to reproduce:

  1. Create a function called delete function delete($myvalue) {}
  2. Run phpcs
  3. It throws a warning Filesystem function delete() detected with dynamic parameter

@jmarcil
Copy link
Collaborator

jmarcil commented Mar 16, 2020

I think that this is a valid way of diminishing false positives.

But why just limit it to filesystem functions? I can see this one giving false positive too:

function exec($myvalue) {}

I'm just assuming you're fixing this one because delete is a common function that is being used in PHP but I want to explore pushing your idea further to have a broader impact.

Side note on php tokens; We should compare using "T_FUNCTION" on ['code'] instead of "function" on ['content'].

@jrfnl
Copy link
Contributor

jrfnl commented Mar 16, 2020

@jmarcil FYI: PHPCSUtils will have an abstract sniff for function call sniffing which takes this and much more (aliased functions in use statements, method calls etc) into account and would greatly reduce false positives for all sniffs looking at (global) function calls.

It might be worth considering adding that as a dependency.

I expect that abstract will be pulled within the next few weeks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants