Fix usage of `amp_register_polyfills` hook #7458

thelovekesh · 2023-02-07T14:40:08Z

Summary

This PR accounts for the following changes:

Make changes to the Webpack config to fix the default export in @wordpress/dom-ready package.
Remove amp_register_polyfills from the following services/classes:
- PairedBrowsing -> This service has a conditional of has_support from DependecySupport which means it will work only on WP >= 5.6 so no polyfills are required. Also, this service has no dependency on React so we can keep it on our plugin.
- ValidationCounts -> This service has a conditional of has_support from DependecySupport which means it will work only on WP >= 5.6 so no polyfills are required. Also, this service has no dependency on React so we can keep it on our plugin.
- AMP_Template_Customizer -> We have disabled AMP customizer on WP < 5.6 so no polyfills required. Also, this service has no dependency on React so we can keep it on our plugin.
Disable AMP customizer on WP < 5.6

With this, we only have the following services left which use amp_register_polyfills only on their dedicated screens:

OnboardingWizardSubmenuPage
OptionsMenu
SupportScreen

Fixes #7457

Checklist

My code is tested and passes existing tests.
My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

…hook when screen is AMP options menu section

github-actions · 2023-02-07T14:45:20Z

Plugin builds for 56daefa are ready 🛎️!

Download development build
Download production build

…P versions

westonruter · 2023-02-07T18:29:07Z

src/Admin/ValidationCounts.php

@@ -89,8 +89,9 @@ 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' );
+		if ( $this->is_options_menu() ) {


Oh, actually maybe this won't work as expected? Let's say you're not on the Settings screen. In this case, the polyfills won't be registered and the result will be possibly that the required scripts are not registered, right?

I think the way to address this is to remove the logic here and move it to is_needed, so that the logic runs if:

We have the expected version of React or we're on one of the AMP settings screens.

In this case, the polyfills won't be registered and the result will be possibly that the required scripts are not registered, right?

Since it has 'wp-api-fetch', 'wp-dom-ready', 'wp-i18n' as dependency scripts while wp_enqueue_script, WP will enqueue them for it... right? I think WP prior to 5.6 already has these scripts' support and below 5.6 we are not exposing this service.

amp-wp/src/Admin/ValidationCounts.php

Line 78 in 378522c

return Services::get( 'dependency_support' )->has_support() && Services::get( 'dev_tools.user_access' )->is_user_enabled();

We have the expected version of React or we're on one of the AMP settings screens.

amp-validation-counts.js doesn't require React for its operation. Here is the dependecies for this:

<?php return array('dependencies' => array('wp-api-fetch', 'wp-dom-ready', 'wp-i18n'), 'version' => '29b558177338ddf7e50f');

src/Admin/ValidationCounts.php

…nCounts class

westonruter · 2023-02-07T19:00:18Z

src/Admin/ValidationCounts.php

-		) {
-			$this->rest_preloader->add_preloaded_path( '/amp/v1/unreviewed-validation-counts' );
-		}
+		);


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

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

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.

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.

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

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?

Yes, we can remove polyfills for it entirely as it's conditional based on has_support.

Removed the amp_register_polyfills hook from the ValidationCounts service.

westonruter · 2023-02-07T19:12:40Z

includes/admin/class-amp-template-customizer.php

@@ -330,8 +330,12 @@ 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' );
+		if ( ! Services::get( 'dependency_support' )->has_support() ) {


Aside: This is essentially re-making the Polyfills service conditional which was removed in #7421

Interestingly, also, there are some cases where this will never be reachable:

amp-wp/includes/admin/functions.php

Lines 18 to 54 in 3c17ea9

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

}

// Fire up the AMP Customizer.

add_action( 'customize_register', [ AMP_Template_Customizer::class, 'init' ], 500 );

Aside: This is essentially re-making the Polyfills service conditional which was removed in #7421

But only for customizers right?

Interestingly, also, there are some cases where this will never be reachable:

Actually, added it as a safeguard. Should we remove it? What if it surpassed the condition in any case?

I'm unsure. Maybe what we should do is just change amp_init_customizer to just return early if ! Services::get( 'dependency_support' )->has_support().

And then at the same time, we can remove all of the amp_register_polyfills actions from this class. What this means is that the custom AMP Customizer integrations won't be available on WP<5.6, but that's effectively the earliest version we support anyway.

Additionally, Customizer usage is declining.

So this means replacing all of the contents of this if condition:

amp-wp/includes/admin/functions.php

