Skip to content

backport Rename Reusable blocks to Patterns#4646

Closed
glendaviesnz wants to merge 17 commits into
WordPress:trunkfrom
glendaviesnz:backport/changes-to-reusable-blocks
Closed

backport Rename Reusable blocks to Patterns#4646
glendaviesnz wants to merge 17 commits into
WordPress:trunkfrom
glendaviesnz:backport/changes-to-reusable-blocks

Conversation

@glendaviesnz
Copy link
Copy Markdown

@glendaviesnz glendaviesnz commented Jun 20, 2023

Renames the Reusable blocks to Patterns. Also adds the option to convert a block or collection of blocks to a non-synced Pattern. This then behaves as imported Patterns do in that once inserted they can be edited completely independently of any other instances of the Pattern. The synced version of the Pattern works as the existing Reusable blocks do in that editing one instance updates all instances.

From: WordPress/gutenberg#51144

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

Testing

Full testing would require GB plugin 16.1 installing.

  • Before switching to PR branch add some reusable blocks
  • Switch to branch and make sure the existing reusable blocks still work as expected
  • Go to /wp-admin/edit.php?post_type=wp_block and check that the existing blocks have Sync Status set to Synced in the right post properties panel
  • Add a new post and add a block or collection of blocks
  • Select the block(s) and in the block toolbar overflow menu select the Create a pattern menu option
  • The sync toggle will be off by default - leave it that way and add the new pattern
  • In the block inspector search for the new pattern name and insert
  • Check that inserted blocks are independent/stand-alone blocks and not a synced reusable block
  • Repeat the above but toggle sync on and make sure the saved block behaves the same as the existing reusable blocks
  • Go to /wp-admin/edit.php?post_type=wp_block and make sure the synced and unsynced Patterns show. Edit Patterns from this page and check that toggling the sync status in right panel works as expected

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

@glendaviesnz
Copy link
Copy Markdown
Author

glendaviesnz commented Jun 21, 2023

Any thoughts on how to best test this? I did look at adding a unit test to tests/phpunit/tests/rest-api/rest-blocks-controller.php but the wp_register_wp_block_postmeta filter is not run there because they are unit tests so the postmeta is not returned.

@glendaviesnz glendaviesnz marked this pull request as ready for review June 21, 2023 03:14
Copy link
Copy Markdown
Contributor

@tellthemachines tellthemachines left a comment

Choose a reason for hiding this comment

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

Changes LGTM! Just one small nit below.

Testing locally, I'm seeing the changes in /wp-admin/edit.php?post_type=wp_block.

Comment thread src/wp-includes/block-editor.php
Comment thread src/wp-includes/post.php
@@ -282,26 +282,26 @@ function create_initial_post_types() {
'wp_block',
array(
'labels' => array(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I was going to say this one could have a @since 6.3.0 note too, but then I checked the create_initial_post_types doc block and there were no additions of the sort for previous changes, so I guess we might as well be consistent and leave it out 😅

Comment thread src/wp-includes/post.php Outdated
Comment on lines +8029 to +8030
*
* @return void
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
*
* @return void

WP-Dev doesn't include @return void annotations.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done.

Comment thread src/wp-includes/post.php Outdated
*
* @since 6.3.0
*
* @see https://github.com/WordPress/gutenberg/pull/51144
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @see https://github.com/WordPress/gutenberg/pull/51144
* @link https://github.com/WordPress/gutenberg/pull/51144

Nit but I think @see is intended for internal function and hook links.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done.

Comment thread src/wp-includes/post.php Outdated
*
* @return void
*/
function wp_register_wp_block_postmeta() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
function wp_register_wp_block_postmeta() {
function wp_register_initial_post_meta() {

WP registers all the post types in one function (along with statuses), create_initial_post_types, so I'm leaning towards using a generic function name to allow for additional meta data to be registered in the future.

Obviously if this changes then the equivalent is needed in default-actions.php -- would this cause problems in GB?

Copy link
Copy Markdown
Author

@glendaviesnz glendaviesnz Jun 23, 2023

Choose a reason for hiding this comment

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

That makes sense. Done. I am not sure if it affects GB. @tellthemachines do you know if we need to update the GB gutenberg_wp_block_register_post_meta to gutenberg_register_initial_post_meta to match this change?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we want to stick with the create_initial naming pattern? We did most recently in create_initial_theme_features.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah, that makes sense to me. Sorry for the bad steer, Glen.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

no worries, have changed to _create_initial_

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

do you know if we need to update the GB gutenberg_wp_block_register_post_meta to gutenberg_register_initial_post_meta to match this change

I don't think so, but we'll need to preserve the existing code in GB in the compat folder for use with previous versions of WP.

Comment thread src/wp-includes/post.php
Comment on lines +8043 to +8052
'show_in_rest' => array(
'schema' => array(
'type' => 'string',
'properties' => array(
'sync_status' => array(
'type' => 'string',
),
),
),
),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
'show_in_rest' => array(
'schema' => array(
'type' => 'string',
'properties' => array(
'sync_status' => array(
'type' => 'string',
),
),
),
),
'show_in_rest' => array(
'schema' => array(
'type' => 'string',
'enum' => array( 'fully', 'unsynced' ),
),
),

This schema entry is incorrect. It's using the properties keyword which would only be used when registering a meta key with a type of object.

We should add an enum keyword that describes the possible values the meta field can contain.

Copy link
Copy Markdown
Author

@glendaviesnz glendaviesnz Jun 23, 2023

Choose a reason for hiding this comment

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

Thanks @TimothyBJacobs. It was initially set as an object, but we reverted to a string, but didn't update the schema correctly. I have updated and set the enum to array( 'partial', 'unsynced' ) as at this point there won't be a fully option as the fully synced blocks don't set the post meta in order to maintain backwards compat with existing reusable blocks. We will be adding a partial option post 6.3 though. Should we add that now, or should I just leave the enum as array( 'unsynced' ) for now?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We will be adding a partial option post 6.3 though. Should we add that now, or should I just leave the enum as array( 'unsynced' ) for now?

No, we shouldn't add it until it becomes available as an option.

at this point there won't be a fully option as the fully synced blocks don't set the post meta in order to maintain backwards compat with existing reusable blocks.

Have ya'll considered setting the default to fully in register_post_meta? What does the client send when they want to set the reusable block to the equivalent of "fully"?

@TimothyBJacobs
Copy link
Copy Markdown
Member

I would say exposing this value through meta is a bit odd for Core. Since this is a first party field, we can expose it as top-level property like sync_status. That'd happen through either modifying the WP_REST_Blocks_Controller or using register_rest_field.

Comment thread src/wp-includes/post.php Outdated
@glendaviesnz
Copy link
Copy Markdown
Author

I would say exposing this value through meta is a bit odd for Core. Since this is a first party field, we can expose it as top-level property like sync_status. That'd happen through either modifying the WP_REST_Blocks_Controller or using register_rest_field.

That makes sense @TimothyBJacobs, but would require changes to Gutenberg which I think is about to have the RC version cut. Can we keep it as is for the first beta and update it after that is out? It is a minor implementation detail and doesn't affect the underlying data structure so doesn't seem like it will have any impact to do this as a follow up. What do you think?

@TimothyBJacobs
Copy link
Copy Markdown
Member

Yep that seems fine to me.

Copy link
Copy Markdown
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

@glendaviesnz Nothing crucial to add to previous reviews, LGTM!

Just two tiny documentation standard nit-picks that would be good to address.

Comment thread src/wp-includes/post.php Outdated
Comment thread src/wp-includes/block-editor.php Outdated
glendaviesnz and others added 2 commits June 26, 2023 09:56
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
@glendaviesnz
Copy link
Copy Markdown
Author

Thanks for the reviews everyone!

Comment thread src/wp-includes/post.php Outdated
Co-authored-by: Peter Wilson <519727+peterwilsoncc@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

@peterwilsoncc peterwilsoncc left a comment

Choose a reason for hiding this comment

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

LGTM, thanks Glen!

@tellthemachines
Copy link
Copy Markdown
Contributor

Committed in r56030 / b524268.

Comment thread src/wp-includes/post.php
'title',
'editor',
'revisions',
'custom-fields',
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is custom fields support actually needed?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

In my testing the postmeta sync_status field was not returned without this.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

We are however going to look at returning it via REST as a top-level sync_status field rather than as post meta so we may be able to drop custom fields support if we go that way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

No open projects

Development

Successfully merging this pull request may close these issues.

6 participants