-
Notifications
You must be signed in to change notification settings - Fork 56
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 Non_Blocking_Scripts_Check
#519
Changes from 12 commits
4c68736
30cedfe
790f60e
c665f51
1f36417
ddeaa8d
819d444
7a74daf
fa2eb87
31fdb1d
295dc83
36e3bb3
cdf5d58
b8c3b4e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,241 @@ | ||
<?php | ||
/** | ||
* Class Non_Blocking_Scripts_Check. | ||
* | ||
* @package plugin-check | ||
*/ | ||
|
||
namespace WordPress\Plugin_Check\Checker\Checks\Performance; | ||
|
||
use Exception; | ||
use WordPress\Plugin_Check\Checker\Check_Categories; | ||
use WordPress\Plugin_Check\Checker\Check_Result; | ||
use WordPress\Plugin_Check\Checker\Checks\Abstract_Runtime_Check; | ||
use WordPress\Plugin_Check\Checker\Preparations\Demo_Posts_Creation_Preparation; | ||
use WordPress\Plugin_Check\Checker\With_Shared_Preparations; | ||
use WordPress\Plugin_Check\Traits\Amend_Check_Result; | ||
use WordPress\Plugin_Check\Traits\Stable_Check; | ||
use WordPress\Plugin_Check\Traits\URL_Aware; | ||
|
||
/** | ||
* Check for non-blocking scripts. | ||
* | ||
* @since 1.1.0 | ||
*/ | ||
class Non_Blocking_Scripts_Check extends Abstract_Runtime_Check implements With_Shared_Preparations { | ||
|
||
use Amend_Check_Result; | ||
use Stable_Check; | ||
use URL_Aware; | ||
|
||
/** | ||
* List of viewable post types. | ||
* | ||
* @since 1.1.0 | ||
* @var array | ||
*/ | ||
private $viewable_post_types; | ||
|
||
/** | ||
* Gets the categories for the check. | ||
* | ||
* Every check must have at least one category. | ||
* | ||
* @since 1.1.0 | ||
* | ||
* @return array The categories for the check. | ||
*/ | ||
public function get_categories() { | ||
return array( Check_Categories::CATEGORY_PERFORMANCE ); | ||
} | ||
|
||
/** | ||
* Runs this preparation step for the environment and returns a cleanup function. | ||
* | ||
* @since 1.1.0 | ||
* | ||
* @return callable Cleanup function to revert any changes made here. | ||
* | ||
* @throws Exception Thrown when preparation fails. | ||
*/ | ||
public function prepare() { | ||
$orig_scripts = isset( $GLOBALS['wp_scripts'] ) ? $GLOBALS['wp_scripts'] : null; | ||
|
||
// Backup the original values for the global state. | ||
$this->backup_globals(); | ||
|
||
return function () use ( $orig_scripts ) { | ||
if ( is_null( $orig_scripts ) ) { | ||
unset( $GLOBALS['wp_scripts'] ); | ||
} else { | ||
$GLOBALS['wp_scripts'] = $orig_scripts; | ||
} | ||
|
||
$this->restore_globals(); | ||
}; | ||
} | ||
|
||
/** | ||
* Returns an array of shared preparations for the check. | ||
* | ||
* @since 1.1.0 | ||
* | ||
* @return array Returns a map of $class_name => $constructor_args pairs. If the class does not | ||
* need any constructor arguments, it would just be an empty array. | ||
*/ | ||
public function get_shared_preparations() { | ||
$demo_posts = array_map( | ||
static function ( $post_type ) { | ||
return array( | ||
'post_title' => "Demo {$post_type} post", | ||
'post_content' => 'Test content', | ||
'post_type' => $post_type, | ||
'post_status' => 'publish', | ||
); | ||
}, | ||
$this->get_viewable_post_types() | ||
); | ||
|
||
return array( | ||
Demo_Posts_Creation_Preparation::class => array( $demo_posts ), | ||
); | ||
} | ||
|
||
/** | ||
* Runs the check on the plugin and amends results. | ||
* | ||
* @since 1.1.0 | ||
* | ||
* @param Check_Result $result The check results to amend and the plugin context. | ||
*/ | ||
public function run( Check_Result $result ) { | ||
$this->run_for_urls( | ||
$this->get_urls(), | ||
function ( $url ) use ( $result ) { | ||
$this->check_url( $result, $url ); | ||
} | ||
); | ||
} | ||
|
||
/** | ||
* Gets the list of URLs to run this check for. | ||
* | ||
* @since 1.1.0 | ||
* | ||
* @return array List of URL strings (either full URLs or paths). | ||
* | ||
* @throws Exception Thrown when a post type URL cannot be retrieved. | ||
*/ | ||
protected function get_urls() { | ||
$urls = array( home_url() ); | ||
|
||
foreach ( $this->get_viewable_post_types() as $post_type ) { | ||
$posts = get_posts( | ||
array( | ||
'posts_per_page' => 1, | ||
'post_type' => $post_type, | ||
'post_status' => array( 'publish', 'inherit' ), | ||
) | ||
); | ||
|
||
if ( ! isset( $posts[0] ) ) { | ||
throw new Exception( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe just skip silently here, sicne a post type could have no posts? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This runtime check creates exactly 1 post in every post type, so it is guaranteed to have one. |
||
sprintf( | ||
/* translators: %s: The Post Type name. */ | ||
__( 'Unable to retrieve post URL for post type: %s', 'plugin-check' ), | ||
$post_type | ||
) | ||
); | ||
} | ||
|
||
$urls[] = get_permalink( $posts[0] ); | ||
} | ||
|
||
return $urls; | ||
} | ||
|
||
/** | ||
* Amends the given result by running the check for the given URL. | ||
* | ||
* @since 1.1.0 | ||
* | ||
* @param Check_Result $result The check result to amend, including the plugin context to check. | ||
* @param string $url URL to run the check for. | ||
* | ||
* @throws Exception Thrown when the check fails with a critical error (unrelated to any errors detected as part of | ||
* the check). | ||
* | ||
* @SuppressWarnings(PHPMD.NPathComplexity) | ||
*/ | ||
protected function check_url( Check_Result $result, $url ) { | ||
// Reset the WP_Scripts instance. | ||
unset( $GLOBALS['wp_scripts'] ); | ||
|
||
// Run the 'wp_enqueue_script' action, wrapped in an output buffer in case of any callbacks printing scripts | ||
// directly. This is discouraged, but some plugins or themes are still doing it. | ||
ob_start(); | ||
wp_enqueue_scripts(); | ||
wp_scripts()->do_head_items(); | ||
wp_scripts()->do_footer_items(); | ||
ob_end_clean(); | ||
|
||
foreach ( wp_scripts()->done as $handle ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just realized what may be a more robust way to handle this, which will also include support for inline scripts: use HTML Tag Processor to iterate over generated markup instead of iterating over the But that can be done in a subsequent PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A new issue for this sounds like a good idea 👍 |
||
$script = wp_scripts()->registered[ $handle ]; | ||
|
||
// TODO: Somehow detect inline scripts added by the plugin that don't have a `src`. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this referring to before/after inline scripts? Or to scripts that are manually printed with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Both I suppose :-) Right now the detection whether a script is coming from a plugin is very basic. Something like Origination + loopback requests would be more robust. |
||
|
||
if ( ! $script->src || strpos( $script->src, $result->plugin()->url() ) !== 0 ) { | ||
continue; | ||
} | ||
|
||
if ( ! empty( $script->extra['strategy'] ) ) { | ||
continue; | ||
} | ||
|
||
$script_path = str_replace( $result->plugin()->url(), $result->plugin()->path(), $script->src ); | ||
|
||
if ( ! in_array( $handle, wp_scripts()->in_footer, true ) ) { | ||
$this->add_result_warning_for_file( | ||
$result, | ||
sprintf( | ||
/* translators: 1: tested URL. 2: 'defer'. 3: 'async' */ | ||
__( 'This script on %1$s is potentially blocking. Consider a %2$s or %3$s script strategy or moving it to the footer.', 'plugin-check' ), | ||
$url, | ||
'defer', | ||
'async' | ||
), | ||
'NonBlockingScripts.BlockingHeadScript', | ||
$script_path | ||
); | ||
} else { | ||
$this->add_result_warning_for_file( | ||
$result, | ||
sprintf( | ||
/* translators: 1: tested URL. 2: 'defer'. 3: 'async' */ | ||
__( 'This script on %1$s is loaded in the footer. Consider a %2$s or %3$s script loading strategy instead.', 'plugin-check' ), | ||
$url, | ||
'defer', | ||
'async' | ||
), | ||
'NonBlockingScripts.NoStrategy', | ||
$script_path | ||
); | ||
} | ||
swissspidy marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} | ||
|
||
/** | ||
* Returns an array of viewable post types. | ||
* | ||
* @since 1.1.0 | ||
* | ||
* @return array Array of viewable post type slugs. | ||
*/ | ||
private function get_viewable_post_types() { | ||
if ( ! is_array( $this->viewable_post_types ) ) { | ||
$this->viewable_post_types = array_filter( get_post_types(), 'is_post_type_viewable' ); | ||
} | ||
|
||
return $this->viewable_post_types; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
<?php | ||
/** | ||
* File contains errors for the non blocking scripts check. | ||
*/ | ||
|
||
add_action( | ||
'wp_enqueue_scripts', | ||
function() { | ||
wp_enqueue_script( | ||
'plugin_check_test_script_header', | ||
plugin_dir_url( __FILE__ ) . 'header.js', | ||
array(), | ||
false, | ||
array( | ||
'in_footer' => false, | ||
) | ||
); | ||
|
||
wp_enqueue_script( | ||
'plugin_check_test_script_footer', | ||
plugin_dir_url( __FILE__ ) . 'footer.js', | ||
array(), | ||
false, | ||
array( | ||
'in_footer' => true, | ||
) | ||
); | ||
|
||
wp_enqueue_script( | ||
'plugin_check_test_script_defer', | ||
plugin_dir_url( __FILE__ ) . 'defer.js', | ||
array(), | ||
false, | ||
array( | ||
'strategy' => 'defer' | ||
) | ||
); | ||
|
||
wp_enqueue_script( | ||
'plugin_check_test_script_async', | ||
plugin_dir_url( __FILE__ ) . 'async.js', | ||
array(), | ||
false, | ||
array( | ||
'strategy' => 'async' | ||
) | ||
); | ||
} | ||
); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,89 @@ | ||
<?php | ||
/** | ||
* Tests for the Non_Blocking_Scripts_Check class. | ||
* | ||
* @package plugin-check | ||
*/ | ||
|
||
use WordPress\Plugin_Check\Checker\Checks\Performance\Non_Blocking_Scripts_Check; | ||
use WordPress\Plugin_Check\Checker\Preparation; | ||
use WordPress\Plugin_Check\Test_Utils\TestCase\Runtime_Check_UnitTestCase; | ||
|
||
class Non_Blocking_Scripts_Check_Tests extends Runtime_Check_UnitTestCase { | ||
|
||
public function test_get_shared_preparations() { | ||
$check = new Non_Blocking_Scripts_Check(); | ||
$preparations = $check->get_shared_preparations(); | ||
|
||
$this->assertIsArray( $preparations ); | ||
|
||
foreach ( $preparations as $class => $args ) { | ||
$instance = new $class( ...$args ); | ||
$this->assertInstanceOf( Preparation::class, $instance ); | ||
} | ||
} | ||
|
||
public function test_prepare() { | ||
// Create variables in global state. | ||
$_GET['test_prepare'] = true; | ||
$_POST['test_prepare'] = true; | ||
$_SERVER['test_prepare'] = true; | ||
|
||
$current_screen = $GLOBALS['current_screen']; | ||
$GLOBALS['current_screen'] = 'test_prepare'; | ||
|
||
$check = new Non_Blocking_Scripts_Check(); | ||
$cleanup = $check->prepare(); | ||
|
||
// Modify the variables in the global state. | ||
$_GET['test_prepare'] = false; | ||
$_POST['test_prepare'] = false; | ||
$_SERVER['test_prepare'] = false; | ||
$GLOBALS['current_screen'] = 'altered'; | ||
|
||
$cleanup(); | ||
|
||
$test_get = $_GET['test_prepare']; | ||
$test_post = $_POST['test_prepare']; | ||
$test_server = $_SERVER['test_prepare']; | ||
$test_globals = $GLOBALS['current_screen']; | ||
|
||
// Restore the global state. | ||
unset( $_GET['test_prepare'] ); | ||
unset( $_POST['test_prepare'] ); | ||
unset( $_SERVER['test_prepare'] ); | ||
$GLOBALS['current_screen'] = $current_screen; | ||
|
||
$this->assertTrue( $test_get ); | ||
$this->assertTrue( $test_post ); | ||
$this->assertTrue( $test_server ); | ||
$this->assertSame( 'test_prepare', $test_globals ); | ||
} | ||
|
||
public function test_run_with_warnings() { | ||
require UNIT_TESTS_PLUGIN_DIR . 'test-plugin-non-blocking-scripts-check/load.php'; | ||
|
||
$check = new Non_Blocking_Scripts_Check(); | ||
$context = $this->get_context( WP_PLUGIN_CHECK_MAIN_FILE ); | ||
$results = $this->run_check( $check, $context ); | ||
|
||
$errors = $results->get_errors(); | ||
$warnings = $results->get_warnings(); | ||
|
||
$this->assertEmpty( $errors ); | ||
$this->assertNotEmpty( $warnings ); | ||
|
||
$header_script = 'tests/phpunit/testdata/plugins/test-plugin-non-blocking-scripts-check/header.js'; | ||
$footer_script = 'tests/phpunit/testdata/plugins/test-plugin-non-blocking-scripts-check/footer.js'; | ||
$async_script = 'tests/phpunit/testdata/plugins/test-plugin-non-blocking-scripts-check/async.js'; | ||
$defer_script = 'tests/phpunit/testdata/plugins/test-plugin-non-blocking-scripts-check/defer.js'; | ||
|
||
$this->assertArrayNotHasKey( $async_script, $warnings, 'An async script should not cause any warnings' ); | ||
$this->assertArrayNotHasKey( $defer_script, $warnings, 'A deferred script should not cause any warnings' ); | ||
$this->assertArrayHasKey( $header_script, $warnings, 'A header script should cause a warning' ); | ||
$this->assertArrayHasKey( $footer_script, $warnings, 'A footer script should cause a warning' ); | ||
|
||
$this->assertSame( 'NonBlockingScripts.BlockingHeadScript', $warnings[ $header_script ][0][0][0]['code'] ); | ||
$this->assertSame( 'NonBlockingScripts.NoStrategy', $warnings[ $footer_script ][0][0][0]['code'] ); | ||
} | ||
} |
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.
maybe add a sanity limit here, check at most 5 or 10 post types, I can imagine some site has 10,000 post types and this breaks site health for them
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.
Hmm no other plugins are active during checks and we're checking only a specific plugin here, I doubt that a single plugin adds that many post types.
If we add a limit, then we'd need to add it to all the runtime checks, but I don't see a need for it.