REST API: Use posts endpoint for reusable blocks #10751

aduth · 2018-10-18T17:04:47Z

Related: #7453
Related: https://core.trac.wordpress.org/ticket/45098

This pull request seeks to refactor reusable blocks to avoid the need for (and thus enable removal of) the blocks API controller, in favor of the existing posts controller. Edit 2018-10-20: The controller class remains, solely for its custom check_read_permission implementation to avoid public visibility of published reusable blocks.

In implementing these changes, the custom controller was found to be used to:

Return a limited subset of post endpoint properties (id, title, content)
Flatten title and content fields (raw string used, no object of raw/rendered)
Force delete on trash
Force publish on save

My assessment of this is:

The first two points introduce a needless and unexpected inconsistency from the posts response
The last two points should be enforced at a post-type level if desirable
- Are they desirable?
A subtlety of the second point is that now a user needs to have edit capabilities for the wp_block type to be able to view it in the editor, or to export the block. This is because the raw content (post.content.raw) is only included when the context=edit parameter is passed
- This is perhaps a main deterrent to proceeding here, though is symptomatic of a larger issue around REST permissions where the prior implementation was an ad hoc workaround (effect of bumping permissions for otherwise unqualified users).

Post type supports title and editor were required to be added for the title and content fields to be included in the REST response [1] [2]. See also related discussion in #9964 (the changes here enable a Gutenberg editor for the Reusable Blocks management experience, thereby effectively closing #9964).

Open questions:

Should there be / how would there be a deprecation migration period for the controller removal?

Testing Instructions:

Verify no regressions in behavior of reusable blocks:

Creating / updating
Inserting
Managing all
Single block vs. template
Import / export

danielbachhuber

👍 Haven't tested but conceptually this is fine by me.

slimmilkduds · 2018-10-18T17:22:01Z

This wouldn’t by any chance fix (or be a step in fixing) the issue with preserving nested blocks when converting reusable blocks to regular blocks? (holding thumbs but is pretty sure it has nothing to do with that issue ☺️)

aduth · 2018-10-18T17:35:30Z

This is a refactoring change. It should have no impact (for better or worse) on existing UI behavior.

noisysocks · 2018-10-19T05:43:02Z

Good thing I searched GitHub before working on literally this! 😛

There's no reason to have the custom endpoint. It stems from a time when it wasn't yet clear that reusable blocks would literally be mini Gutenberg posts that are embedded into a larger Gutenberg post.

Will review and test this now. Let's get it in for the API freeze.

Should there be / how would there be a deprecation migration period for the controller removal?

Not sure how we'd deprecate this without changing the URL, which isn't ideal.

Maybe we could add an error_log warning to 4.1 which informs developers that the endpoint will be changed in 4.2?

noisysocks

This works great! 👏

I left some suggestions for improvements—I'd really like if we kept the test_capabilities integration test so that it's hard to introduce a security regression down the line.

I suggest we wait for 4.2 to merge this and add a warning to 4.1 which logs that the route is changing.

noisysocks · 2018-10-19T05:56:43Z

phpunit/class-rest-blocks-controller-test.php

-	 *
-	 * @dataProvider data_capabilities
-	 */
-	public function test_capabilities( $action, $role, $expected_status ) {


I'd like to keep this integration test. #3936 was a nasty bug which I don't want to regress, and it's a pain to manually test the CPT capabilities since one has to check every role/action combination.

See #4725 for context.

I'd like to keep this integration test.

Restored in e3b1279.

For what it's worth, it did prove to be a very useful test! I had found we in-fact need the controller for check_read_permission, because the default REST post controller otherwise ignores capabilities if the post is published.

https://github.com/WordPress/WordPress/blob/da7a80d67fea29c2badfc538bfc01c8a585f0cbe/wp-includes/rest-api/endpoints/class-wp-rest-posts-controller.php#L1319

Discussed in Slack:

https://wordpress.slack.com/archives/C02RQC26G/p1539972633000100

noisysocks · 2018-10-19T06:16:18Z

lib/register.php

+			'supports'        => array(
+				'title',
+				'editor',
+			),


Because the CPT now supports title and editor, we can go a step further and remove this line from gutenberg.php:267:

unset( $actions['edit'] );

This PR would then close #9964.

Because the CPT now supports title and editor, we can go a step further and remove this line from gutenberg.php:267:

Updated in c8ad73a

Oops, spoke too soon!

noisysocks

Sorry, I spoke too soon! 😅

I'm noticing two small regressions:

When exporting a reusable block as JSON, the saved file name is always json.json instead of e.g. my-reusable-block.json.
Importing a reusable block from JSON always fails with Invalid Reusable Block JSON file.

noisysocks · 2018-10-19T06:26:16Z

packages/editor/src/store/effects/reusable-blocks.js

@@ -184,7 +184,10 @@ export const deleteReusableBlocks = async ( action, store ) => {
 	] ) );

 	try {
-		await apiFetch( { path: `/wp/v2/${ postType.rest_base }/${ id }`, method: 'DELETE' } );
+		await apiFetch( {
+			path: `/wp/v2/${ postType.rest_base }/${ id }?force=true`,


Thinking about this some more, I think we should remove the ?force=true so that deleting a reusable block in Gutenberg matches what happens when you delete a reusable block in the Manage Reusable Blocks interface.

I'm okay with this. As alluded to, seems like it would make sense to have this be controlled at the post-type level, for the sort of consistency you note. In the meantime, letting "Trashed" do its thing seems quite okay with me.

Related to this one, we'll need to do similar with the forced "Published" status.

aduth · 2018-10-19T18:51:58Z

phpunit/class-rest-blocks-controller-test.php

@@ -8,7 +8,7 @@
 /**
 * Tests for WP_REST_Blocks_Controller.
 */
-class REST_Blocks_Controller_Test extends WP_Test_REST_Controller_Testcase {
+class REST_Blocks_Controller_Test extends WP_UnitTestCase {


This change might make someone somewhere upset, but implementing 9 abstract methods when the controller implements only check_read_permission is excessive, and would hint more at unreliability of tests for the extended posts controller.

implementing 9 abstract methods when the controller implements only check_read_permission is excessive, and would hint more at unreliability of tests for the extended posts controller.

Yeah, it's annoying. We can improve upon it at some point.

I guess another option could be extending WP_Test_REST_Posts_Controller, to inherit the tests implemented there we're assuming for our controller. Still lots of redundancy.

Or another way to signal "this is already tested elsewhere"? Extend WP_Test_REST_Controller_Testcase with the methods implemented as simple $this->skip with a message explaining?

(Not going to let this block merge)

aduth · 2018-10-19T18:56:09Z

I'm noticing two small regressions:

The two were in-fact the same, fixed in 0235f5d.

It reminded me also that since we pass context=edit, we need to check that the user would be able to edit the post before providing the export option: ac75c14

youknowriad

LGTM 👍

Feedback addressed. Separate technical review.

noisysocks · 2018-10-22T23:33:55Z

Thanks @aduth! Looks great.

* REST API: Use posts endpoint for reusable blocks * REST: Restore WP_REST_Blocks_Controller for permissions check * Reusable Blocks: Enable post listing edit * Reusable Blocks: Trash on delete action * List Reusable Blocks: Provide context to post request * Reusable Blocks: Verify edit capability on export action * List Reusable Blocks: Import as published

REST API: Use posts endpoint for reusable blocks

e681848

aduth added [Feature] Synced Patterns Related to synced patterns (formerly reusable blocks) REST API Interaction Related to REST API labels Oct 18, 2018

aduth requested review from danielbachhuber, youknowriad, desrosj and noisysocks October 18, 2018 17:04

danielbachhuber approved these changes Oct 18, 2018

View reviewed changes

aduth mentioned this pull request Oct 18, 2018

WIP: Blocks: Reimplement shared block as embedded editor #7453

Closed

noisysocks added this to the 4.2 - API freeze milestone Oct 19, 2018

noisysocks mentioned this pull request Oct 19, 2018

Reusable Blocks: "Manage All Reusable Blocks" single item edit experience #9964

Closed

noisysocks previously approved these changes Oct 19, 2018

View reviewed changes

noisysocks previously requested changes Oct 19, 2018

View reviewed changes

aduth added 5 commits October 19, 2018 14:32

REST: Restore WP_REST_Blocks_Controller for permissions check

e3b1279

Reusable Blocks: Enable post listing edit

c8ad73a

Reusable Blocks: Trash on delete action

9c1da9e

List Reusable Blocks: Provide context to post request

0235f5d

Reusable Blocks: Verify edit capability on export action

ac75c14

aduth commented Oct 19, 2018

View reviewed changes

List Reusable Blocks: Import as published

921f669

youknowriad approved these changes Oct 19, 2018

View reviewed changes

youknowriad modified the milestones: 4.2, 4.1 - UI freeze Oct 19, 2018

aduth merged commit 611fe31 into master Oct 19, 2018

aduth deleted the remove/blocks-controller branch October 19, 2018 19:23

designsimply mentioned this pull request Oct 24, 2018

Prevent Reusable Blocks from losing changes #4876

Closed

aduth mentioned this pull request Nov 1, 2018

Expose rendered blocks in REST API endpoint for Reusable Blocks #8625

Closed

noisysocks mentioned this pull request Nov 5, 2018

Add the core/template block to reusable blocks that only has a single block #10271

Closed

noisysocks mentioned this pull request Nov 14, 2018

Fetch reusable blocks in the view context on first render. #11343

Closed

noisysocks mentioned this pull request Feb 14, 2019

User flow and meaning of 'manage reusable blocks' unclear #10435

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

REST API: Use posts endpoint for reusable blocks #10751

REST API: Use posts endpoint for reusable blocks #10751

aduth commented Oct 18, 2018 •

edited

Loading

danielbachhuber left a comment

slimmilkduds commented Oct 18, 2018

aduth commented Oct 18, 2018

noisysocks commented Oct 19, 2018

noisysocks left a comment •

edited

Loading

noisysocks Oct 19, 2018

aduth Oct 19, 2018

noisysocks Oct 19, 2018

aduth Oct 19, 2018

noisysocks left a comment

noisysocks Oct 19, 2018

aduth Oct 19, 2018

aduth Oct 19, 2018

aduth Oct 19, 2018

danielbachhuber Oct 19, 2018

aduth Oct 19, 2018

aduth commented Oct 19, 2018

youknowriad left a comment

noisysocks commented Oct 22, 2018

REST API: Use posts endpoint for reusable blocks #10751

REST API: Use posts endpoint for reusable blocks #10751

Conversation

aduth commented Oct 18, 2018 • edited Loading

danielbachhuber left a comment

Choose a reason for hiding this comment

slimmilkduds commented Oct 18, 2018

aduth commented Oct 18, 2018

noisysocks commented Oct 19, 2018

noisysocks left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noisysocks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth commented Oct 19, 2018

youknowriad left a comment

Choose a reason for hiding this comment

noisysocks commented Oct 22, 2018

aduth commented Oct 18, 2018 •

edited

Loading

noisysocks left a comment •

edited

Loading