Use capabilities not roles #36
Conversation
|
Sorry, not quite sure what I am doing yet. Can you help me out jrf or grapplerulrich |
|
@khacoder You don't need to close the PR. We can keep it open and work on it. What do you need help with? Ping me on slack. |
grappler
force-pushed
the
feature/theme-review-sniffs
branch
2 times, most recently
from
563cd1d
to
6671823
Compare
|
When merged, solves #27 |
grappler
added
pr: needs update
pr: needs review
and removed
pr: wait for upstream
labels
| * Test file: UseCapabilitiesNotRoles Sniff. | ||
| */ | ||
|
|
||
| add_dashboard_page( 'page_title' , 'menu_title' , 'super_admin' , 'menu_slug' , 'function'); // ERROR. |
There was a problem hiding this comment.
Not important, but the Error/Warning comment should be in normal sentence case, no need for all caps.
| @@ -0,0 +1,41 @@ | |||
| <?php | |||
| /** | |||
| * Test file: UseCapabilitiesNotRoles Sniff. | |||
There was a problem hiding this comment.
This comment is redundant as it is linked to the sniff by the directory and file name.
| * | ||
| * @var array Capabilities available in core. | ||
| */ | ||
| protected $core_capabilities = array( |
There was a problem hiding this comment.
I'm missing the unfiltered_upload capability. Not that it's used much, but it is a valid capability.
| /** | ||
| * Array of core capabilities. | ||
| * | ||
| * @since 0.xx.0 |
There was a problem hiding this comment.
Please add a note on what logic has been used to order this list.
You may also want to include a link to the core handbook and/or codex which lists these capabilities to make it easier in the future to keep the list up to date.
| 'manage_network_users' => true, | ||
| 'manage_network_plugins' => true, | ||
| 'manage_network_themes' => true, | ||
| 'manage_network_options' => true, |
There was a problem hiding this comment.
Line 91-98 is duplicate of line 83-90
Also towards the bottom of the list there is a whole range of additional duplicates for things like edit_posts, delete_posts etc
There was a problem hiding this comment.
Ah, yes the list was not very good. I have updated it again and it should be good now.
| $this->phpcsFile->addWarning( | ||
| 'The parameter %s is an unknown role or capability. Check %s() to ensure it is a capability and not a role.', | ||
| $stackPtr, | ||
| 'PossibleRoleFound', |
There was a problem hiding this comment.
Suggested alternative: UnknownCapabilityFound
|
|
||
| /** | ||
| * Array of core capabilities. | ||
| * |
There was a problem hiding this comment.
Please document that these are the valid values.
|
|
||
| /** | ||
| * Array of core roles. | ||
| * |
There was a problem hiding this comment.
Please document that it is undesirable to use these.
In that respect, I would actually reverse the property order in the file. core_capabilities first, core_roles second.
| * | ||
| * @var array Function name with parameter position. | ||
| */ | ||
| protected $target_functions = array( |
There was a problem hiding this comment.
Is there any logic to the function order ? I can see they are semi-grouped, but the groups are not annotated, nor are the functions within a group ordered by any logical order.
There was a problem hiding this comment.
I have added a bit more documentation.
| 'author_can' => 2, | ||
| 'current_user_can_for_blog' => 2, | ||
| ); | ||
|
|
There was a problem hiding this comment.
At a later stage, this sniff should probably also check for certain class methods, like WP_User::has_cap().
Might be useful to add a @todo note in the class comment about this or to open an issue for it as a reminder.
|
I am happy to make a PR upstream. I will work on here for the time being till it is fully ready. |
|
@grappler Okidoki. let me know when you're ready for me to have another look. |
|
I will update it for PHPCS 3.x and push the PR upstream. |
|
Once this has been pulled upstream, this PR can be closed and a new PR will need to be opened just to add the new upstream sniff to the ruleset. |
|
I would like to finish this one if I could please. |
|
@khacoder FYI: @grappler has actually already pulled an updated version to WPCS upstream which includes your original commit for the credits. This PR will be closed in favour of adding the upstream WPCS version to the ruleset once that PR has been merged, so there is nothing here at this moment to continue with. If you like, you can look over the upstream PR & give feedback there: WordPress/WordPress-Coding-Standards#1364 |
|
OK no problem, you may as well close this one?? |
|
For history: This PR was closed when we replaced the master/develop branches with the re-organized stand-alone repo. It will not be re-opened for the following reasons:
|
No description provided.