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 Non_Blocking_Scripts_Check #519

Merged
merged 14 commits into from
Jul 23, 2024
241 changes: 241 additions & 0 deletions includes/Checker/Checks/Performance/Non_Blocking_Scripts_Check.php
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'] );

Check warning on line 69 in includes/Checker/Checks/Performance/Non_Blocking_Scripts_Check.php

View check run for this annotation

Codecov / codecov/patch

includes/Checker/Checks/Performance/Non_Blocking_Scripts_Check.php#L69

Added line #L69 was not covered by tests
} 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 ) {
Copy link
Member

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

Copy link
Member Author

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.

$posts = get_posts(
array(
'posts_per_page' => 1,
'post_type' => $post_type,
'post_status' => array( 'publish', 'inherit' ),
)
);

if ( ! isset( $posts[0] ) ) {
throw new Exception(
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

The 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(

Check warning on line 143 in includes/Checker/Checks/Performance/Non_Blocking_Scripts_Check.php

View check run for this annotation

Codecov / codecov/patch

includes/Checker/Checks/Performance/Non_Blocking_Scripts_Check.php#L142-L143

Added lines #L142 - L143 were not covered by tests
/* translators: %s: The Post Type name. */
__( 'Unable to retrieve post URL for post type: %s', 'plugin-check' ),
$post_type
)

Check warning on line 147 in includes/Checker/Checks/Performance/Non_Blocking_Scripts_Check.php

View check run for this annotation

Codecov / codecov/patch

includes/Checker/Checks/Performance/Non_Blocking_Scripts_Check.php#L145-L147

Added lines #L145 - L147 were not covered by tests
);
}

$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 ) {
Copy link
Member

Choose a reason for hiding this comment

The 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 done handles. There could be one output buffer for the head and another for the footer. This would then handle cases where a plugin is adding the strategy attribute manually via the script_loader_tag filter. It would also flag cases where a script fails to get a strategy due to a blocking dependent script depending on an async/defer script.

But that can be done in a subsequent PR.

Copy link
Member Author

Choose a reason for hiding this comment

The 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`.
Copy link
Member

Choose a reason for hiding this comment

The 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 wp_print_inline_script_tag() or otherwise?

Copy link
Member Author

Choose a reason for hiding this comment

The 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;

Check warning on line 188 in includes/Checker/Checks/Performance/Non_Blocking_Scripts_Check.php

View check run for this annotation

Codecov / codecov/patch

includes/Checker/Checks/Performance/Non_Blocking_Scripts_Check.php#L188

Added line #L188 was not covered by tests
}

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;
}
}
1 change: 1 addition & 0 deletions includes/Checker/Default_Check_Repository.php
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ private function register_default_checks() {
'localhost' => new Checks\Plugin_Repo\Localhost_Check(),
'no_unfiltered_uploads' => new Checks\Plugin_Repo\No_Unfiltered_Uploads_Check(),
'trademarks' => new Checks\Plugin_Repo\Trademarks_Check(),
'non_blocking_scripts' => new Checks\Performance\Non_Blocking_Scripts_Check(),
)
);

Expand Down
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'] );
}
}