Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add button into Site Health to reenable CSS transient caching #4522

Merged
merged 30 commits into from
Apr 10, 2020
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
13359f8
Add button into Site Health to reenable CSS transient caching
schlessera Apr 3, 2020
c19c1ac
Use wp_jsonencode() instead of addslashes()
schlessera Apr 4, 2020
7b56dd1
Remove unneeded PHPCS exception
schlessera Apr 4, 2020
d1e2aa2
Use wp.ajax and heredoc for JS script
schlessera Apr 4, 2020
cc2bd07
Update styling
schlessera Apr 4, 2020
e820e65
Add capability check
schlessera Apr 4, 2020
8869d99
Use current_user_can()
schlessera Apr 4, 2020
24fa804
Add CSS transient caching option to options manager validation
schlessera Apr 4, 2020
ce1682f
Fix code style issues
schlessera Apr 4, 2020
a258998
Only read Option::DISABLE_CSS_TRANSIENT_CACHING if exists
schlessera Apr 6, 2020
3ae8fed
Remove <script> tags from JS code passed to wp_add_inline_script()
schlessera Apr 7, 2020
41bf7a9
Remove pesky 2nd blank line
schlessera Apr 9, 2020
61f7d8b
Rename AjaxAction to ReenableCssTransientCachingAjaxAction
schlessera Apr 9, 2020
25ca0c8
Remove $access and default to authenticated users only
schlessera Apr 9, 2020
caea4dd
Remove $scope and default to admin backend only
schlessera Apr 9, 2020
fcaeffa
Remove $action and hardcode as const
schlessera Apr 9, 2020
d60da1e
Remove $callback and move method into AjaxAction
schlessera Apr 9, 2020
50e1ca1
Remove $selector and hardcode as const
schlessera Apr 9, 2020
f0e1db0
Only register AJAX action on site-health.php screen
schlessera Apr 9, 2020
9c41211
Use tabs instead of spaces to indent CSS
schlessera Apr 9, 2020
66a7b73
Escape button label translations
schlessera Apr 9, 2020
de1038c
Add nonce verification
schlessera Apr 9, 2020
f94dbf3
Disable button after click
schlessera Apr 9, 2020
5cbe86b
Use DOMContentLoaded instead of a timeout
schlessera Apr 9, 2020
712c1f2
Indent JS code with tabs instead of spaces
schlessera Apr 9, 2020
a52b391
Document $hook_suffix argument
schlessera Apr 9, 2020
daa92ca
Merge branch 'develop' of github.com:ampproject/amp-wp into add-css-c…
westonruter Apr 10, 2020
b9a1eb1
Flesh out guidance for CSS transient caching
westonruter Apr 10, 2020
ff9a37e
Avoid string interpolation for JS injection to better ensure late-esc…
westonruter Apr 10, 2020
a26112b
Use nowdoc instead of heredoc
westonruter Apr 10, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions includes/options/class-amp-options-manager.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
* @package AMP
*/

use AmpProject\AmpWP\Option;

/**
* Class AMP_Options_Manager
*/
Expand Down Expand Up @@ -262,6 +264,12 @@ public static function validate_options( $new_options ) {
}
}

if ( array_key_exists( Option::DISABLE_CSS_TRANSIENT_CACHING, $new_options ) && true === $new_options[ Option::DISABLE_CSS_TRANSIENT_CACHING ] ) {
$options[ Option::DISABLE_CSS_TRANSIENT_CACHING ] = true;
} else {
unset( $options[ Option::DISABLE_CSS_TRANSIENT_CACHING ] );
}

// Store the current version with the options so we know the format.
$options['version'] = AMP__VERSION;

Expand Down
227 changes: 227 additions & 0 deletions src/Admin/AjaxAction.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,227 @@
<?php
/**
* Class AjaxAction.
*
* @package AmpProject\AmpWP
*/

namespace AmpProject\AmpWP\Admin;

