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

False positive with 'alert' on PHP files #18

Closed
viniciusfabri opened this issue Feb 8, 2023 · 2 comments · Fixed by #20
Closed

False positive with 'alert' on PHP files #18

viniciusfabri opened this issue Feb 8, 2023 · 2 comments · Fixed by #20

Comments

@viniciusfabri
Copy link

Steps to reproduce

  1. Add the following line to a PHP class
$this->logger->alert('bla');
  1. Run testing suite

Expected result

Testing suite passses

Actual result

It fails with a 'git_blacklist' error

Additional info

There's a generic 'alert(' blacklist that should only apply to JS files I think: https://github.com/YouweGit/testing-suite/blob/master/config/magento2/grumphp.yml#L11

@rutgerrademaker
Copy link
Contributor

@viniciusfabri

Maybe we could add some regex to allow for ->alert( while disallowing alert(
Played a little bit around with this, but could not get it to work properly
I'm no regex hero, but this should be correct I think (?<![->]{2})alert( or (?<![->]{2})alert[\x28] as it seems grumphp has some issues with regxexes that needs to be defined /escaped in yaml

See also https://github.com/phpro/grumphp/blob/master/doc/tasks/git_blacklist.md

Another Idea I came up with would be to split the tasks / lists to be used per language (php/js/phtml)
Have not tested if that is a feasible option yet

@viniciusfabri
Copy link
Author

New false positive:
image

rutgerrademaker added a commit to rutgerrademaker/testing-suite that referenced this issue Feb 17, 2023
### Changed
- `alert(` was removed from the git blacklist as it conflicts with PSR3, see
  [YouweGit#18](YouweGit#18)
fredden pushed a commit that referenced this issue Feb 20, 2023
### Changed
- `alert(` was removed from the git blacklist as it conflicts with PSR3, see
  [#18](#18)
igorwulff pushed a commit that referenced this issue Jun 15, 2023
* ## 2.16.2
### Changed
- `alert(` was removed from the git blacklist as it conflicts with PSR3, see
  [#18](#18)

* Add bitexpert php stand module to Magento projects
In addition to generating code, it also adds some extra rules
Checkout the links from the docs for additional information
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 a pull request may close this issue.

2 participants