-
Notifications
You must be signed in to change notification settings - Fork 84
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
Fix compliance of project with PHPCS #47
Comments
Just wondering if anyone had given this any thought yet ? |
Yes a PR on this would be more than welcomed! As for the impact of what it might break, I think it would be wise to merge it as part of the eventual version 3.0 that supports better documentation. Note that the actual release with 3.0 will also have to coincide with moving this into an OWASP project. I'll work on that soon and I welcome everyone that wants to be part of the project to contact me (twitter dm: jonathanmarcil is the quickest way). |
BTW I noticed you specified that this change wouldn't break anything, and I believe that, but if we're currently into a broken state I'm afraid that some people implemented workarounds that might actually break once we do it right (especially the symlink part). 3.0 should be a reality soon anyways so I'm very willing to move forward regardless. |
@jmarcil I agree it would be best to do in a We did something sort of related in the PHPCompatibility standard a while back (for improved compatibility with Composer) and included upgrade instructions mentioning to remove any "hacks" in the changelog and the fall-out of that has been minimal. See: https://github.com/PHPCompatibility/PHPCompatibility/releases/tag/8.0.0 |
Either way, have a look at PR #50 ;-) |
Just out of curiosity: what will be the practical implications of this project moving to OWASP ? And what would be involved in that from a contributor perspective ? |
FloeDesignTechnologies organization is not active anymore, so we simply need a new home for the repo. OWASP can also help with project visibility, long term ownership transfers and funding. I can also see doing that move reviving some sort of documentation initiative for PHP within OWASP. Everybody that submits GitHub PRs will still be contributors as usual and moving to OWASP won't change that. However if people wants to join the OWASP project as a "leader" this gives control over some of the OWASP perks mentioned above and it gives admin access on the repo. That all said, the final decision of proceeding forward with the move to OWASP haven't been made yet. Sorry if all that bureaucracy slows down contributions. #50 looks neat. |
@jmarcil Thanks for your response. So will the repo on GH be removed ? Or moved to another GH organisation ? What does "moving the project" entail in that respect ? I'm also wondering why the (wait for a) decision, as you say, slows down contributions ? Is there any reason not to merge something while the future is unclear ? Either way, I'm interested to see this project becoming more active and more comprehensive as I do believe it can be a valuable addition to the PHPCS ruleset for projects. I also believe there is room for improvement to a number of the sniffs, as well as the CI/unit test process. If nothing comes of the move to OWASP, you'd be welcome to move it to the PHPCSStandards organisation if needs be. Not a real organisation, just a way to group certain PHPCS related projects I'm working on and to make them more easily findable. |
There is no plans of removing the GH repo, preventing any interruption is why I want to move it somewhere else in the first place. The reason not to merge while I'm figuring this out is my own limitations as human being and time management. I can only work on a single thread at a time for this project, sorry 😐. Thank you for the offer to transfer on your organisation, I'll certainly consider it as a viable option. Any ways this goes, I would gladly welcome you as a contributor as you seem one of the most motivated person around this project nowadays. I've created #54 if you want to discuss more as I think we're getting pretty much off topic of PHPCS compliance 😉 |
Respect. As there's no automated CI in place, merging now or later won't make much difference anyway, other than in availability of fixes to end-users. I know from previous experience that if there is CI in place and branches are protected, merging now would generally be better as otherwise the build results often don't get reported properly in the moved repo which would mean that all PRs need to be rebased and rebuild before they can be merged. Either way, not relevant for this repo until CI gets introduced.
Well, I've kept an eye this repo for quite a while now and mentioned it in a number of talks about PHPCS at conferences, but the issue as described above is a blocker for adoption by most standards/projects. I'd happily contribute more in the future once PR #50 is merged, got plenty of ideas, but that's for discussion after #50. |
The way the project is currently set up with the standard name being
Security
, but the namespace name beingPHPCS_SecurityAudit
is non-standard and breaks expectations.It is the cause of issues #38 and #45 and the reason hacks like the symlink are needed, while the normal
installed_paths
functionality in PHPCS will not work for this standard.I'd be willing to fix this for this project, but only if a PR to that effect would be considered welcome.
It will involve renaming all namespaces, removing the symlink and more, but can be done without breaking existing rulesets using
Security.Category.SniffName
references.Would you be willing to consider such a PR ?
The text was updated successfully, but these errors were encountered: