这是indexloc提供的服务，不要输入任何密码
Skip to content

phpcs-security-audit > Add Security ruleset to phpcs #345

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 18, 2021
Merged

phpcs-security-audit > Add Security ruleset to phpcs #345

merged 1 commit into from
Feb 18, 2021

Conversation

@mihai-stancu
Copy link
Contributor

@mihai-stancu mihai-stancu commented Feb 14, 2021

Hello,

I'd like to propose the addition of a phpcs security ruleset provided by FloeDesignTechnologies here.

Approach

Since it's a phpcs ruleset it's not a standalone command so it cannot be installed via phar-download or composer-bin-plugin so I used composer-global-install which incidentally also installs squizlabs/PHP_CodeSniffer as a dependency.

Drawbacks

The package also requires dealerdirect/phpcodesniffer-composer-installer which normally would configure phpcs to recognize the new ruleset but since it does that by discovering where phpcs is installed via composer it will not discover the correct phpcs binary (from /tools). So I also executed a sh line to correcly configure phpcs.

There seem to be a lot of drawbacks to this approach. The only alternative I see would be downloading rulesets separately. Please advise.

@jakzal
Copy link
Owner

jakzal commented Feb 16, 2021

Since it's a phpcs ruleset it's not a standalone command so it cannot be installed via phar-download or composer-bin-plugin so I used composer-global-install which incidentally also installs squizlabs/PHP_CodeSniffer as a dependency.

Why composer-bin-plugin cannot be used?

@mihai-stancu
Copy link
Contributor Author

mihai-stancu commented Feb 16, 2021

Since it's a phpcs ruleset it's not a standalone command so it cannot be installed via phar-download or composer-bin-plugin so I used composer-global-install which incidentally also installs squizlabs/PHP_CodeSniffer as a dependency.

Why composer-bin-plugin cannot be used?

Apologies, I didn't fully understand how composer-bin-plugin worked and made some wrong assumptions. It can be done via composer-bin-plugin, I'll update the PR.

I could also change phpcs from phar-download to composer-bin-plugin so that:

  • they share the same namespace
  • phpcs only gets installed once
  • the dealerdirect/phpcodesniffer-composer-installer autodiscovery would work
  • we would not need to use the sh configuration thing

@jakzal
Copy link
Owner

jakzal commented Feb 16, 2021

I could also change phpcs from phar-download to composer-bin-plugin

Please, do that. Thanks!

@mihai-stancu
Copy link
Contributor Author

mihai-stancu commented Feb 16, 2021

Note: phpcs is actually already installed as a dependency of phpinsights but I think it's just using phpcs, not integrating with it.

Should the namespace I'm adding be phpcs because I'm adding phpcs + some rulesets?

@mihai-stancu
Copy link
Contributor Author

mihai-stancu commented Feb 16, 2021

I'm having a bit of trouble after running composer global bin phpcs require pheromone/phpcs-security-audit manually, it doesn't have dealerdirect/phpcodesniffer-composer-install inside the namespace although it's a dependency.

@mihai-stancu
Copy link
Contributor Author

mihai-stancu commented Feb 16, 2021

My bad, v2.0.1 doesn't actually require dealerdirect/phpcodesniffer-composer-install which is why it was never installed / triggered.

@jakzal
Copy link
Owner

jakzal commented Feb 17, 2021

Looks good. Ready?

@jakzal
Copy link
Owner

jakzal commented Feb 17, 2021

@mihai-stancu you might want to clean up your branch. Perhaps rebase?

Also, it seems that your commiter email address is not connected to your github account.

@mihai-stancu
Copy link
Contributor Author

mihai-stancu commented Feb 17, 2021

Hey,

I experimented a bit last night and was waiting for the Scrutinizer check to pass in the morning.

I'll rebase soon. 10ks.

@mihai-stancu
Copy link
Contributor Author

mihai-stancu commented Feb 17, 2021

The PR looks clean now after the rebase. All good?

@jakzal
Copy link
Owner

jakzal commented Feb 18, 2021

@mihai-stancu can you do one more thing for me and run bin/devkit.php update:readme and commit changes please?

@mihai-stancu
Copy link
Contributor Author

mihai-stancu commented Feb 18, 2021

I've done that, but there's a catch -- by adding the phpcodesniffer-composer-install plugin explicitly as a tool in the phpcs.json file it gets automatically added to the README.

Is there a way to specify that it shouldn't?

@jakzal
Copy link
Owner

jakzal commented Feb 18, 2021

That's fine. Keep it simple.

@mihai-stancu
Copy link
Contributor Author

mihai-stancu commented Feb 18, 2021

So should we let the phpcodesniffer-composer-install line appear in the README (i.e.: should I rerun the devkit and update the README)?

@jakzal
Copy link
Owner

jakzal commented Feb 18, 2021

Yes, please. Let's not make exceptions. We already list other "pre-installation" type of tools.

@jakzal jakzal merged commit b2e622a into jakzal:master Feb 18, 2021
@jakzal
Copy link
Owner

jakzal commented Feb 18, 2021

Thank you @mihai-stancu 🍺

github-actions bot pushed a commit to jakzal/phpqa that referenced this pull request Feb 19, 2021
@jakzal
Update toolbox 1.40.1 -> 1.41.0
a99f9b4 
**Breaking changes**:

* Install larastan in a separate namespace with its own phpstan installation. Phpstan installation with larastan is available as . jakzal/toolbox#349

Updates:

* psalm (4.6.0 -> 4.6.1) jakzal/toolbox#351

New tools:

* phpcs-security-audit - [Finds vulnerabilities and weaknesses related to security in PHP code](https://github.com/FloeDesignTechnologies/phpcs-security-audit) jakzal/toolbox#345 (thank you @mihai-stancu)

* phpcodesniffer-composer-install - [Easy installation of PHP_CodeSniffer coding standards (rulesets).](https://github.com/Dealerdirect/phpcodesniffer-composer-installer) jakzal/toolbox#345 (thank you @mihai-stancu)
@jakzal jakzal mentioned this pull request Feb 19, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jakzal jakzal jakzal approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@mihai-stancu @jakzal