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

Fix compliance of project with PHPCS #47

Closed
jrfnl opened this issue Jan 13, 2020 · 10 comments · Fixed by #50
Closed

Fix compliance of project with PHPCS #47

jrfnl opened this issue Jan 13, 2020 · 10 comments · Fixed by #50

Comments

@jrfnl
Copy link
Contributor

jrfnl commented Jan 13, 2020

The way the project is currently set up with the standard name being Security, but the namespace name being PHPCS_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 ?

@jrfnl
Copy link
Contributor Author

jrfnl commented Jan 27, 2020

Just wondering if anyone had given this any thought yet ?

@jmarcil
Copy link
Collaborator

jmarcil commented Feb 2, 2020

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).

@jmarcil
Copy link
Collaborator

jmarcil commented Feb 2, 2020

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.

@jrfnl
Copy link
Contributor Author

jrfnl commented Feb 3, 2020

@jmarcil I agree it would be best to do in a 3.0 release as, generally speaking, people will pay more attention to the changelog for a major release.

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

@jrfnl
Copy link
Contributor Author

jrfnl commented Feb 3, 2020

Either way, have a look at PR #50 ;-)

@jrfnl
Copy link
Contributor Author

jrfnl commented Feb 4, 2020

Note that the actual release with 3.0 will also have to coincide with moving this into an OWASP project.

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 ?

@jmarcil
Copy link
Collaborator

jmarcil commented Feb 5, 2020

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.

@jrfnl
Copy link
Contributor Author

jrfnl commented Feb 5, 2020

@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.
Documentation is lacking a bit at the moment for the repos, but I'm working on that.

@jmarcil
Copy link
Collaborator

jmarcil commented Feb 7, 2020

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 😉

@jrfnl
Copy link
Contributor Author

jrfnl commented Feb 7, 2020

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

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.

I would gladly welcome you as a contributor

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.

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