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

WordPress-VIP-Go: Add ignore statement for ruleset since VariableAnalysis.CodeAnalysis.VariableAnalysis #636

Closed
wants to merge 1 commit into from

Conversation

rebeccahum
Copy link
Contributor

@rebeccahum rebeccahum commented Mar 9, 2021

Removing the global $wpdb in #635 triggered a warning on line 300.

…ysis.CodeAnalysis.VariableAnalysis was being triggered
@rebeccahum rebeccahum requested a review from a team as a code owner March 9, 2021 16:33
@rebeccahum rebeccahum closed this Mar 9, 2021
@rebeccahum rebeccahum deleted the rebecca/fix_vip_go_ruleset_test branch March 9, 2021 17:06
@rebeccahum
Copy link
Contributor Author

Nevermind, going to stick this in #635 since related.

@jrfnl
Copy link
Collaborator

jrfnl commented Mar 9, 2021

My comment is superseded by the issue being closed while I was writing it.

Posting it anyway in case this comes up again.


With the current develop branch I cannot reproduce the issue this is supposedly solving.
Running phpcs ./wordpress-vip-go/ruleset-test.inc --standard=wordpress-vip-go does not show any errors or warnings on line 300.

I might be misunderstanding, so could you please elaborate on what this is supposed to be solving ?

Other notes:

  • Is an ignore comment the right way to solve it ? Or should the variable be added to the list of variables at the top of the file ?
  • If an ignore comment is the way to go: can the ignore comment be made error code specific ?

@rebeccahum
Copy link
Contributor Author

rebeccahum commented Mar 9, 2021

@jrfnl The test was failing on #635 and I mistakenly thought it was unrelated but after further inspection, it was related so I'm putting it in that PR.

Edit: After thinking more on this, adding the ignore comment is a dependency that we don't need so I have added the global to that line.

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.

2 participants