/**
* Base class to define a new AJAX action.
*
* @package AmpProject\AmpWP
*/
final class AjaxAction {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder… should the REST API rather be used?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, maybe it is premature to create an abstract AjaxAction construct. How about just naming this to be specific for the purpose of the re-enabling transient caching? Once we have another use case, then it would make sense to create an abstraction construct since we'll have another data point to inform it.



schlessera marked this conversation as resolved.
Show resolved Hide resolved
/**
* The AJAX action should be accessible by any visitor.
*
* @var int
*/
const ACCESS_ANY = 0;

/**
* The AJAX action should only be accessible by authenticated users.
*
* @var int
*/
const ACCESS_USER = 1;

/**
* The AJAX action should only be accessible by unauthenticated visitors.
*
* @var int
*/
const ACCESS_NO_USER = 2;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can just restrict this to authenticated users, as we don't have a use case for unauthenticated admin actions. These could be supported later if we need to.


/**
* The AJAX handler listens on both the backend and the frontend.
*
* @var int
*/
const SCOPE_BOTH = 0;

/**
* The AJAX handler listens on the backend only.
*
* @var int
*/
const SCOPE_BACKEND = 1;

/**
* The AJAX handler listens on the frontend only.
*
* @var int
*/
const SCOPE_FRONTEND = 2;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have no use case for frontend scope, do we? Perhaps the scopes should be omitted in favor of leaving this to be exclusively the realm of the admin? If we add actions to the frontend, I'm sure it will look different.

I think the scopes can be removed.


/**
* Action to use for enqueueing the JS logic at the frontend.
*
* @var string
*/
const FRONTEND_ENQUEUE_ACTION = 'wp_enqueue_scripts';

/**
* Action to use for enqueueing the JS logic at the backend.
*
* @var string
*/
const BACKEND_ENQUEUE_ACTION = 'admin_enqueue_scripts';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs to be more fine-grained. We only want it to fire on the site health screen, not on any other admin page.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now, this only starts the check to see if we're on the Site Health screen. The selector to attach the click event to is the very first thing that is being checked with an early return, so nothing happens when our button is not present.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something is still happening, though: CPU cycles to parse and evaluate the JS. This JS is being enqueued on the page which may not need it, namely wp-util. None of this JS should appear on any page other than Site Health.

Copy link
Collaborator Author

@schlessera schlessera Apr 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it will only ever be added on the page that contains this "button": a.reenable-css-transient-caching. That is the very first early return in the method that is hook to this action:

https://github.com/ampproject/amp-wp/pull/4522/files#diff-e97af8c0e681095ef90cd07c4cea405dR166-R168

As that button will only be present on the Site Health page, the JS will also only be enqueued there.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote nonsense above, as the code is only checking whether the variable is set, not whether a query against the DOM succeeded.

I'm adapting the code to use the $hook_suffix argument for the admin_enqueue_scripts action.


/**
* Access setting for the AJAX handler.
*
* @var int
*/
private $access;

/**
* Scope setting to define where the AJAX handler is listening.
*
* @var int
*/
private $scope;

/**
* Name of the action to identify incoming AJAX requests.
*
* @var string
*/
private $action;

/**
* Callback to execute when an AJAX request was received.
*
* @var callable
*/
private $callback;

/**
* Selector to attach the click handler to.
*
* @var string
*/
private $selector;

/**
* Instantiate an AjaxAction instance.
*
* @param string $action Name of the action to identify the AJAX request.
* @param callable $callback Callback function to call when the AJAX request came in.
* @param string $selector Optional. Selector to attach the click event to. Leave empty to skip the click handler.
* @param int $access Optional. Defaults to restricting the AJAX action to authenticated users only.
* @param int $scope Optional. Scope of where the AJAX handler is listening. Defaults to backend only.
*/
public function __construct(
$action,
callable $callback,
$selector = '',
$access = self::ACCESS_USER,
$scope = self::SCOPE_BACKEND
) {
if ( ! is_int( $access ) || $access < self::ACCESS_ANY || $access > self::ACCESS_NO_USER ) {
$access = self::ACCESS_USER;
}

if ( ! is_int( $scope ) || $scope < self::SCOPE_BOTH || $scope > self::SCOPE_FRONTEND ) {
$scope = self::SCOPE_BACKEND;
}

$this->action = $action;
$this->callback = $callback;
$this->selector = $selector;
$this->access = $access;
$this->scope = $scope;
}

/**
* Register the AJAX action with the WordPress system.
*/
public function register() {
if ( in_array( $this->access, [ self::ACCESS_USER, self::ACCESS_ANY ], true ) ) {
add_action( $this->get_action( self::ACCESS_USER ), $this->callback );
}

if ( in_array( $this->access, [ self::ACCESS_NO_USER, self::ACCESS_ANY ], true ) ) {
add_action( $this->get_action( self::ACCESS_NO_USER ), $this->callback );
}

if ( in_array( $this->scope, [ self::SCOPE_FRONTEND, self::SCOPE_BOTH ], true )
&& ! has_action( static::FRONTEND_ENQUEUE_ACTION, [ $this, 'register_ajax_script' ] ) ) {
add_action( static::FRONTEND_ENQUEUE_ACTION, [ $this, 'register_ajax_script' ] );
}

if ( in_array( $this->scope, [ self::SCOPE_BACKEND, self::SCOPE_BOTH ], true )
&& ! has_action( static::BACKEND_ENQUEUE_ACTION, [ $this, 'register_ajax_script' ] ) ) {
add_action( static::BACKEND_ENQUEUE_ACTION, [ $this, 'register_ajax_script' ] );
}
}

/**
* Register the AJAX logic.
*/
public function register_ajax_script() {
if ( empty( $this->selector ) ) {
return;
}

if ( self::SCOPE_FRONTEND === $this->scope && is_admin() ) {
return;
}

if ( self::SCOPE_BACKEND === $this->scope && ! is_admin() ) {
return;
}

$selector = wp_json_encode( $this->selector );
$action = wp_json_encode( $this->action );

$script = <<< JS_SCRIPT
<script>
( function () {
var selector = {$selector};
setTimeout( function () {
( document.querySelectorAll( selector ) || [] )
.forEach( ( element ) => {
element.addEventListener( 'click', function ( event ) {
event.preventDefault();
wp.ajax.post( {$action}, {} )
.done( function () {
element.classList.remove( 'ajax-failure' );
element.classList.add( 'ajax-success' )
} )
.fail( function () {
element.classList.remove( 'ajax-success' );
element.classList.add( 'ajax-failure' )
} );
} );
} );
}, 1000 );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the 1-second timeout? Wouldn't a DOMContentLoaded event handler suffice?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried that but it seems that the SiteHealth screen does other magic after the DOM content was loaded, and I also couldn't find an event to check for. Any suggestion what I could use here instead of a dumb timeout?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, it seems to work now. I must have done something else wrong when I tried it in the beginning.

} )();
</script>
JS_SCRIPT;

wp_enqueue_script( 'wp-util' );
wp_add_inline_script( 'wp-util', $script );
}
schlessera marked this conversation as resolved.
Show resolved Hide resolved

/**
* Get the action name to use for registering the AJAX handler.
*
* @param int $access The access setting to use for the action.
*
* @return string WordPress action to register the AJAX handler against.
*/
private function get_action( $access ) {
$prefix = 'wp_ajax_';

if ( self::ACCESS_NO_USER === $access ) {
$prefix .= 'nopriv_';
}

return $prefix . $this->action;
}
}
82 changes: 80 additions & 2 deletions src/Admin/SiteHealth.php
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,15 @@ public function init() {
add_filter( 'site_status_tests', [ $this, 'add_tests' ] );
add_filter( 'debug_information', [ $this, 'add_debug_information' ] );
add_filter( 'site_status_test_php_modules', [ $this, 'add_extensions' ] );
add_action( 'admin_print_styles', [ $this, 'add_styles' ] );

$reenable_css_transient_caching_ajax_action = new AjaxAction(
'reenable_css_transient_caching',
[ $this, 'reenable_css_transient_caching' ],
'a.reenable-css-transient-caching'
);

$reenable_css_transient_caching_ajax_action->register();
}

