More pre-merge cleanup #124
Conversation
…n core. Also adds auto-update info to the parent theme (if any) and adds a comma before the debug strings.
Also applies the filters inline, rather than in function calls.
There was a problem hiding this comment.
Great work @pbiron. And thanks for adding all those inline comment, it really helps to be sure to not forget anything for the core patch.
Also, I second the approach you used in wp_autoupdates_notices. I was wondering about using something like this when I added this function at the beginning of the project, but I think it's definitely better to fit to core.
Thanks
|
I probably have a few more commits to this PR coming. |
|
|
||
| update_site_option( 'wp_auto_update_plugins', $auto_updates ); | ||
|
|
||
| wp_redirect( self_admin_url( "plugins.php?disable-auto-update=true&plugin_status=$status&paged=$page&s=$s" ) ); |
There was a problem hiding this comment.
Just a nitpick but when concatenating "inline", it is usually more readable to wrap the variables in {}. Then the above would be:
"plugins.php?disable-auto-update=true&plugin_status={$status}&paged={$page}&s={$s}"
At the same time, this can be done "the WP way" by passing an array to add_query_arg() too :)
There was a problem hiding this comment.
I agree 100%.
However, the string is exactly how all of the equivalent ones in wp-admin/plugins.php are constructed, e.g, https://core.trac.wordpress.org/browser/trunk/src/wp-admin/plugins.php#L38.
When creating the core merge patch I've been trying to do equivalent things in exactly the same way as existing core code (which is why the way the redirect URL is constructed in wp_autoupdates_handle_plugins_enable_disable() is different than in wp_autoupdates_handle_themes_enable_disable()`).
There was a problem hiding this comment.
I've been trying to do equivalent things in exactly the same way as existing core code
Yeah, I understand. But not all of the core code is necessarily written in the best way possible. There's always some room for improvement. Perhaps after merging this, we can open a ticket and fix the old core code too? Looking there, most of these were added over 11 years ago (https://core.trac.wordpress.org/changeset/11029), that's in the PHP 4.x era :)
In any case, that's just a nitpick, not a blocker.
There was a problem hiding this comment.
Agreed, opening a ticket after merge was going to be my suggestion as well.
Remember that conversation we were having in Slack about inconsistencies between list tables? It also applies to the "driver" files for the screens on which list tables appear :-(
…ated closer to what it will be in core.
functions.php
Outdated
| $theme['actions']['autoupdate'] = current_user_can( 'update_themes' ) | ||
| ? wp_nonce_url( admin_url( sprintf( 'themes.php?action=%s&theme=%s', $auto_update ? 'disable-auto-update' : 'enable-auto-update', urlencode( $slug ) ) ), 'updates' ) | ||
| : null; |
There was a problem hiding this comment.
Just a nitpick but for readability's sake nested ternaries should be avoided as much as possible (this is mentioned in the WP coding style as overly "clever code"). The above three lines would probably be more readable/easier to comprehend if they were in an if - else block or part of it was defined in a variable before adding to the rest.
There was a problem hiding this comment.
Also agree here. I struggled with that line.
Defining the action string in a variable is closer to the equiv code in core, will push another commit with that change.
…@data-wp-action.
Also adds comments of where each rule is be in core.
…action explicit, instead of with string concatenation.
…e themes screen. And that's the last commit. I promise.
This is another "pre-core merge" PR. It adjusts things so that the code in the plugin is much closer to what it will be in core.
This will make it easier to produce the core patch and/or to "backport" to the plugin changes from core after the initial commit.