Conform to VIP Go Coding Standards

[akrawiec]

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/

[justintadlock]

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.

[justintadlock]

If there's a significant issue, of course, I'll put out an update for the 2.x branch.

[akrawiec]

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

[justintadlock]

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.

[akrawiec]

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

[justintadlock]

Reopening because there are things here that I want to address in 3.0.