Make WordPress Core

Opened 2 weeks ago

Closed 9 days ago

Last modified 6 days ago

#54806 closed defect (bug) (fixed)

Site Editor: Remote block patterns are not loaded

Reported by: noisysocks Owned by: hellofromTonya
Milestone: 5.9 Priority: normal
Severity: normal Version:
Component: Editor Keywords: has-patch has-testing-info commit dev-reviewed
Focuses: Cc:

Description

Originally reported in https://github.com/WordPress/gutenberg/issues/37814.

  1. Activate an "empty" theme, e.g. the one that's in the Gutenberg repo. https://github.com/WordPress/gutenberg/tree/trunk/test/emptytheme
  2. Open the Site Editor.
  3. Open the inserter.
  4. Select "Patterns".
  5. Note that many block pattern categories are missing. (They appear in the post editor.)

The problem is that remote patterns are loaded when the current_screen hook is called.

https://github.com/WordPress/wordpress-develop/blob/7f2ddd0ae6f128d4292d7cbd20d9e359bb176609/src/wp-includes/default-filters.php#L335

Before proceeding, this hook checks that $current_screen→is_block_editor is true.

https://github.com/WordPress/wordpress-develop/blob/7f2ddd0ae6f128d4292d7cbd20d9e359bb176609/src/wp-includes/block-patterns.php#L55-L57

But is_block_editor is not set to true until after the current_screen hook is called.

https://github.com/WordPress/wordpress-develop/blob/7f2ddd0ae6f128d4292d7cbd20d9e359bb176609/src/wp-admin/admin.php#L212

https://github.com/WordPress/wordpress-develop/blob/7f2ddd0ae6f128d4292d7cbd20d9e359bb176609/src/wp-admin/site-editor.php#L32

The inserter hides the categories as they are empty.

Unfortunately there is no suitable hook that fires in between the call to $current_screen→is_block_editor( true ) and the call to WP_Block_Patterns_Registry::get_instance()->get_all_registered().

Perhaps we should call _load_remote_block_patterns and _load_remote_featured_patterns directly instead of using a hook? What do you think @hellofromtonya?

Attachments (1)

54806-test-before-after-pr2178.gif (3.8 MB) - added by hellofromTonya 9 days ago.
Test Report: before (can reproduce ✅) and after PR 2178 (resolves issue ✅)

Change History (20)

This ticket was mentioned in PR #2178 on WordPress/wordpress-develop by noisysocks.


10 days ago

  • Keywords has-patch added

Load any remote block patterns from w.org explicitly instead of using the current_screen filter. The current_screen filter is unreliable as, in many cases e.g. the Site Editor, $current_scren->is_block_editor is not set until after the filter is executed.

Trac ticket: https://core.trac.wordpress.org/ticket/54806

#2 @peterwilsoncc
10 days ago

Related #46195

The previous fix was to add some logic to WP_Screen to improve the accuracy of block editor detection, see d575d70fe.

I'm in two minds about the proposed patch in the linked pull request as I worry it could lead to blocking HTTP requests during page load when patterns are not needed.

I'll dig in to the code further and see if my concern is valid.

#3 @noisysocks
10 days ago

I'm in two minds about the proposed patch in the linked pull request as I worry it could lead to blocking HTTP requests during page load when patterns are not needed.

Which screens don't need to load patterns? In my patch, I explicitly load them in the site editor (site-editor.php) and block editor (edit-form-blocks.php) which are the two places they're needed. I'm not loading them in the widgets block editor (widgets-form-blocks.php) as we don't support patterns there yet. There are no other calls to →is_block_editor( true ).

#4 @peterwilsoncc
10 days ago

I'm thinking about _load_remote_block_patterns() being called at an unexpectedly and relying on the is_block_editor check exists to prevent the request if it's not needed. For example, custom pages that use the block editor.

As it is, I don't think it's a problem, I just wanted to hold things up so I could check the plugin repo (it's dandy) and order of operations on a few things.

#5 follow-up: @noisysocks
10 days ago

I could keep the if ( ! $current_screen->is_block_editor ) check in _load_remote_block_patterns for backwards compatibly if we want. Just in case anybody is using it as it's not marked private.

#6 in reply to: ↑ 5 @peterwilsoncc
10 days ago

Replying to noisysocks:

I could keep the if ( ! $current_screen->is_block_editor ) check in _load_remote_block_patterns for backwards compatibly if we want. Just in case anybody is using it as it's not marked private.

Yeah, let's keep it there to avoid the minor break in back-compatibility.

#7 @noisysocks
10 days ago

👍 done.

@hellofromTonya
9 days ago

Test Report: before (can reproduce ✅) and after PR 2178 (resolves issue ✅)

#8 @hellofromTonya
9 days ago

  • Keywords has-testing-info added

Test Report

Env:

  • Theme: empty theme
  • Plugins: none
  • WordPress: trunk
  • Localhost: wp-env
  • Browsers: Firefox, Chrome, Edge, Safari
  • OS: macOS Big Sur

Steps

Theme:

  1. Download files from Gutenberg repo
  2. Activate the empty theme

To test:

  1. Go to Posts > All Posts and edit "Hello world!" post
  2. Toggle open the inserter (click on the + icon next to the WordPress logo)
  3. Click on the Patterns tab. Expected behavior: patterns are loaded into the UI and dropdown is populated for exploring
  4. Go to Appearance > Editor (beta)
  5. Repeat steps 2 and 3. Expected behavior: patterns are loaded into the UI and dropdown is populated for exploring

Test Results

  • Without the PR
    • Patterns available in the block editor
    • Patterns not available in site editor (able to reproduce the reported issue ✅)
  • With the PR applied:
    • Patterns available in the block editor
    • Patterns available in site editor (resolves reported issue ✅)

#9 @hellofromTonya
9 days ago

  • Keywords commit added

Capturing comments from my review of the PR:

  • The changes apply to only the block editor and site editor screens 👍
  • Function signature deprecation is consistent with other instances in Core 👍
  • Backwards compatibility functionality in _load_remote_block_patterns() is retained 👍
  • In testing, this PR resolves the reported issue (see the test report) while not impacting the block editor 👍

The solution works around the current_screen limitations to directly trigger remote pattern loading when on block or site editor screens. Should not impact other UIs/screens. I think this makes sense.

LGTM 👍 Marking for commit.

What do you think @peterwilsoncc?

#10 @audrasjb
9 days ago

  • Keywords dev-reviewed added

I tested the proposed PR and it fixes the issue: patterns are available in both the block and site editors.

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


9 days ago

#12 @peterwilsoncc
9 days ago

Thanks @noisysocks, with the latest push the linked pull request looks commit ready to me.

Thanks for the gif and testing notes @hellofromTonya

#13 @hellofromTonya
9 days ago

  • Owner set to hellofromTonya
  • Status changed from new to reviewing

Cool. Thanks @audrasjb and @peterwilsoncc for testing and reviewing. Let's it committed and backported w00t

Self-assigning for commit.

#14 @hellofromTonya
9 days ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 52593:

Editor: Explicitly load remote block patterns in the block and site editor screens.

Remote block patterns from wp.org were to be loaded through a callback hooked into the current_screen filter. Within 2 callbacks, i.e. _load_remote_featured_patterns() and _load_remote_block_patterns(), a guard clause bailed out early if the $current_screen->is_block_editor is false.

However, the current_screen filter is unreliable to detect the block editor. Why? In the block and Site Editor screens, $current_scren->is_block_editor is not set until after the filter is executed. Whoopsie.

This commit no longer uses the current_screen filter. Instead, it explicitly loads the remote block patterns by invoking both private functions (now not callbacks) directly in the screen files for the block and site editor screens.

With this change, passing WP_Screen object into these functions is no longer needed. As the _load_remote_block_patterns() function was introduced in 5.8.0, its function parameter is now deprecated and the guard clause retained for backwards compatibility.

Follow-up to [51021], [52377].

Props poena, noisysocks, peterwilsoncc, hellofromTonya, audrasjb.
Fixes #54806.

#16 @hellofromTonya
9 days ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening to backport [52593] to 5.9. Thank you @audrasjb for the 2nd committer review!

#17 @hellofromTonya
9 days ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 52594:

Editor: Explicitly load remote block patterns in the block and site editor screens.

Remote block patterns from wp.org were to be loaded through a callback hooked into the current_screen filter. Within 2 callbacks, i.e. _load_remote_featured_patterns() and _load_remote_block_patterns(), a guard clause bailed out early if the $current_screen->is_block_editor is false.

However, the current_screen filter is unreliable to detect the block editor. Why? In the block and Site Editor screens, $current_scren->is_block_editor is not set until after the filter is executed. Whoopsie.

This commit no longer uses the current_screen filter. Instead, it explicitly loads the remote block patterns by invoking both private functions (now not callbacks) directly in the screen files for the block and site editor screens.

With this change, passing WP_Screen object into these functions is no longer needed. As the _load_remote_block_patterns() function was introduced in 5.8.0, its function parameter is now deprecated and the guard clause retained for backwards compatibility.

Follow-up to [51021], [52377].

Props poena, noisysocks, peterwilsoncc, hellofromTonya, audrasjb.
Merges [52593] to the 5.9 branch.
Fixes #54806.

#18 @hellofromTonya
9 days ago

Alrighty, this has been committed to trunk and backported to 5.9 branch. Thank you everyone for your contributions and to @poena for reporting it.

This ticket was mentioned in Slack in #core-editor by poena. View the logs.


6 days ago

Note: See TracTickets for help on using tickets.