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

Remove (abandoned) sensiolabs/security-checker #2356

Merged
merged 1 commit into from
Feb 2, 2021

Conversation

bobdenotter
Copy link
Member

No description provided.

Copy link
Member

@I-Valchev I-Valchev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Contributor

@toofff toofff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not replace it with this repository that Symfony recommends instead of just removing the dependency?

@I-Valchev
Copy link
Member

Hi @toofff , our understanding is that the security checker is very similar to roave/security-advisories, hence no need to keep two dependencies. Is that an incorrect conclusion in your view? @toofff

@toofff
Copy link
Contributor

toofff commented Feb 2, 2021

@I-Valchev if it does the same job then yes :)

On the other hand it is not put in the CI, is this normal?

@bobdenotter
Copy link
Member Author

Why not replace it with this repository that Symfony recommends instead of just removing the dependency?

It's not a matter of simply replacing it. The recommendation is for something tat's meant as a standalone tool, not something to do a one-on-one replacement with. We could integrate that in the CI, but regardless the sensiolabs/security-checker has got to go.
And like Ivo mentions: I don't think there's added value in having both roave/security-advisories as well as using github.com/fabpot/local-php-security-checker.

@toofff
Copy link
Contributor

toofff commented Feb 2, 2021

@bobdenotter

The pull request did not mention the use of the roave/security-advisories package, that's why I commented on the pull request. But if the other package does the job, then yes ;)

On the other hand, could it be good to add it to the CI as a non-blocking job?

@I-Valchev
Copy link
Member

@toofff According to the documentation https://github.com/Roave/SecurityAdvisories, it already works on update, which we already do.

I don't if it is added explicitly though, in fact it looks like the better solution. Do you want to create a PR for it?

In the meantime, let's merge this in.

@I-Valchev I-Valchev merged commit 612debf into 4.1 Feb 2, 2021
@I-Valchev I-Valchev deleted the fix/dont-run-security-checker branch February 2, 2021 09:56
@I-Valchev I-Valchev restored the fix/dont-run-security-checker branch February 2, 2021 16:17
@I-Valchev I-Valchev deleted the fix/dont-run-security-checker branch February 2, 2021 16:19
@paras-malhotra
Copy link

@I-Valchev you can consider the Enlightn Security Checker.

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

Successfully merging this pull request may close these issues.

4 participants