Only load block editor related assets when it is used for a post #6534

pierlon · 2021-08-12T14:28:38Z

Summary

Fixes #6521

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

github-actions · 2021-08-12T14:31:47Z

Plugin builds for 99915e9 are ready 🛎️!

Download development build
Download production build

westonruter

I think changes like this may be a better alternative:

diff --git a/includes/validation/class-amp-validation-manager.php b/includes/validation/class-amp-validation-manager.php
index 402ec4070..a5ff256e0 100644
--- a/includes/validation/class-amp-validation-manager.php
+++ b/includes/validation/class-amp-validation-manager.php
@@ -2175,6 +2175,11 @@ public static function enqueue_block_validation() {
 			return;
 		}
 
+		$screen = get_current_screen();
+		if ( ! $screen instanceof WP_Screen || 'post' !== $screen->base ) {
+			return;
+		}
+
 		$editor_support = Services::get( 'editor.editor_support' );
 
 		// Block validation script uses features only available beginning with WP 5.6.
diff --git a/src/Editor/EditorSupport.php b/src/Editor/EditorSupport.php
index 4e64a4ed6..0fb0f32aa 100644
--- a/src/Editor/EditorSupport.php
+++ b/src/Editor/EditorSupport.php
@@ -50,6 +50,10 @@ public function maybe_show_notice() {
 			return;
 		}
 
+		if ( 'post' !== $screen->base ) {
+			return;
+		}
+
 		$is_block_editor = (
 			! empty( $screen->is_block_editor )
 			||

westonruter · 2021-08-13T21:08:09Z

src/Editor/EditorSupport.php

@@ -91,6 +91,6 @@ function () {
 	 * @return bool
 	 */
 	public function editor_supports_amp_block_editor_features() {
-		return $this->dependency_support->has_support();
+		return $this->dependency_support->has_support() && use_block_editor_for_post( get_post() );


This might be too low level as it would prevent using this method in other contexts, such as if we want to show a notice on the settings screen or site health to encourage the user to update to use the block editor.

That would be what DependencySupport::has_support() is for, no? EditorSupport::editor_supports_amp_block_editor_features() is specifically for checking block editor support and is currently being used as a check before enqueuing block editor related assets (for posts).

I don't think so because isn't the problem that we're enqueueing on the wrong screen?

It's being used in three places:

\AMP_Post_Meta_Box::enqueue_block_assets()

\AMP_Validation_Manager::enqueue_block_validation()

\AmpProject\AmpWP\Editor\EditorSupport::maybe_show_notice()

In the first, it's checking if the post global is set and so it isn't having this problem. However, the second two are not checking for the $post global and so they could be causing problems.

The three cases here should all be checking if get_current_screen()->is_block_editor (instead of checking whether the base is 'post') rather than exclusively checking if get_post() is set since it could be that a plugin creates that global on another screen. This is actually being done today in the last one so my suggested change isn't relevant there.

So maybe this is a better suggested alternative:

diff --git a/includes/admin/class-amp-post-meta-box.php b/includes/admin/class-amp-post-meta-box.php index 86998c170..83e9955db 100644 --- a/includes/admin/class-amp-post-meta-box.php +++ b/includes/admin/class-amp-post-meta-box.php @@ -215,6 +215,11 @@ public function enqueue_admin_assets() { * @since 1.0 */ public function enqueue_block_assets() { + $screen = get_current_screen(); + if ( ! $screen || empty( $screen->is_block_editor ) ) { + return; + } + $post = get_post(); if ( ! $post instanceof WP_Post || ! in_array( $post->post_type, AMP_Post_Type_Support::get_eligible_post_types(), true ) ) { return; diff --git a/includes/validation/class-amp-validation-manager.php b/includes/validation/class-amp-validation-manager.php index 402ec4070..779e958df 100644 --- a/includes/validation/class-amp-validation-manager.php +++ b/includes/validation/class-amp-validation-manager.php @@ -2175,6 +2175,11 @@ public static function enqueue_block_validation() { return; } + $screen = get_current_screen(); + if ( ! $screen || empty( $screen->is_block_editor ) ) { + return; + } + $editor_support = Services::get( 'editor.editor_support' ); // Block validation script uses features only available beginning with WP 5.6.

In \AmpProject\AmpWP\Editor\EditorSupport::maybe_show_notice() it's checking is_gutenberg_page() which may not be relevant anymore.

The three cases here should all be checking if get_current_screen()->is_block_editor (instead of checking whether the base is 'post') rather than exclusively checking if get_post() is set since it could be that a plugin creates that global on another screen.

Since all those cases are using \AmpProject\AmpWP\Editor\EditorSupport::editor_supports_amp_block_editor_features() would you agree that that would be the most correct place to modify? That is:

diff --git a/src/Editor/EditorSupport.php b/src/Editor/EditorSupport.php --- a/src/Editor/EditorSupport.php (revision 38de6f6f86ca13e7c834e5688c4fa7144fc377b6) +++ b/src/Editor/EditorSupport.php (date 1628896711900) @@ -91,6 +91,7 @@ * @return bool */ public function editor_supports_amp_block_editor_features() { - return $this->dependency_support->has_support() && use_block_editor_for_post( get_post() ); + $screen = get_current_screen(); + return $this->dependency_support->has_support() && $screen && $screen->is_block_editor; } }

In \AmpProject\AmpWP\Editor\EditorSupport::maybe_show_notice() it's checking is_gutenberg_page() which may not be relevant anymore.

That's only relevant to users using Gutenberg v5.5 and older, where it's likely that their instance of WordPress (WP < v5) does not have get_current_screen()->is_block_editor.

Since all those cases are using \AmpProject\AmpWP\Editor\EditorSupport::editor_supports_amp_block_editor_features() would you agree that that would be the most correct place to modify?

I think so, but probably good to use ! empty( $screen->is_block_editor ) to prevent possible notices on older versions of WP.

Nevertheless, if this is done the phpdoc for the method should be updated to indicate that it is not only whether the editor in the “current environment” supports plugin features, but also that the current screen is the block editor.

Maybe the method name should be changed to reflect this.

That's only relevant to users using Gutenberg v5.5 and older, where it's likely that their instance of WordPress (WP < v5) does not have get_current_screen()->is_block_editor.

Correct, as we don't fully support versions older than WP 5.6.

Any concerns on also checking if the post type is eligible for AMP in the same method (EditorSupport::editor_supports_amp_block_editor_features())?

This logic, yeah?

if ( ! in_array( get_post_type(), AMP_Post_Type_Support::get_supported_post_types(), true ) ) { return; }

Yeah, that seems like a good move because that same logic is duplicated currently in \AMP_Post_Meta_Box::enqueue_block_assets() and \AmpProject\AmpWP\Editor\EditorSupport::maybe_show_notice():

amp-wp/includes/admin/class-amp-post-meta-box.php

Lines 217 to 221 in 041cc29

public function enqueue_block_assets() {

$post = get_post();

if ( ! $post instanceof WP_Post || ! in_array( $post->post_type, AMP_Post_Type_Support::get_eligible_post_types(), true ) ) {

return;

}

amp-wp/src/Editor/EditorSupport.php

Lines 63 to 65 in 041cc29

if ( ! in_array( get_post_type(), AMP_Post_Type_Support::get_eligible_post_types(), true ) ) {

return;

}

The logic is absent however in \AMP_Validation_Manager::enqueue_block_validation(). Indeed, I just disabled AMP for the Page post type, and erroneously the AMP toggle and AMP sidebar show up on that screen:

amp-ui-on-disabled-post-type.mov

This is fixed by adding the above condition.

This also extends to AMP_Post_Meta_Box::enqueue_block_assets() as I can see it is currently using this condition:

amp-wp/includes/admin/class-amp-post-meta-box.php

Lines 218 to 221 in 041cc29

$post = get_post();

if ( ! $post instanceof WP_Post || ! in_array( $post->post_type, AMP_Post_Type_Support::get_eligible_post_types(), true ) ) {

return;

}

Note how it's using AMP_Post_Type_Support::get_eligible_post_types() when it should be using AMP_Post_Type_Support::get_supported_post_types(), as a post can be eligible without being actually supported (e.g. by being disabled). So in the above example with the page post type disabled, I'm erroneously getting amp-block-editor enqueued onto the page even though it is entirely disabled.

This is also seems to be a bug in \AmpProject\AmpWP\Editor\EditorSupport::maybe_show_notice():

amp-wp/src/Editor/EditorSupport.php

Lines 63 to 65 in 041cc29

if ( ! in_array( get_post_type(), AMP_Post_Type_Support::get_eligible_post_types(), true ) ) {

return;

}

It seems this should also rather be using AMP_Post_Type_Support::get_supported_post_types().

…pports the editor and AMP

# Conflicts: # tests/php/src/Editor/EditorSupportTest.php

westonruter · 2021-08-16T04:00:47Z

src/Editor/EditorSupport.php

 	 *
 	 * @return bool
 	 */
-	public function editor_supports_amp_block_editor_features() {
-		return $this->dependency_support->has_support();
+	public function supports_current_screen() {


Maybe something like is_current_screen_supported_block_editor_for_amp_enabled_post_type would be more clear if also verbose. To me supports_current_screen is somewhat vague.

Done in 8235b9f.

…mp_enabled_post_type()`

westonruter · 2021-08-16T18:56:58Z

includes/admin/class-amp-post-meta-box.php

 		$post = get_post();
-		if ( ! $post instanceof WP_Post || ! in_array( $post->post_type, AMP_Post_Type_Support::get_eligible_post_types(), true ) ) {
-			return;
-		}

 		$editor_support = Services::get( 'editor.editor_support' );
-		if ( ! $editor_support->editor_supports_amp_block_editor_features() ) {
+		if ( ! $editor_support->is_current_screen_supported_block_editor_for_amp_enabled_post_type() ) {
 			return;
 		}


Per below, to use the proposed is_block_editor_screen_for_amp_enabled_post_type:

$dependency_support = Services::get( 'dependency_support' ); if ( ! $dependency_support->has_support() ) { return; // @codeCoverageIgnore } $editor_support = Services::get( 'editor.editor_support' ); if ( ! $editor_support->is_block_editor_screen_for_amp_enabled_post_type() ) { return; }

Notice how here we need ! $dependency_support->has_support() but in maybe_show_notice we need $dependency_support->has_support().

westonruter · 2021-08-16T18:57:37Z

includes/templates/amp-enabled-classic-editor-toggle.php

@@ -29,7 +29,7 @@
 	<?php if ( ! Services::get( 'dependency_support' )->has_support_from_core() ) : ?>
 		<div class="notice notice-info notice-alt inline">
 			<p>
-				<?php esc_html_e( 'Your version of WordPress is too old manage whether AMP is enabled. Please upgrade.', 'amp' ); ?>
+				<?php esc_html_e( 'Your version of WordPress is too old to manage whether AMP is enabled. Please upgrade.', 'amp' ); ?>


Nice catch. But if someone is on an old version of WP, they they deserve to have a typo. 😉

westonruter · 2021-08-16T19:02:21Z

includes/validation/class-amp-validation-manager.php

 		// Block validation script uses features only available beginning with WP 5.6.
-		if ( ! $editor_support->editor_supports_amp_block_editor_features() ) {
+		if ( ! $editor_support->is_current_screen_supported_block_editor_for_amp_enabled_post_type() ) {
 			return; // @codeCoverageIgnore
 		}


Per above, this could then be:

// Block validation script uses features only available beginning with WP 5.6. $dependency_support = Services::get( 'dependency_support' ); if ( ! $dependency_support->has_support() ) { return; // @codeCoverageIgnore } // Only enqueue scripts on the block editor for AMP-enabled posts. $editor_support = Services::get( 'editor.editor_support' ); if ( ! $editor_support->is_block_editor_screen_for_amp_enabled_post_type() ) { return; }

westonruter · 2021-08-16T19:04:02Z

src/Editor/EditorSupport.php

-	public function editor_supports_amp_block_editor_features() {
-		return $this->dependency_support->has_support();
+	public function is_current_screen_supported_block_editor_for_amp_enabled_post_type() {
+		$screen = get_current_screen();
+		return $this->dependency_support->has_support()
+			&&
+			$screen
+			&&
+			! empty( $screen->is_block_editor )
+			&&
+			in_array( get_post_type(), AMP_Post_Type_Support::get_supported_post_types(), true );
 	}


Per above, this could instead be:

public function is_block_editor_screen_for_amp_enabled_post_type() { $screen = get_current_screen(); return ( $screen && ! empty( $screen->is_block_editor ) && in_array( get_post_type(), AMP_Post_Type_Support::get_supported_post_types(), true ) ); }

westonruter · 2021-08-16T19:05:48Z

src/Editor/EditorSupport.php

-		$screen = get_current_screen();
-
-		if ( ! $screen ) {
-			return;
-		}
-
-		$is_block_editor = (
-			! empty( $screen->is_block_editor )
-			||
-			// Applicable to Gutenberg v5.5.0 and older.
-			( function_exists( 'is_gutenberg_page' ) && is_gutenberg_page() )
-		);
-		if ( ! $is_block_editor ) {
-			return;
-		}
-
-		if ( ! in_array( get_post_type(), AMP_Post_Type_Support::get_eligible_post_types(), true ) ) {
-			return;
-		}
-
-		if ( $this->editor_supports_amp_block_editor_features() ) {
+		if ( $this->is_current_screen_supported_block_editor_for_amp_enabled_post_type() ) {
 			return;
 		}


I just realized that this is currently resulting in the wp_add_inline_script() logic being run for all screens other than the edit post screen. Like if I go to the Dashboard, is_current_screen_supported_block_editor_for_amp_enabled_post_type() is returning false because it's not the block editor.

Maybe putting all that logic in the same method goes too far.

Maybe instead of is_current_screen_supported_block_editor_for_amp_enabled_post_type we need a is_block_editor_screen_for_amp_enabled_post_type which looks like:

public function is_block_editor_screen_for_amp_enabled_post_type() { $screen = get_current_screen(); return ( $screen && ! empty( $screen->is_block_editor ) && in_array( get_post_type(), AMP_Post_Type_Support::get_supported_post_types(), true ) ); }

Then this method can instead be starting with:

if ( $this->dependency_support->has_support() ) { return; } if ( ! $this->is_block_editor_screen_for_amp_enabled_post_type() ) { return; }

Oh right, nice catch! I've made the amendment in 5c93b00.

Co-authored-by: Weston Ruter <[email protected]>

westonruter · 2021-08-18T23:50:03Z

With 99915e9, the \AMP_Post_Meta_Box::enqueue_admin_assets() method is updated for the Classic Editor.

In the following tests, the page post type disabled for AMP and the post post type enabled.

Site with `has_support_from_core`

Post Type	Before	After
`page`	Enqueued `amp-post-meta-box.css` and `amp-post-meta-box.js` 👎	Nothing enqueued. 👍
`post`	Enqueued `amp-post-meta-box.css` and `amp-post-meta-box.js`	Both still enqueued. 👍

To align with the block editor's behavior, the AMP status is now hidden entirely in the classic editor when the post type is disabled for AMP.

Before	After

Site without `has_support_from_core`

Post Type	Before	After
`page`	Enqueued `amp-post-meta-box.css` 👎	Nothing enqueued. 👍
`post`	Enqueued `amp-post-meta-box.css`	Style still enqueued. 👍

Post Type	Before	After
`post`
`page`

westonruter · 2021-08-19T00:11:31Z

For now the Block Editor, again with post enabled for AMP and page disabled for AMP.

Site with `has_support`

`\AMP_Post_Meta_Box::enqueue_block_assets()`

Post Type	Before	After
`post`	Both `amp-block-editor.js` and `amp-block-editor.css` are enqueued. 👍	Both `amp-block-editor.js` and `amp-block-editor.css` are enqueued. 👍
`page`	Both `amp-block-editor.js` and `amp-block-editor.css` are enqueued (and AMP Settings appear in blocks erroneously). 👎	Neither script nor style is enqueued (and no AMP Settings in blocks). 👍

`\AMP_Validation_Manager::enqueue_block_validation()`

Post Type	Before	After
`post`	amp-block-validation script & style enqueued	amp-block-validation script & style still enqueued 👍
`page`	amp-block-validation script & style enqueued (when they should not be) 👎	Neither amp-block-validation script nor style enqueued 👍

`\AmpProject\AmpWP\Editor\EditorSupport::maybe_show_notice()`

No notice should be shown in any scenario since has_support.

Post Type	Before	After
`post`	No notice shown.	No notice shown. 👍
`page`	No notice shown.	No notice shown. 👍

Site without `has_support`

`\AMP_Post_Meta_Box::enqueue_block_assets()`

Post Type	Before	After
`post`	amp-block-editor script/style not enqueued	amp-block-editor script/style not enqueued 👍
`page`	amp-block-editor script/style not enqueued	amp-block-editor script/style not enqueued 👍

`\AMP_Validation_Manager::enqueue_block_validation()`

Post Type	Before	After
`post`	amp-block-validation script/style not enqueued	amp-block-validation script/style not enqueued 👍
`page`	amp-block-validation script/style not enqueued	amp-block-validation script/style not enqueued 👍

`\AmpProject\AmpWP\Editor\EditorSupport::maybe_show_notice()`

Post Type	Before	After
`post`	Notice displayed.	Notice displayed. 👍
`page`	Notice displayed 👎	Notice not shown (since post type not enabled). 👍

westonruter

Good work. This was a tedious one.

Co-authored-by: Pierre Gordon <[email protected]>

westonruter · 2021-08-19T00:30:56Z

Oh, most important of all, this the PHP notice no longer shows up on the widgets screen.

pierlon · 2021-08-19T00:35:28Z

Thanks for carrying this one across the finish line 🙇.

Only load block editor related assets when it is used for a post

38de6f6

pierlon added this to the v2.1.4 milestone Aug 12, 2021

pierlon requested review from westonruter and dhaval-parekh August 12, 2021 14:28

westonruter requested changes Aug 13, 2021

View reviewed changes

pierlon added 5 commits August 13, 2021 21:08

Fix typo in notice message

8b2c888

Ensure block editor related functionality only runs when post type su…

36b96e1

…pports the editor and AMP

Merge branch 'develop' into fix/6521-block-editor-assets

51cf64b

# Conflicts: # tests/php/src/Editor/EditorSupportTest.php

Unset global $current_screen and $wp_scripts before running test

54a0694

Fix Test_AMP_Validation_Manager::test_enqueue_block_validation() test

1a48487

pierlon force-pushed the fix/6521-block-editor-assets branch from f207589 to 1a48487 Compare August 14, 2021 03:40

pierlon added 2 commits August 14, 2021 00:19

Setup environment at correct place

38a4cfd

Set correct user before running assertion

a26b858

pierlon requested a review from westonruter August 14, 2021 04:34

westonruter reviewed Aug 16, 2021

View reviewed changes

Change method name to `is_current_screen_supported_block_editor_for_a…

8235b9f

…mp_enabled_post_type()`

westonruter requested changes Aug 16, 2021

View reviewed changes

Remove dependency support check when checking for block editor on screen

5c93b00

Co-authored-by: Weston Ruter <[email protected]>

pierlon requested a review from westonruter August 17, 2021 00:37

pierlon and others added 5 commits August 16, 2021 20:59

Don't directly set is_block_editor property

5da3c18

Update test assertions for WP < 5.0

07932e4

Simplify test

808dfaf

Shorten method name now that it omits has_support

97f37d0

Omit AMP status from classic post editor when post type not supported

99915e9

westonruter approved these changes Aug 19, 2021

View reviewed changes

westonruter merged commit cd5ceab into develop Aug 19, 2021

westonruter deleted the fix/6521-block-editor-assets branch August 19, 2021 00:26

westonruter added a commit that referenced this pull request Aug 19, 2021

Only load block editor related assets when it is used for a post (#6534)

28932bc

Co-authored-by: Pierre Gordon <[email protected]>

	public function enqueue_block_assets() {
	$post = get_post();
	if ( ! $post instanceof WP_Post \|\| ! in_array( $post->post_type, AMP_Post_Type_Support::get_eligible_post_types(), true ) ) {
	return;
	}

	if ( ! in_array( get_post_type(), AMP_Post_Type_Support::get_eligible_post_types(), true ) ) {
	return;
	}

Only load block editor related assets when it is used for a post #6534

Only load block editor related assets when it is used for a post #6534

Uh oh!

Conversation

pierlon commented Aug 12, 2021

Summary

Checklist

Uh oh!

github-actions bot commented Aug 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

westonruter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pierlon Aug 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

westonruter commented Aug 18, 2021

Site with has_support_from_core

Site without has_support_from_core

Uh oh!

westonruter commented Aug 19, 2021

Site with has_support

\AMP_Post_Meta_Box::enqueue_block_assets()

\AMP_Validation_Manager::enqueue_block_validation()

\AmpProject\AmpWP\Editor\EditorSupport::maybe_show_notice()

Site without has_support

\AMP_Post_Meta_Box::enqueue_block_assets()

\AMP_Validation_Manager::enqueue_block_validation()

\AmpProject\AmpWP\Editor\EditorSupport::maybe_show_notice()

Uh oh!

westonruter left a comment

Choose a reason for hiding this comment

Uh oh!

westonruter commented Aug 19, 2021

Uh oh!

pierlon commented Aug 19, 2021

Uh oh!

Uh oh!

github-actions bot commented Aug 12, 2021 •

edited

Loading

pierlon Aug 13, 2021 •

edited

Loading

Site with `has_support_from_core`

Site without `has_support_from_core`

Site with `has_support`

`\AMP_Post_Meta_Box::enqueue_block_assets()`

`\AMP_Validation_Manager::enqueue_block_validation()`

`\AmpProject\AmpWP\Editor\EditorSupport::maybe_show_notice()`

Site without `has_support`

`\AMP_Post_Meta_Box::enqueue_block_assets()`

`\AMP_Validation_Manager::enqueue_block_validation()`

`\AmpProject\AmpWP\Editor\EditorSupport::maybe_show_notice()`