Conform to VIP Go Coding Standards
Opened this issue · 6 comments
We're looking to use this plugin on VIP Go, and after Automattic/VIP review, there are several places where the code needs to be updated to conform to their standards.
A subsequent PR will provide updates to follow the code to address Blockers, Proceed With Caution, and Performance Blockers: https://lobby.vip.wordpress.com/wordpress-com-documentation/code-review-what-we-look-for/
Members is currently undergoing a major rewrite for 3.0. So, any PRs for the 2.0 branch likely won't get merged. I'll be happy to look over any code that might need to be changed and seeing how that might fit into 3.0.
If there's a significant issue, of course, I'll put out an update for the 2.x branch.
Thanks for the quick update. VIP has provided the feedback already, which I'll include here for you to look over. We've made the majority of the changes already though, if that makes it easier for you:
—Adam
Admin/tmpl/cap-control.php
————
L20, 21, 25, 26 - We should use double curly brackets for escaping via Moustache
admin/class-cap-tabs.php
————
L224 - We should escape class $icon with esc_attr
admin/class-manage-roles.php
————
L98 - We recommend not to use $_REQUEST due to it’s ambiguity, see: https://lobby.vip.wordpress.com/wordpress-com-documentation/code-review-what-we-look-for/#using-_REQUEST
admin/class-manage-users.php
————
L126, 130 - We recommend not to use $_REQUEST due to it’s ambiguity, see: https://lobby.vip.wordpress.com/wordpress-com-documentation/code-review-what-we-look-for/#using-_REQUEST
L140, 248, 262, 279 - We recommend to use in_array with a strict parameter, see: https://lobby.vip.wordpress.com/wordpress-com-documentation/code-review-what-we-look-for/#in_array-without-strict-parameter
L181, 284 - We recommend to use wp_safe_redirect over wp_redirect followed with an exit(), see https://lobby.vip.wordpress.com/wordpress-com-documentation/code-review-what-we-look-for/#wp_safe_redirect-instead-of-wp_redirect
L247 - We highly recommend to use strict comparison operators, please see https://lobby.vip.wordpress.com/wordpress-com-documentation/code-review-what-we-look-for/#strict-comparisons-equals
L302 - We recommend escaping $notice[‘message’] with esc_html
admin/class-role-list-table.php
————
L208, 231 - We highly recommend to use strict comparison operators, please see https://lobby.vip.wordpress.com/wordpress-com-documentation/code-review-what-we-look-for/#strict-comparisons-equals
L245 - We should late escape $label with esc_url
Admin/class-role-edit.php
————
L144, 145, 171, 184 - We highly recommend using strict comparisons when using in_array, please see: https://lobby.vip.wordpress.com/wordpress-com-documentation/code-review-what-we-look-for/#in_array-without-strict-parameter
L313 - We highly recommend to use strict comparison operators, please see https://lobby.vip.wordpress.com/wordpress-com-documentation/code-review-what-we-look-for/#strict-comparisons-equals
admin/class-role-new.php
————
L221 - We recommend to use wp_safe_redirect over wp_redirect, see: https://lobby.vip.wordpress.com/wordpress-com-documentation/code-review-what-we-look-for/#wp_safe_redirect-instead-of-wp_redirect
L334 - We highly recommend to escape the value with esc_attr
Admin/class-user-edit.php
————
L107 - We highly recommend using strict comparisons when using in_array, please see: https://lobby.vip.wordpress.com/wordpress-com-documentation/code-review-what-we-look-for/#in_array-without-strict-parameter
L144 - Leaving out commented code is not recommended, see: https://lobby.vip.wordpress.com/wordpress-com-documentation/code-review-what-we-look-for/#commented-out-debug-code
Admin/class-user-new.php
————
L119, 168, 176 - We highly recommend using strict comparisons when using in_array, please see: https://lobby.vip.wordpress.com/wordpress-com-documentation/code-review-what-we-look-for/#in_array-without-strict-parameter
admin/class-meta-box-content-permissions.php
————
L288 - We highly recommend to use strict comparison operators, please see https://lobby.vip.wordpress.com/wordpress-com-documentation/code-review-what-we-look-for/#strict-comparisons-equals
admin/class-roles.php
————
L87 - We recommend not to use $_REQUEST due to it’s ambiguity, see: https://lobby.vip.wordpress.com/wordpress-com-documentation/code-review-what-we-look-for/#using-_REQUEST
L90 - We highly recommend to use strict comparison operators, please see https://lobby.vip.wordpress.com/wordpress-com-documentation/code-review-what-we-look-for/#strict-comparisons-equals
admin/class-settings.php
————
L309 - We highly recommend to escape $class with esc_attr
admin/class-meta-box-publish-role.php
————
L101, 107, 113 - We recommend sanitizing output with absent since an int is expected before running it through number_format_i18n
Admin/functions-addons.php
————
L37 - We should be using vip_safe_wp_remote_get() instead of wp_remote_get(), please see https://lobby.vip.wordpress.com/wordpress-com-documentation/fetching-remote-data/
admin/functions-admin.php
————
L87 - We highly recommend to use strict comparison operators, please see https://lobby.vip.wordpress.com/wordpress-com-documentation/code-review-what-we-look-for/#strict-comparisons-equals
L127 - We recommend caching any direct database calls, see https://lobby.vip.wordpress.com/wordpress-com-documentation/code-review-what-we-look-for/#direct-db-queries
Inc/Class-widget-login.php
————
L123 - We should escape incase it was inputted by user
L209, 210, 213, 214, 217, 218, 221, 222, 225, 226, 233, 234, 237, 238, 241, 242, 245, 246, 249, 250, 259, 265, 271, 276, 277, 281, 282 - We recommend escaping the attributes
Inc/class-widget-users.php
————
L105 - We should escape incase it was inputted by user
L108 - We recommend adding caching to get_users as this is an intensive function
L208, 209, 212, 213, 220, 221, 228, 229, 237, 238, 241, 242, 249, 250, 253, 254, 257, 258, 261, 262, 269, 270 - We recommend escaping the attributes
Inc/functions-cap-groups.php
————
L79 - We should sanitize all input before registering the post type cap group
L237 - We highly recommend using strict comparisons when using in_array, please see: https://lobby.vip.wordpress.com/wordpress-com-documentation/code-review-what-we-look-for/#in_array-without-strict-parameter
inc/functions-content-permissions.php
————
L105, 113 - https://lobby.vip.wordpress.com/wordpress-com-documentation/code-review-what-we-look-for/#in_array-without-strict-parameter
L230 - Let’s escape $message
inc/functions-private-site.php
————
L196 - We should escape $blogname
Inc/functions-roles.php
————
L285, 366, 379 - We recommend to use in_array with a strict parameter, see: https://lobby.vip.wordpress.com/wordpress-com-documentation/code-review-what-we-look-for/#in_array-without-strict-parameter
L406, 431, 444, 457 - We should escape the URL and run the input values through rawlurlencode, please see: https://vip.wordpress.com/documentation/encode-values-passed-to-add_query_arg/
L472 - We should run the input values through rawlurlencode, see: https://vip.wordpress.com/documentation/encode-values-passed-to-add_query_arg/
Inc/functions-shortcodes.php
————
L184, 187, 197, 200 - We recommend to use in_array with a strict parameter, see: https://lobby.vip.wordpress.com/wordpress-com-documentation/code-review-what-we-look-for/#in_array-without-strict-parameter
Inc/template.php
————
L70 - We highly recommend to use strict comparison operators, please see https://lobby.vip.wordpress.com/wordpress-com-documentation/code-review-what-we-look-for/#strict-comparisons-equals
Js/edit-role.js
————
L377 & 378 - we recommend creating the string programmatically instead of via concatenation
None of the "lobby" links are available to me. They're private pages.
With a quick scan of about a couple dozen of those, I'm seeing a good mix of things I'll probably address and some things that are probably going to be a hard no (I won't change). Other things are trivial code-styling things. For those, I'll look at them on a case-by-case basis.
It seems to me that this report was simply generated by a tool rather than having a human look over the results. I'm already seeing a few false-positives.
I'm also seeing a recommendation to use a vip_*() function, which will definitely not be added to the plugin since I write code for WordPress and not VIP. :)
Thanks for sharing the report. It'll definitely be useful as I work through the 3.0 update. Lots of good feedback there.
I do want to be candid about not changing all of the above things and to avoid setting up any expectations.
Thanks Justin for taking a look. Hopefully, you can work in some of the recommendations into the next release, but it looks like we'll have to maintain our own fork of the plugin to ensure VIP compatibility/compliance. Cheers. —AK
Reopening because there are things here that I want to address in 3.0.