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 2 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
24 changes: 18 additions & 6 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,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() ) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think you mean:

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

We can also remove the @codeCoverageIgnore right?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that's right.

Copy link
Member

Choose a reason for hiding this comment

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

Or rather, you can make it:

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

/**
* Fires when AMP Polyfills should be registered.
*/
do_action( 'amp_register_polyfills' );
}

wp_enqueue_script(
$handle,
Expand Down Expand Up @@ -585,8 +589,12 @@ 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' );
if ( ! Services::get( 'dependency_support' )->has_support() ) {
/**
* Fires when AMP Polyfills should be registered.
*/
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.
Expand Down Expand Up @@ -645,8 +653,12 @@ public function enqueue_legacy_preview_scripts() {
return;
}

/** This action is documented in includes/class-amp-theme-support.php */
do_action( 'amp_register_polyfills' );
if ( ! Services::get( 'dependency_support' )->has_support() ) {
/**
* Fires when AMP Polyfills should be registered.
*/
do_action( 'amp_register_polyfills' );
}

$asset_file = AMP__DIR__ . '/assets/js/amp-customize-preview-legacy.asset.php';
$asset = require $asset_file;
Expand Down
20 changes: 14 additions & 6 deletions src/Admin/ValidationCounts.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
thelovekesh marked this conversation as resolved.
Show resolved Hide resolved
do_action( 'amp_register_polyfills' );
if ( $this->is_options_menu() ) {
Copy link
Member

Choose a reason for hiding this comment

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

Good idea.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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();

Copy link
Collaborator Author

@thelovekesh thelovekesh Feb 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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');

do_action( 'amp_register_polyfills' );
}

$asset_file = AMP__DIR__ . '/assets/js/' . self::ASSETS_HANDLE . '.asset.php';
$asset = require $asset_file;
Expand Down Expand Up @@ -122,12 +123,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_options_menu() ) {
$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_options_menu() {
thelovekesh marked this conversation as resolved.
Show resolved Hide resolved
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.

}
}