Cleanup #119
Cleanup #119
Conversation
Also fix/reorder error messages for easier translation, remove some unneeded escaping, and other minor cleanup.
…he "name" of Ajax action to kebab-case to be more inline with core conventions.
| @@ -1299,63 +1300,60 @@ function wp_autoupdates_toggle_auto_updates() { | |||
| wp_send_json_error( array( 'error' => __( 'Invalid data. No selected item.', 'wp-autoupdates' ) ) ); | |||
| } | |||
|
|
|||
| $type = sanitize_text_field( $_POST['type'] ); | |||
There was a problem hiding this comment.
I believe the sanitizing was at the request of @whyisjake . If he's OK with these no longer being sanitized, then rest looks good to me.
@azaozz I just pushed an additional commit with 2 other minor changes, so be sure to check those.
There was a problem hiding this comment.
Right. All strings coming from the user, ($_GET, $_POST, $_REQUEST, many from $_SERVER, etc.) must be sanitized. However strings that must match a particular, limited example can be matched directly. In this case $_POST['state'] can only be enable or disable, all other strings there are ignored. Then a direct match makes more sense as it's faster.
Similarly when a number is expected, it would be enough to cast to int or do intval() (with some limitations), instead of first sanitizing the string.
Also, further simplification of Ajax, as well as only clearing the "time to next auto-update" text when a manual update succeeds (instead of when that link is clicked).
functions.php
Outdated
| if ( in_array( $plugin, $wp_auto_update_plugins, true ) ) { | ||
| $wp_auto_update_plugins = array_diff( $wp_auto_update_plugins, array( $plugin ) ); | ||
| $action_type = 'disable-auto-update=true'; | ||
| } else { | ||
| $wp_auto_update_plugins[] = $plugin; | ||
| $wp_auto_update_plugins = array_unique( $wp_auto_update_plugins ); | ||
| $action_type = 'enable-auto-update=true'; | ||
| } |
There was a problem hiding this comment.
This doesn't sound right. The "user action" (enable or disable) should always be explicit. Toggling the setting may result in unwanted changes, edge cases, etc. Example: the user opens two tabs and clicks "enable" on both. Will the end result be "enabled" or "disabled"?
There was a problem hiding this comment.
OK, let me do another push that restores the old code for that part. Give me just a few minutes
functions.php
Outdated
| if ( in_array( $theme, $wp_auto_update_themes, true ) ) { | ||
| $wp_auto_update_themes = array_diff( $wp_auto_update_themes, array( $theme ) ); | ||
| $action_type = 'disable-auto-update=true'; | ||
| } else { | ||
| $wp_auto_update_themes[] = $theme; | ||
| $wp_auto_update_themes = array_unique( $wp_auto_update_themes ); | ||
| $action_type = 'enable-auto-update=true'; | ||
| } |
There was a problem hiding this comment.
Same as a7abdcd#r421918954.
functions.php
Outdated
| $plugin = ! empty( esc_html( $_GET['plugin'] ) ) ? wp_unslash( esc_html( $_GET['plugin'] ) ) : ''; | ||
| $page = isset( $_GET['paged'] ) && ! empty( esc_html( $_GET['paged'] ) ) ? wp_unslash( esc_html( $_GET['paged'] ) ) : ''; | ||
| $status = isset( $_GET['plugin_status'] ) && ! empty( esc_html( $_GET['plugin_status'] ) ) ? wp_unslash( esc_html( $_GET['plugin_status'] ) ) : ''; | ||
| $s = isset( $_GET['s'] ) && ! empty( esc_html( $_GET['s'] ) ) ? wp_unslash( esc_html( $_GET['s'] ) ) : ''; |
There was a problem hiding this comment.
If I'm reading it right this would be needed only for the plugin. Still, the $page would always be numeric, right? Sanitization would be (int) $_GET['paged'], no need of string sanitization.
There was a problem hiding this comment.
correct, only in the plugin. And yes, casting to int is sufficient. That sanitation has been there from the beginning and I hadn't gotten around to removing it based on your previous comments.
functions.php
Outdated
| if ( ! current_user_can( 'update_plugins' ) || ! wp_autoupdates_is_plugins_auto_update_enabled() ) { | ||
| wp_die( __( 'Sorry, you are not allowed to enable plugins automatic updates.', 'wp-autoupdates' ) ); | ||
| } | ||
| if ( ! ( isset( $_GET['action'] ) && 'toggle-auto-update' === $_GET['action'] ) ) { |
There was a problem hiding this comment.
Same as a7abdcd#r421918954.
"Toggle" is not suitable, the action has to be explicit, either "enable" or "disable".
Also some other minor cleanup.