Skip to content

Problem with AjaxNullComparisionSniff #2614

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

Closed
slepic opened this issue Sep 27, 2019 · 4 comments
Closed

Problem with AjaxNullComparisionSniff #2614

slepic opened this issue Sep 27, 2019 · 4 comments

Comments

@slepic
Copy link

slepic commented Sep 27, 2019

version: at least 3.4.2 up to current master

In the following snippet, warning is triggered

class X
{
/**
 * @api
 */
public function x()
{
}

private function y($x)
{
  if ($x === null) { //warning reported here
    
  }
} 
}

Reason for this is that method y() has parameter but does not have doc comment and the method above (the method x()) has a doc comment with @api annotation.
This causes the sniff to believe that the method y() is an api method, although actualy x() is.

You can see in the sniff it just searches for the previous doc comment, which does not necesarily belong to the method in question:

https://github.com/squizlabs/PHP_CodeSniffer/blob/master/src/Standards/MySource/Sniffs/PHP/AjaxNullComparisonSniff.php#L56

Putting even an empty doc comment above the methody y() makes the warning go away.

@jrfnl
Copy link
Contributor

jrfnl commented Sep 27, 2019

Yup. This is a known issue and was fixed in #2456, but that PR will not get merged....

@gsherwood
Copy link
Member

I'm really surprised you are using this sniff - it's a project-specific sniff that I will delete in version 4 (and should have deleted a long time ago). Is this sniff actually providing value to you?

@slepic
Copy link
Author

slepic commented Sep 30, 2019

Well, it was part of our company's ruleset long before i started working here.
We are using http://apidocjs.com/ to generate docs for our REST API, and thats where the sniff got hits in our project. I suppose we could remove it from the ruleset. I dont really think it ever brought any significant value anyway. And if it's gonna get deprecated and removed, we will simply stop using it. I want to keep third party packages as recent as possible and updates as smooth as possible.

So as for this issue instance. I'm quite settled, so its up to you whether you want to close this issue or keep it open for whatever reason...

Thanks anyway for quick replies and explanations.

@gsherwood
Copy link
Member

I'm going to close this as I don't think looking into this sniff is a good use of time. I think you've made the right decision to remove it, but please let me know if you discover that it was bringing some value.

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

No branches or pull requests

3 participants