/**
Expand Down Expand Up @@ -240,11 +249,13 @@ public function icu_version() {
* @return array The test data.
*/
public function css_transient_caching() {
$disabled = AMP_Options_Manager::get_option( Option::DISABLE_CSS_TRANSIENT_CACHING, false );

if ( wp_using_ext_object_cache() ) {
$status = 'good';
$color = 'blue';
$label = __( 'Transient caching of parsed stylesheets is not used due to external object cache', 'amp' );
} elseif ( AMP_Options_Manager::get_option( Option::DISABLE_CSS_TRANSIENT_CACHING, false ) ) {
} elseif ( $disabled ) {
$status = 'recommended';
$color = 'orange';
$label = __( 'Transient caching of parsed stylesheets is disabled', 'amp' );
Expand All @@ -254,7 +265,7 @@ public function css_transient_caching() {
$label = __( 'Transient caching of parsed stylesheets is enabled', 'amp' );
}

return [
$data = [
'badge' => [
'label' => esc_html__( 'AMP', 'amp' ),
'color' => $color,
Expand All @@ -264,6 +275,17 @@ public function css_transient_caching() {
'status' => $status,
'label' => esc_html( $label ),
];

if ( $disabled ) {
$data['actions'] = sprintf(
'<p><a class="button reenable-css-transient-caching" href="#">%s</a><span class="dashicons dashicons-yes success-icon"></span><span class="dashicons dashicons-no failure-icon"></span><span class="success-text">%s</span><span class="failure-text">%s</span></p>',
__( 'Re-enable transient caching', 'amp' ),
__( 'Reload the page to refresh the diagnostic check.', 'amp' ),
__( 'The operation failed, please reload the page and try again.', 'amp' )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would these warrant esc_html__()? I know the others aren't doing it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we should escape them.

);
}

return $data;
}

/**
Expand Down Expand Up @@ -497,4 +519,60 @@ public function add_extensions( $extensions ) {
]
);
}

/**
* Re-enable the CSS Transient caching.
*
* This is triggered via an AJAX call from the Site Health panel.
*/
public function reenable_css_transient_caching() {
if ( ! current_user_can( 'manage_options' ) ) {
wp_send_json_error( 'Unauthorized.', 401 );
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about nonce checking?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will add.


$result = AMP_Options_Manager::update_option( Option::DISABLE_CSS_TRANSIENT_CACHING, false );

if ( false === $result ) {
wp_send_json_error( 'CSS transient caching could not be re-enabled.', 500 );
}

wp_send_json_success( 'CSS transient caching was re-enabled.', 200 );
}

/**
* Add needed styles for the Site Health integration.
*/
public function add_styles() {
echo '
<style>
.wp-core-ui .button.reenable-css-transient-caching ~ .success-icon,
.wp-core-ui .button.reenable-css-transient-caching ~ .success-text,
.wp-core-ui .button.reenable-css-transient-caching ~ .failure-icon,
.wp-core-ui .button.reenable-css-transient-caching ~ .failure-text {
display: none;
}

.wp-core-ui .button.reenable-css-transient-caching ~ .success-icon,
.wp-core-ui .button.reenable-css-transient-caching ~ .failure-icon {
font-size: xx-large;
padding-right: 1rem;
}

.wp-core-ui .button.reenable-css-transient-caching.ajax-success ~ .success-icon,
.wp-core-ui .button.reenable-css-transient-caching.ajax-success ~ .success-text,
.wp-core-ui .button.reenable-css-transient-caching.ajax-failure ~ .failure-icon,
.wp-core-ui .button.reenable-css-transient-caching.ajax-failure ~ .failure-text {
display: inline-block;
}

.wp-core-ui .button.reenable-css-transient-caching.ajax-success ~ .success-icon {
color: #46b450;
}

.wp-core-ui .button.reenable-css-transient-caching.ajax-failure ~ .failure-icon {
color: #dc3232;
}
</style>
';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems spaces are being used here when tabs are expected.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be better to have spaces in the HTML markup rather than tabs?

Also, things like this would be nicer via views so you can split the logic from the markup. We should discuss at one point whether we want to have a very simple view abstraction (can be a single class if we want to keep it simple) to pull in external (templated) files.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love the code examples on that page, btw... :)

}
}