Conversation
Looks Good Ron..:) |
Thanks! I'll try to attend the next meeting and bring this up. |
Thanks @pbiron |
So it sounds like it is outside the scope of this plugin. I am guessing that Ajax is being used when initially installing and activating a plugin. (Atleast it does not jump to the top of the screen when installing/activating a plugin for the first time.) That means on the Plugins -> Install New seems to have Ajax activated. |
Thank you for the POC PR! |
No, it's perfectly within scope for the plugin. The implementation may change when merged into core, but that's the same with a lot of what's in the plugin. |
@paaljoachim @pbiron updated the PR for the themes screen on multisite. I'll do another test on single-site to make sure everything behaves as expected. One thing I noticed is we need some |
Also would be good to abstract out the HTML creation so Ajax and non-Ajax use the same markup. |
This does not work for the themes screen on single-site. I'll be happy to do another PR to get that fixed with this PR as a dependency. Needed:
I'll be happy to discuss at the next open meeting. Regards, Ronald Huereca |
Thanx Ronald! Can you open an issue about other things we could do (e.g., other hooks we could/should add) to help and/or avoid conflicts with existing |
Done. Thank you. #63 |
@paaljoachim @pbiron I updated the PR to cover single-site themes. This is ready for review/scrutiny :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pbiron shall I keep this PR up to date with changes, or wait until the 0.6 milestone to finalize? |
@pbiron this has been updated with the latest 0.5.1 changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On quick inspection things seem to work great. I'll have to go over the code in detail later.
The other thing I notice is that without the dashicon animating while processing is happening it's unclear to a user that anything is happening :-( Yes, the wording changes from Enable auto-updates
to Enabling auto-updates
but that change is so minor I didn't really see it the first couple of times.
I don't really know what to do about that. Maybe one thing would be to change the text to Enabling auto-updates ...
(append ellipsis). Even better would be if the ellipsis could be animated; that is, draw one full-stop
, then two, then 3, then back to 1, then 2, then 3, etc. I have no idea whether that would be possible.
Also see the 3 comments attached to specific lines (e.g., the version arg when enqueueing the JS, and about the blur()
in the on.click
handlers.
Thanks all and particularly @ronalfy for all the great work on this pull request. |
functions.php
Outdated
$nonce = sanitize_text_field( $_POST['nonce'] ); | ||
$type = sanitize_text_field( $_POST['type'] ); | ||
$asset = sanitize_text_field( urldecode( $_POST['asset'] ) ); | ||
if ( ! wp_verify_nonce( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency with other WordPress Ajax functions, like wp_ajax_update_plugin
it might be worth using check_ajax_referer
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TimothyBJacobs Changed with bbc30fd
@TimothyBJacobs @audrasjb @pbiron ready for re-review. Made the CSS changes and removes the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this pull request will need a security audit before merge 🙂
functions.php
Outdated
* Disable auto updates via Ajax. | ||
*/ | ||
function wp_autoupdates_disable_auto_updates() { | ||
$nonce = sanitize_text_field( $_POST['nonce'] ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m a bit worried about using sanitize_text_field
as it's only sanitization. Shouldn't we also escape unwanted HTML bits?
Pinging @whyisjake as WordPress security team maintainer for further consideration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, great idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coding standards want me to use wp_unslash
and then a sanitization before even touching the variable. But I'll let the security team come up with the suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few nits here, overall, looks good.
functions.php
Outdated
|
||
wp_send_json_success( | ||
array( | ||
'enabled_count' => '(' . $enabled_count . ')', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add an absint
here?
'enabled_count' => '(' . $enabled_count . ')', | |
'enabled_count' => '(' . absint( $enabled_count ) . ')', |
functions.php
Outdated
|
||
wp_send_json_success( | ||
array( | ||
'enabled_count' => '(' . $enabled_count . ')', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add an absint
here too.
'enabled_count' => '(' . $enabled_count . ')', | |
'enabled_count' => '(' . absint( $enabled_count ) . ')', |
functions.php
Outdated
array( | ||
'enabled_count' => '(' . $enabled_count . ')', | ||
'disabled_count' => '(' . absint( count( $all_plugins ) - $enabled_count ) . ')', | ||
'return_html' => $return_html, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if we should add another esc_
function here. I can see the parts above look great, but...
functions.php
Outdated
array( | ||
'enabled_count' => '(' . $enabled_count . ')', | ||
'disabled_count' => '(' . absint( count( $all_themes ) - $enabled_count ) . ')', | ||
'return_html' => $return_html, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment here as above about thinking about late escaping.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@whyisjake is wp_kses_post
sufficient for late escaping of the HTML, or do you want to be stricter than that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See af2480b please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WFM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are some WPCS related things that should be addressed at some point, but don't think those need to be done before this is merged. So, once the others sign off, I think it's good to go.
💥 |
Resolves #65
This is a concept PR that demonstrates Ajax enabling/disabling of plugins. Would love to discuss this more and flesh it out more for themes as well if this concept is accepted.
Thanks,
Ronald Huereca