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.

Sign up for GitHub

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

Fix usage of amp_register_polyfills hook #7458

Merged
merged 13 commits into from
Feb 7, 2023
Merged

Fix usage of amp_register_polyfills hook #7458

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
077523c
Update ValidationCounts service to register `amp_register_polyfills` …
thelovekesh Feb 7, 2023
378522c
Update `amp_register_polyfills` to register Polyfills only on older W…
thelovekesh Feb 7, 2023
3c17ea9
Update is_options_menu() to is_dedicated_plugin_screen() in Validatio…
thelovekesh Feb 7, 2023
a931e7a
Update docblock reference for amp_register_polyfills
thelovekesh Feb 7, 2023
5dba91a
Update webpack config to bundle @wordpress/dom-ready package with def…
thelovekesh Feb 7, 2023
5343399
Remove amp_register_polyfills hook from ValidationCounts service
thelovekesh Feb 7, 2023
ad16781
Remove amp customizer init if WP < 5.6
thelovekesh Feb 7, 2023
47c7934
Remove amp_register_polyfills hook from AMP_Template_Customizer
thelovekesh Feb 7, 2023
ec2974f
Remove amp_register_polyfills hook from PairedBrowsing
thelovekesh Feb 7, 2023
8535112
Remove amp_register_polyfills hook assertions from PairedBrowsingTest
thelovekesh Feb 7, 2023
e7500ab
Skip AMP customizer tests below WP 5.6
thelovekesh Feb 7, 2023
8fd04db
Update test skip message
thelovekesh Feb 7, 2023
56daefa
Skip tests for PairedBrowsing if WP < 5
thelovekesh Feb 7, 2023
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
9 changes: 0 additions & 9 deletions includes/admin/class-amp-template-customizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,9 +330,6 @@ public function add_customizer_scripts() {
$dependencies = $asset['dependencies'];
$version = $asset['version'];

/** This action is documented in includes/class-amp-theme-support.php */
do_action( 'amp_register_polyfills' );

wp_enqueue_script(
$handle,
amp_get_asset_url( 'js/amp-customize-controls.js' ),
Expand Down Expand Up @@ -585,9 +582,6 @@ public function add_legacy_customizer_scripts() {
$dependencies = $asset['dependencies'];
$version = $asset['version'];

/** This action is documented in includes/class-amp-theme-support.php */
do_action( 'amp_register_polyfills' );

wp_enqueue_script(
'amp-customize-controls', // Note: This is not 'amp-customize-controls-legacy' to not break existing scripts that have this dependency.
amp_get_asset_url( 'js/amp-customize-controls-legacy.js' ),
Expand Down Expand Up @@ -645,9 +639,6 @@ public function enqueue_legacy_preview_scripts() {
return;
}

/** This action is documented in includes/class-amp-theme-support.php */
do_action( 'amp_register_polyfills' );

$asset_file = AMP__DIR__ . '/assets/js/amp-customize-preview-legacy.asset.php';
$asset = require $asset_file;
$dependencies = $asset['dependencies'];
Expand Down
32 changes: 1 addition & 31 deletions includes/admin/functions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,38 +16,8 @@
* @internal
*/
function amp_init_customizer() {

if ( ! Services::get( 'dependency_support' )->has_support() ) {
// @codeCoverageIgnoreStart
add_action(
'customize_controls_init',
static function () {
global $wp_customize;
if (
Services::get( 'reader_theme_loader' )->is_theme_overridden()
||
array_intersect( $wp_customize->get_autofocus(), [ 'panel' => AMP_Template_Customizer::PANEL_ID ] )
||
isset( $_GET[ QueryVar::AMP_PREVIEW ] ) // phpcs:ignore WordPress.Security.NonceVerification.Recommended
) {
wp_die(
esc_html(
sprintf(
/* translators: %s is minimum WordPress version */
__( 'Customizer for AMP is unavailable due to WordPress being out of date. Please upgrade to WordPress %s or greater.', 'amp' ),
DependencySupport::WP_MIN_VERSION
)
),
esc_html__( 'AMP Customizer Unavailable', 'amp' ),
[
'response' => 503,
'back_link' => true,
]
);
}
}
);
// @codeCoverageIgnoreEnd
return; // @codeCoverageIgnore
}

// Fire up the AMP Customizer.
Expand Down
2 changes: 1 addition & 1 deletion src/Admin/OnboardingWizardSubmenuPage.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ public function enqueue_assets( $hook_suffix ) {
return;
}

/** This action is documented in includes/class-amp-theme-support.php */
/** This action is documented in src/Admin/OptionsMenu.php */
do_action( 'amp_register_polyfills' );

$asset_file = AMP__DIR__ . '/assets/js/' . self::ASSET_HANDLE . '.asset.php';
Expand Down
6 changes: 5 additions & 1 deletion src/Admin/OptionsMenu.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,11 @@ public function enqueue_assets( $hook_suffix ) {
return;
}

/** This action is documented in includes/class-amp-theme-support.php */
/**
* Fires before registering plugin assets that may require core asset polyfills.
*
* @internal
*/
do_action( 'amp_register_polyfills' );

$asset_file = AMP__DIR__ . '/assets/js/' . self::ASSET_HANDLE . '.asset.php';
Expand Down
11 changes: 0 additions & 11 deletions src/Admin/PairedBrowsing.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,13 +182,6 @@ public function init_app() {
public function init_client() {
add_action( 'admin_bar_menu', [ $this, 'add_admin_bar_menu_item' ], 102 );

/**
* Fires before registering plugin assets that may require core asset polyfills.
*
* @internal
*/
do_action( 'amp_register_polyfills' );

$handle = 'amp-paired-browsing-client';
$asset = require AMP__DIR__ . '/assets/js/amp-paired-browsing-client.asset.php';
$dependencies = $asset['dependencies'];
Expand Down Expand Up @@ -308,10 +301,6 @@ public function ensure_app_location() {
* @return string Custom template if in paired browsing mode, else the supplied template.
*/
public function filter_template_include_for_app() {

/** This action is documented in includes/class-amp-theme-support.php */
do_action( 'amp_register_polyfills' );

$handle = 'amp-paired-browsing-app';
wp_enqueue_style(
$handle,
Expand Down
2 changes: 1 addition & 1 deletion src/Admin/SupportScreen.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ public function enqueue_assets( $hook_suffix ) {
return;
}

/** This action is documented in includes/class-amp-theme-support.php */
/** This action is documented in src/Admin/OptionsMenu.php */
do_action( 'amp_register_polyfills' );

$asset_file = AMP__DIR__ . '/assets/js/' . self::ASSET_HANDLE . '.asset.php';
Expand Down
18 changes: 11 additions & 7 deletions src/Admin/ValidationCounts.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,9 +89,6 @@ public function register() {
* Enqueue admin assets.
*/
public function enqueue_scripts() {
/** This action is documented in includes/class-amp-theme-support.php */
thelovekesh marked this conversation as resolved.
Show resolved Hide resolved
do_action( 'amp_register_polyfills' );

$asset_file = AMP__DIR__ . '/assets/js/' . self::ASSETS_HANDLE . '.asset.php';
$asset = require $asset_file;
$dependencies = $asset['dependencies'];
Expand Down Expand Up @@ -122,12 +119,19 @@ public function enqueue_scripts() {
* screen related to `amp_validation_error` post type (which includes the `amp_validation_error` taxonomy).
*/
protected function maybe_add_preload_rest_paths() {
if (
if ( $this->is_dedicated_plugin_screen() ) {
$this->rest_preloader->add_preloaded_path( '/amp/v1/unreviewed-validation-counts' );
}
}

/**
* Whether the current screen is pages inside the AMP Options menu.
*/
public function is_dedicated_plugin_screen() {
return (
AMP_Options_Manager::OPTION_NAME === get_admin_page_parent()
||
AMP_Validated_URL_Post_Type::POST_TYPE_SLUG === get_current_screen()->post_type
) {
$this->rest_preloader->add_preloaded_path( '/amp/v1/unreviewed-validation-counts' );
}
);
Copy link
Member

Choose a reason for hiding this comment

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

For is_needed the conditions may need to be expanded to account for all of these screens and subscreens:

image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you want the ValidationCounts service to be needed only on the AMP settings menus?

Copy link
Member

Choose a reason for hiding this comment

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

It can be on all screens when we core is shipping the expected version of React, but otherwise it can be limited to only the AMP settings pages.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ValidationCounts doesn't have any dependency on React and uses 'wp-api-fetch', 'wp-dom-ready', 'wp-i18n'(which also doesn't have any dependency on React). So I think we can keep it on all screens.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, good point. Nevertheless, we will need to not register the polyfills for this since the polyfills are unconditional now.

Copy link
Member

Choose a reason for hiding this comment

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

That is, unless we limit the service to only run on our dedicated screens. But probably simpler to just remove the polyfill registration entirely for this. Since the service is already conditional based on whether it has_support, there doesn't seem to be any need for polyfills anyway, right?

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 can remove polyfills for it entirely as it's conditional based on has_support.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed the amp_register_polyfills hook from the ValidationCounts service.

}
}
16 changes: 12 additions & 4 deletions tests/php/src/Admin/PairedBrowsingTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,14 +142,15 @@ public function test_init_frontend_app() {

// Check that init_client() was not called.
$this->assertFalse( has_action( 'admin_bar_menu', [ $this->instance, 'add_admin_bar_menu_item' ] ) );
$this->assertEquals( 0, did_action( 'amp_register_polyfills' ) );
}

/**
* @covers ::init_frontend()
* @covers ::init_client()
*/
public function test_init_frontend_client() {
$this->maybe_skip_test();

wp_set_current_user( self::factory()->user->create( [ 'role' => 'administrator' ] ) );
$post = self::factory()->post->create_and_get();
AMP_Options_Manager::update_option( Option::THEME_SUPPORT, AMP_Theme_Support::TRANSITIONAL_MODE_SLUG );
Expand All @@ -161,7 +162,6 @@ public function test_init_frontend_client() {

// Check that init_client() was called.
$this->assertEquals( 102, has_action( 'admin_bar_menu', [ $this->instance, 'add_admin_bar_menu_item' ] ) );
$this->assertEquals( 1, did_action( 'amp_register_polyfills' ) );
$this->assertTrue( wp_script_is( 'amp-paired-browsing-client' ) );
$printed_scripts = get_echo( 'wp_print_scripts' );
$this->assertStringContainsString( DevMode::DEV_MODE_ATTRIBUTE, $printed_scripts );
Expand Down Expand Up @@ -234,10 +234,9 @@ function () use ( &$redirected ) {

/** @covers ::filter_template_include_for_app() */
public function test_filter_template_include_for_app_when_allowed() {
$this->assertEquals( 0, did_action( 'amp_register_polyfills' ) );
$this->maybe_skip_test();

$include_path = $this->instance->filter_template_include_for_app();
$this->assertEquals( 1, did_action( 'amp_register_polyfills' ) );
$this->assertTrue( wp_style_is( 'amp-paired-browsing-app' ) );
$this->assertTrue( wp_script_is( 'amp-paired-browsing-app' ) );

Expand All @@ -250,4 +249,13 @@ public function test_filter_template_include_for_app_when_allowed() {
$this->assertStringContainsString( 'ampPairedBrowsingAppData', $template );
$this->assertStringContainsString( 'ampPairedBrowsingQueryVar', $template );
}

/**
* Skip tests when wp-url and wp-dom-ready are not available.
*/
public function maybe_skip_test() {
if ( version_compare( get_bloginfo( 'version' ), '5.0', '<' ) ) {
$this->markTestSkipped( 'Skipping test as PairedBrowsing can\'t be tested due to lack of wp-dom-ready and wp-url scripts in WP < 5.0' );
}
}
}
16 changes: 16 additions & 0 deletions tests/php/test-includes-admin-functions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
use AmpProject\AmpWP\Option;
use AmpProject\AmpWP\Tests\Helpers\LoadsCoreThemes;
use AmpProject\AmpWP\Tests\TestCase;
use AmpProject\AmpWP\DependencySupport;

/**
* Class Test_AMP_Admin_Includes_Functions
Expand Down Expand Up @@ -39,6 +40,8 @@ public function tear_down() {

/** @covers ::amp_init_customizer() */
public function test_amp_init_customizer_legacy_reader() {
$this->maybe_skip_amp_customizer_test();

AMP_Options_Manager::update_option( Option::THEME_SUPPORT, AMP_Theme_Support::READER_MODE_SLUG );
AMP_Options_Manager::update_option( Option::READER_THEME, ReaderThemes::DEFAULT_READER_THEME );
amp_init_customizer();
Expand All @@ -50,6 +53,8 @@ public function test_amp_init_customizer_legacy_reader() {

/** @covers ::amp_init_customizer() */
public function test_amp_init_customizer_modern_reader() {
$this->maybe_skip_amp_customizer_test();

switch_theme( 'twentytwenty' );
AMP_Options_Manager::update_option( Option::THEME_SUPPORT, AMP_Theme_Support::READER_MODE_SLUG );
AMP_Options_Manager::update_option( Option::READER_THEME, 'twentyseventeen' );
Expand All @@ -62,6 +67,8 @@ public function test_amp_init_customizer_modern_reader() {

/** @covers ::amp_init_customizer() */
public function test_amp_init_customizer_canonical() {
$this->maybe_skip_amp_customizer_test();

AMP_Options_Manager::update_option( Option::THEME_SUPPORT, AMP_Theme_Support::STANDARD_MODE_SLUG );
amp_init_customizer();
$this->assertTrue( amp_is_canonical() );
Expand All @@ -71,6 +78,15 @@ public function test_amp_init_customizer_canonical() {
$this->assertFalse( has_action( 'admin_menu', 'amp_add_customizer_link' ) );
}

/**
* Check if AMP customizer test should be skipped in old WP versions.
*/
public function maybe_skip_amp_customizer_test() {
if ( ! version_compare( get_bloginfo( 'version' ), DependencySupport::WP_MIN_VERSION, '>=' ) ) {
$this->markTestSkipped( sprintf( 'Skipping as AMP customizer is not supported in WP versions older than %s', DependencySupport::WP_MIN_VERSION ) );
}
}

/** @covers ::amp_admin_get_preview_permalink() */
public function test_amp_admin_get_preview_permalink() {
// No posts exist yet.
Expand Down
Loading