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

Add sniff that will check that capabilities are used correctly. #2112

Merged
merged 1 commit into from
Dec 2, 2022

Conversation

dingo-d
Copy link
Member

@dingo-d dingo-d commented Nov 25, 2022

The sniff will check if the functions that are accepting capabilities as the argument actually use capabilities (as described in the support article) and not roles.

The sniff was rewritten from the original one in #1364 to include helper functions from PHPCSUtils. The list of core functions that are using capabilities as a parameter was updated with changes up to WordPress 6.1.0. The list of capabilities was updated as well. The sniff is also compatible with PHP 8 (named arguments).

Original PRs:
#1364
WPTT/WPThemeReview#36

Ref:
https://make.wordpress.org/themes/handbook/review/required/#10-classic-themes
https://developer.wordpress.org/plugins/users/roles-and-capabilities/
https://developer.wordpress.org/plugins/security/checking-user-capabilities/

Closes: #1364

@dingo-d dingo-d added Component: Extra Type: Enhancement Status: Awaiting feedback Focus: WP interoperability Doing things the WP way, prefer using WP functionality over userland/PHP native labels Nov 25, 2022
@dingo-d dingo-d self-assigned this Nov 25, 2022
@jrfnl
Copy link
Member

jrfnl commented Nov 25, 2022

One remark: as part of working with @dingo-d on this sniff, I updated the list of known capabilities with anything I could find in Core. I would very much like to invite someone to very critically review that list as there is a risk that I may have added non-officially supported capabilities.

Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

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

Looking great! Just some final touches and then we can get this merged (after ? years... ) 🎉

WordPress-Extra/ruleset.xml Outdated Show resolved Hide resolved
WordPress/Docs/WP/CapabilitiesStandard.xml Show resolved Hide resolved
WordPress/Docs/WP/CapabilitiesStandard.xml Outdated Show resolved Hide resolved
WordPress/Docs/WP/CapabilitiesStandard.xml Show resolved Hide resolved
WordPress/Docs/WP/CapabilitiesStandard.xml Outdated Show resolved Hide resolved
WordPress/Sniffs/WP/CapabilitiesSniff.php Outdated Show resolved Hide resolved
WordPress/Tests/WP/CapabilitiesUnitTest.inc Outdated Show resolved Hide resolved
WordPress/Tests/WP/CapabilitiesUnitTest.inc Outdated Show resolved Hide resolved
WordPress/Tests/WP/CapabilitiesUnitTest.inc Outdated Show resolved Hide resolved
WordPress/Tests/WP/CapabilitiesUnitTest.php Outdated Show resolved Hide resolved
@dingo-d dingo-d added this to the 3.0.0 milestone Nov 25, 2022
@dingo-d dingo-d force-pushed the grappler/feature/36-use-capabilities-not-roles branch from ef579fc to e7ffcad Compare November 25, 2022 16:46
@dingo-d
Copy link
Member Author

dingo-d commented Nov 25, 2022

Blocked by: #2114. Once that PR is merged I will rebase and the tests should be passing (tests are passing because the fix was applied here, but it shouldn't be committed in this PR).

@dingo-d dingo-d force-pushed the grappler/feature/36-use-capabilities-not-roles branch 3 times, most recently from c22e353 to cb68657 Compare November 26, 2022 15:34
@dingo-d dingo-d requested review from jrfnl and GaryJones November 26, 2022 15:36
Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

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

Thanks @dingo-d for working on this and making all those changes!

Went through it one last time with a fine toothcomb:

  • Ran PHPCS with --generator=text to see the CLI docs output.
  • Ran PHPCS against the test files to verify the error messages and codes.
  • Re-ran PHPCS against the test files with a custom ruleset with the <config name="minimum_supported_wp_version" value="2.9"/> directive set.
  • Re-ran PHPCS against the test files using the CLI --runtime-set minimum_supported_wp_version 2.9 arg.
  • Ran the tests.

These final remarks are mostly just small peanuts, dotting the i's and crossing the t's. The logic of the sniff is sound and doesn't need any changes anymore, the tests are good and comprehensive, the docs are clear. We're nearly there.

WordPress/Tests/WP/CapabilitiesUnitTest.php Outdated Show resolved Hide resolved
WordPress/Docs/WP/CapabilitiesStandard.xml Outdated Show resolved Hide resolved
WordPress/Tests/WP/CapabilitiesUnitTest.1.inc Outdated Show resolved Hide resolved
WordPress/Tests/WP/CapabilitiesUnitTest.1.inc Outdated Show resolved Hide resolved
WordPress/Tests/WP/CapabilitiesUnitTest.1.inc Outdated Show resolved Hide resolved
WordPress/Sniffs/WP/CapabilitiesSniff.php Outdated Show resolved Hide resolved
WordPress/Sniffs/WP/CapabilitiesSniff.php Outdated Show resolved Hide resolved
WordPress/Sniffs/WP/CapabilitiesSniff.php Outdated Show resolved Hide resolved
WordPress/Sniffs/WP/CapabilitiesSniff.php Outdated Show resolved Hide resolved
WordPress/Sniffs/WP/CapabilitiesSniff.php Outdated Show resolved Hide resolved
@dingo-d dingo-d force-pushed the grappler/feature/36-use-capabilities-not-roles branch 2 times, most recently from 33f6c24 to f3930af Compare November 28, 2022 19:32
@dingo-d dingo-d requested a review from jrfnl November 28, 2022 19:33
Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

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

I would very much like to invite someone to very critically review that list as there is a risk that I may have added non-officially supported capabilities.

The review of the capabilities list is IMO not a blocker for merging, but would still be very welcome!

@sandeshjangam Would this ☝🏻 be something you'd be interested in helping us with ?


Other than the above remark from earlier on, everything looks to have been addressed.

Still saw three inconsistencies, so I've fixed those in a new commit (so you all can see easily what I changed). Feel free to squash on (or before) merge.

Approved (at long last). 🎉

@jrfnl jrfnl force-pushed the grappler/feature/36-use-capabilities-not-roles branch from 2b90ee4 to 1ff7cf6 Compare November 29, 2022 00:26
Copy link
Member

@GaryJones GaryJones left a comment

Choose a reason for hiding this comment

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

The sniff will check if the functions that are accepting capabilities as the argument actually use capabilities and not roles.
It was rewritten to include helper functions from PHPCSUtils, cleanup code, and add additional tests.
The list of core functions that are using capabilities as a parameter was updated with changes up to WordPress 6.1.0,
as well as the list of capabilities.
The sniff is also compatible with PHP 8 (named arguments).

Co-authored-by: Juliette <[email protected]>
Co-authored-by: Ulrich Pogson <[email protected]>
Co-authored-by: Kevin Haig <[email protected]>
Co-authored-by: Gary Jones <[email protected]>
@dingo-d
Copy link
Member Author

dingo-d commented Dec 2, 2022

I agree with the added changes 👍🏼

@jrfnl jrfnl force-pushed the grappler/feature/36-use-capabilities-not-roles branch from 1ff7cf6 to a9d9be3 Compare December 2, 2022 09:28
@jrfnl
Copy link
Member

jrfnl commented Dec 2, 2022

Rebased without changes and squashed the extra commit into the original. Will merge once the build has passed.

@jrfnl jrfnl enabled auto-merge December 2, 2022 09:29
@jrfnl jrfnl merged commit 80115b2 into develop Dec 2, 2022
@jrfnl jrfnl deleted the grappler/feature/36-use-capabilities-not-roles branch December 2, 2022 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Extra Focus: WP interoperability Doing things the WP way, prefer using WP functionality over userland/PHP native Type: Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants