WordPress VIP coding standards compliance #1111
Conversation
There was a problem hiding this comment.
This looks great @kidunot89!
Having reviewed the live-updates feature and the way it uses request variables I'm thinking if we should pull the feature from the current release or somehow reduce the amount of request variables we're passing to the JS side via JSON in DOM.
classes/class-admin.php
Outdated
| // input var okay, CSRF okay | ||
| 'current_query_count' => count( $_GET ), // WPCS: CSRF ok. | ||
| // input var okay, CSRF okay | ||
| 'current_page' => isset( $_GET['paged'] ) ? esc_js( $_GET['paged'] ) : '1', // phpcs:ignore WordPress.Security.NonceVerification.Recommended |
There was a problem hiding this comment.
Should this be sanitized to an integer value?
There was a problem hiding this comment.
absint( wp_unslash( $_GET['paged] ) ) would probably be my preference here, but there are many ways to sanitize/escape this.
There was a problem hiding this comment.
I gotcha, I wasn't worried about the code and was just trying to silence the PHPCS warning.
classes/class-admin.php
Outdated
| 'current_query_count' => count( $_GET ), // WPCS: CSRF ok. | ||
| // input var okay, CSRF okay | ||
| 'current_page' => isset( $_GET['paged'] ) ? esc_js( $_GET['paged'] ) : '1', // phpcs:ignore WordPress.Security.NonceVerification.Recommended | ||
| 'current_order' => isset( $_GET['order'] ) ? esc_js( $_GET['order'] ) : 'desc', // phpcs:ignore WordPress.Security.NonceVerification.Recommended |
There was a problem hiding this comment.
Could we check that it's one of the allowed values of order before setting it? Do we support anything apart from desc and asc?
There was a problem hiding this comment.
I agree, if there are only asc and desc we can do a check for one and manually set this instead of excepting any value.
kidunot89
force-pushed
the
devops/vip-compliance
branch
from
39d8bcb to
ee379e1
Compare
kidunot89
changed the title
| @@ -154,7 +154,7 @@ function ( $var ) { | |||
| $result = $this->plugin->db->insert( $recordarr ); | |||
|
|
|||
| // This is helpful in development environments: | |||
| // error_log( $this->debug_backtrace( $recordarr ) ); | |||
| // error_log( $this->debug_backtrace( $recordarr ) );. | |||
There was a problem hiding this comment.
Could we remove this completely?
|
Looks like need to resolve the merge conflict. @kidunot89 could you do that, please? |
kidunot89
force-pushed
the
devops/vip-compliance
branch
from
96dbebb to
6458a3b
Compare
| @@ -23,6 +23,6 @@ | |||
| <exclude-pattern>*/ui/lib/*</exclude-pattern> | |||
| <exclude-pattern>*/vendor/*</exclude-pattern> | |||
| <exclude-pattern>*/build/*</exclude-pattern> | |||
| <exclude-pattern>*/local/public/*</exclude-pattern> | |||
| <exclude-pattern>*/local/*</exclude-pattern> | |||
There was a problem hiding this comment.
Since it only used is in development, I've added the whole local directory to be ignored by PHPCS.
There was a problem hiding this comment.
There was a problem hiding this comment.
It would be nice to have all PHP files with consistent formatting even if they're used for development purposes.
There was a problem hiding this comment.
Looks good @kidunot89!
Could we please create issues for all instances of phpcs:ignore to add the nonce checks?
Summary
Adds the VIP coding standards and clears up all the warnings from the PHPCS test.