Lines 20 to 51 in 3c17ea9

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

}

With just return.

I think you mean:

if ( ! Services::get( 'dependency_support' )->has_support() ) { return; }

We can also remove the @codeCoverageIgnore right?

Yes, that's right.

Or rather, you can make it:

if ( ! Services::get( 'dependency_support' )->has_support() ) { return; // @codeCoverageIgnore }

…ault export library

- We are not using amp customizer on WP < 5.6, so polyfills are not needed. See: #7458 (comment)

westonruter

Great work! This is now ready for QA. 🎉

Update ValidationCounts service to register amp_register_polyfills …

077523c

…hook when screen is AMP options menu section

Update amp_register_polyfills to register Polyfills only on older W…

378522c

…P versions

westonruter reviewed Feb 7, 2023

View reviewed changes

src/Admin/ValidationCounts.php Show resolved Hide resolved

westonruter reviewed Feb 7, 2023

View reviewed changes

src/Admin/ValidationCounts.php Outdated Show resolved Hide resolved

Update is_options_menu() to is_dedicated_plugin_screen() in Validatio…

3c17ea9

…nCounts class

westonruter reviewed Feb 7, 2023

View reviewed changes

Update docblock reference for amp_register_polyfills

a931e7a

westonruter reviewed Feb 7, 2023

View reviewed changes

westonruter mentioned this pull request Feb 7, 2023

Update polyfills service to load unconditionally #7421

Merged

2 tasks

thelovekesh added 2 commits February 8, 2023 02:50

Update webpack config to bundle @wordpress/dom-ready package with def…

5dba91a

…ault export library

Remove amp_register_polyfills hook from ValidationCounts service

5343399

westonruter added this to the v2.4 milestone Feb 7, 2023

thelovekesh added 7 commits February 8, 2023 03:06

Remove amp customizer init if WP < 5.6

ad16781

Remove amp_register_polyfills hook from AMP_Template_Customizer

47c7934

- We are not using amp customizer on WP < 5.6, so polyfills are not needed. See: #7458 (comment)

Remove amp_register_polyfills hook from PairedBrowsing

ec2974f

Remove amp_register_polyfills hook assertions from PairedBrowsingTest

8535112

Skip AMP customizer tests below WP 5.6

e7500ab

Update test skip message

8fd04db

Skip tests for PairedBrowsing if WP < 5

56daefa

thelovekesh requested a review from westonruter February 7, 2023 22:43

westonruter approved these changes Feb 7, 2023

View reviewed changes

westonruter merged commit 81b1307 into develop Feb 7, 2023

westonruter deleted the fix/polyfills-service branch February 7, 2023 22:56

westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Feb 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix usage of `amp_register_polyfills` hook #7458

Fix usage of `amp_register_polyfills` hook #7458

thelovekesh commented Feb 7, 2023 •

edited

Loading

github-actions bot commented Feb 7, 2023 •

edited

Loading

westonruter Feb 7, 2023

westonruter Feb 7, 2023

thelovekesh Feb 7, 2023

thelovekesh Feb 7, 2023 •

edited

Loading

westonruter Feb 7, 2023

thelovekesh Feb 7, 2023

westonruter Feb 7, 2023

thelovekesh Feb 7, 2023

westonruter Feb 7, 2023

westonruter Feb 7, 2023

thelovekesh Feb 7, 2023

thelovekesh Feb 7, 2023

westonruter Feb 7, 2023

thelovekesh Feb 7, 2023

westonruter Feb 7, 2023

westonruter Feb 7, 2023

thelovekesh Feb 7, 2023

westonruter Feb 7, 2023

westonruter Feb 7, 2023

westonruter left a comment

	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
	}

	// Fire up the AMP Customizer.
	add_action( 'customize_register', [ AMP_Template_Customizer::class, 'init' ], 500 );

Fix usage of amp_register_polyfills hook #7458

Fix usage of amp_register_polyfills hook #7458

Conversation

thelovekesh commented Feb 7, 2023 • edited Loading

Summary

Checklist

github-actions bot commented Feb 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thelovekesh Feb 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter left a comment

Choose a reason for hiding this comment

Fix usage of `amp_register_polyfills` hook #7458

Fix usage of `amp_register_polyfills` hook #7458

thelovekesh commented Feb 7, 2023 •

edited

Loading

github-actions bot commented Feb 7, 2023 •

edited

Loading

thelovekesh Feb 7, 2023 •

edited

Loading