WordPress.org

Make WordPress Core

Opened 5 months ago

Closed 3 months ago

#45098 closed task (blessed) (fixed)

Introduce WP_REST_Block_Renderer_Controller and WP_REST_Blocks_Controller classes

Reported by: danielbachhuber Owned by: desrosj
Milestone: 5.0 Priority: normal
Severity: normal Version:
Component: REST API Keywords: has-patch has-unit-tests fixed-5.0
Focuses: rest-api Cc:

Description (last modified by danielbachhuber)

The WP_REST_Block_Renderer_Controller and WP_REST_Blocks_Controller classes currently exist in Gutenberg. They should be in WordPress core instead.

Related https://github.com/WordPress/gutenberg/issues/10655

Attachments (7)

45098.diff (38.2 KB) - added by desrosj 5 months ago.
45098.2.diff (38.3 KB) - added by desrosj 5 months ago.
45098.3.diff (65.6 KB) - added by danielbachhuber 5 months ago.
45098.4.diff (1.7 KB) - added by danielbachhuber 5 months ago.
45098.5.diff (4.6 KB) - added by peterwilsoncc 5 months ago.
45098.6.diff (3.0 KB) - added by danielbachhuber 5 months ago.
Includes additional labels for 'wp_block' post type.
45098.7.diff (5.2 KB) - added by peterwilsoncc 5 months ago.

Download all attachments as: .zip

Change History (41)

#1 @danielbachhuber
5 months ago

  • Description modified (diff)

#2 @danielbachhuber
5 months ago

  • Owner set to desrosj
  • Status changed from new to assigned

#3 @desrosj
5 months ago

  • Keywords has-patch has-unit-tests added; needs-patch needs-unit-tests removed

Currently blocked by this PR to Gutenberg and #45097.

Attaching a preliminary patch as a starting point for after those two blockers are resolved.

@desrosj
5 months ago

#4 @TimothyBlynJacobs
5 months ago

Why is Gutenberg registering a REST route for every registered block type. It looks like this should be a single route. It appears to already be retrieving the block name from the request object.

This ticket was mentioned in Slack in #core-restapi by desrosj. View the logs.


5 months ago

#6 @desrosj
5 months ago

It appears that dynamic route registration was added in https://github.com/WordPress/gutenberg/pull/5602, more specifically 6557877.

$block->attributes being dynamically passed to the properties for each endpoint seems to be the reason behind that.

#7 @aduth
5 months ago

While effort has stalled, part of the changes included in the following pull request had proposed the removal of the custom controller for reusable blocks:

https://github.com/WordPress/gutenberg/pull/7453

It's not entirely relevant to the main point of the pull request, but it had been prompted in the course of its development as a question: Is there really any reason we need custom endpoints, vs. simply using the existing /wp/v2/posts endpoints (as reusable blocks are simply posts of type wp_block)?

#8 @danielbachhuber
5 months ago

@aduth That seems like a substantial architectural change to be making at this point. Do you see that landing in the next two days?

#9 @aduth
5 months ago

Not likely.

This ticket was mentioned in Slack in #core-restapi by desrosj. View the logs.


5 months ago

@desrosj
5 months ago

#11 @desrosj
5 months ago

To summarize conversations in Slack, @aduth is experimenting today with removing the /blocks endpoint today in favor of using the pre-existing /posts endpoint.

In case the results of that experiment are not promising, I have attached a patch for moving the WP_REST_Block_Renderer_Controller and WP_REST_Blocks_Controller classes over to core in their current state.

The tests are currently failing for the patch due to the absence of the wp_block post type in core.

This ticket was mentioned in Slack in #core-restapi by desrosj. View the logs.


5 months ago

#13 @aduth
5 months ago

Pull request for aforementioned blocks controller removal from Gutenberg:

https://github.com/WordPress/gutenberg/pull/10751

This ticket was mentioned in Slack in #core-restapi by desrosj. View the logs.


5 months ago

#15 @danielbachhuber
5 months ago

In 45098.3.diff, I've incorporated the changes in https://github.com/WordPress/gutenberg/pull/10751

Notably, we still need the wp_block post type registered before 45098.3.diff can land.

#16 @pento
5 months ago

In 43804:

Blocks: Add the reusable block post type, wp_block.

See #45098.

#17 @pento
5 months ago

  • Keywords fixed-5.0 added

Endpoints landed in [43805].

#18 @swissspidy
5 months ago

Minor note about coding standards in [43804]:

  • Missing space in post type label name: __( 'Blocks')
  • Arrow not aligned for capabilities key (contrary to e.g. supports or labels

#19 @peterwilsoncc
5 months ago

In [43804] the following meta caps can be removed as they are mapped but not used due to the capabilities array when registering the post:

  • edit_published_blocks
  • delete_published_blocks
  • edit_others_blocks
  • delete_others_blocks

@pento As mentioned in our conversation in Slack, I am going to keep digging into this and think about custom roles that allow editing of pages but not posts. At the moment, the page editor can't edit shared blocks.

#20 @pento
5 months ago

I had to add those caps to the post registration, as edit_post and delete_post ultimately map to them.

I'm undecided about what to do with CPTs. I'm inclined to leave the behaviour as is, if a CPT chooses to allow reusable blocks, they need to take that into account in how they map caps to roles.

#21 @Presskopp
5 months ago

Hi, there's a period missing at the end of "Sorry, you are not allowed to read blocks of this post"

wp-includes/rest-api/endpoints/class-wp-rest-block-renderer-controller.php:96
Last edited 5 months ago by Presskopp (previous) (diff)

#22 @danielbachhuber
5 months ago

In 43806:

Blocks: Fix coding standards introduced in [43804].

Props Presskopp, swissspidy.
See #45098

#23 @peterwilsoncc
5 months ago

I'm inclined to leave the behaviour as is, if a CPT chooses to allow reusable blocks, they need to take that into account in how they map caps to roles.

Is there a reason to avoid adding the new *_blocks caps via an upgrade routine in core?

Given WP allows custom roles, I think mapping the caps to a particular post type causes an unnecessary break to back-compat and is therefore doing it wrong.

The more I think about this, I'm more convinced that the plugin was doing it correctly. The meta caps can continue to use edit_post, etc as that's the standard behaivour for attachments and other CPTs.

#24 @peterwilsoncc
5 months ago

I've found an error when an author or contributor attempts to reuse a block created by another user.

When inserting a block, the author can read others blocks and it inserts correctly.

When the author attempts to go back and edit their post, the editor attempts to fetch the blocks in the edit context, causing any user without the edit_others_posts permission to see the error "Block has been deleted or is unavailable."

To reproduce:

  1. Create two users, user1 (any role) and user2 (author role)
  2. Log in as user1 and create a reusable block
  3. Log in as user2 and create a post using user1's block
  4. The block renders correctly.
  5. Save the post and refresh the edit screen.
  6. The block fails to render and displays the error "Block has been deleted or is unavailable."

Note: This happens with the current version and in my proposed patch.

---

In 45098.5.diff, I've modified the permissions to use those as originally used by the feature plugin to account for custom roles and capabilities.

The meta cap changes are no longer required as the primitive caps exist and the edit_post and other default meta caps map to the post types primitive.

Last edited 5 months ago by peterwilsoncc (previous) (diff)

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


5 months ago

@danielbachhuber
5 months ago

Includes additional labels for 'wp_block' post type.

#27 @danielbachhuber
5 months ago

In 43849:

Blocks: Include necessary labels for the 'wp_block' post type.

See #45098

#28 @peterwilsoncc
5 months ago

45098.7.diff is 45098.5.diff but with the roles upgrade routine modified to avoid updating the option 33 odd times.

  • modified the permissions to use those as originally used by the feature plugin to account for custom roles and capabilities.
  • The meta cap changes are no longer required as the primitive caps exist and the edit_post and other default meta caps map to the post types primitive.

#29 @peterwilsoncc
4 months ago

I've realised 45098.7.diff won't work, it doesn't re-instantiate $wp_roles and even if it did, any classes storing them as a property won't get the new ones.

#30 @peterwilsoncc
4 months ago

I've created a follow up ticket for the roles/caps issue so it doesn't get lost in the fixed-5.0 keyword. See #45423.

#31 @jeremyfelt
3 months ago

In 44146:

Blocks: Add the reusable block post type, wp_block.

Merges [43804] from the 5.0 branch to trunk.

See #45098.

#32 @jeremyfelt
3 months ago

In 44150:

REST API: Add endpoints for blocks.

WP_REST_Block_Renderer_Controller allows rendering of server-side rendered blocks, whilst WP_REST_Blocks_Controller allows retrieving of reusable blocks.

Merges [43805] and [43806] from the 5.0 branch to trunk.

Props desrosj, danielbachhuber, pento, Presskopp, swissspidy.
See #45065, #45098.

#33 @jeremyfelt
3 months ago

In 44215:

Blocks: Include necessary labels for the 'wp_block' post type.

Merges [43849] from the 5.0 branch to trunk.

Props danielbachhuber.
See #45098.

#34 @SergeyBiryukov
3 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed
Note: See TracTickets for help on using tickets.