Merge pull request #7458 from ampproject/fix/polyfills-service

Fix usage of `amp_register_polyfills` hook
ampproject · Feb 7, 2023 · 81b1307 · 81b1307
2 parents 65f1565 + 56daefa
commit 81b1307
Show file tree

Hide file tree

Showing 10 changed files with 155 additions and 95 deletions.
diff --git a/includes/admin/class-amp-template-customizer.php b/includes/admin/class-amp-template-customizer.php
@@ -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' ),
@@ -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' ),
@@ -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'];

diff --git a/includes/admin/functions.php b/includes/admin/functions.php
@@ -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.

diff --git a/src/Admin/OnboardingWizardSubmenuPage.php b/src/Admin/OnboardingWizardSubmenuPage.php
@@ -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';

diff --git a/src/Admin/OptionsMenu.php b/src/Admin/OptionsMenu.php
@@ -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';

diff --git a/src/Admin/PairedBrowsing.php b/src/Admin/PairedBrowsing.php
@@ -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'];
@@ -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,

diff --git a/src/Admin/SupportScreen.php b/src/Admin/SupportScreen.php
@@ -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';

diff --git a/src/Admin/ValidationCounts.php b/src/Admin/ValidationCounts.php
@@ -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 */
-		do_action( 'amp_register_polyfills' );
-
 		$asset_file   = AMP__DIR__ . '/assets/js/' . self::ASSETS_HANDLE . '.asset.php';
 		$asset        = require $asset_file;
 		$dependencies = $asset['dependencies'];
@@ -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' );
-		}
+		);
 	}
 }
diff --git a/tests/php/src/Admin/PairedBrowsingTest.php b/tests/php/src/Admin/PairedBrowsingTest.php
@@ -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 );
@@ -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 );
@@ -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' ) );
 
@@ -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' );
+		}
+	}
 }
diff --git a/tests/php/test-includes-admin-functions.php b/tests/php/test-includes-admin-functions.php
@@ -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
@@ -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();
@@ -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' );
@@ -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() );
@@ -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.