Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add block types REST API. #21065

Merged
merged 37 commits into from
May 26, 2020
Merged

Conversation

spacedmonkey
Copy link
Member

@spacedmonkey spacedmonkey commented Mar 22, 2020

Description

Add a simple REST API to allow get registered Block types.

How has this been tested?

The REST API request a logged in user to have edit_post capability.
APIs

  • __experimental/block-types List of all blocks
  • __experimental/block-types/<namespace>/<block_type> ie __experimental/block-types/core/post-author". Get details on a single block type
  • __experimental/block-types/<namespace> ie __experimental/block-types/core . Get all blocks in a single name space.

fixes #2751
Trac ticket: #47620

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@spacedmonkey
Copy link
Member Author

CC @kadamwhite for visibility.

@aduth
Copy link
Member

aduth commented Mar 23, 2020

Regarding the question at #2751 (comment):

Can anyone interested please take a look and see if this REST API has the required fields and data. Remember we can add fields later.

I don't know that it's been audited for accuracy lately, but the original Block Registration RFC might be a good source of properties expected to be needed?

https://github.com/WordPress/gutenberg/blob/master/docs/rfc/block-registration.md#block-api

@gziolo has been working on some of the specifics around block.json and may know more.

@aduth aduth added the REST API Interaction Related to REST API label Mar 23, 2020
@spacedmonkey
Copy link
Member Author

@aduth Worth noting the current fields that are in this API are based on what is currently register when you register a block on the PHP. I have a ticket in trac in #48529. This PR will only cover the existing fields for now. Adding the extra fields will covered in another PR or core patch.

@gziolo
Copy link
Member

gziolo commented Mar 26, 2020

@aduth Worth noting the current fields that are in this API are based on what is currently register when you register a block on the PHP. I have a ticket in trac in #48529. This PR will only cover the existing fields for now. Adding the extra fields will covered in another PR or core patch.

Yes, we discussed it already. The work was divided into two parts on Trac and this one covers what we had before on the server before we started introducing block.json. It aligns with Block Registration RFC. is_dynamic is the only property that wasn't included there but it's fine as this is something that we use on the server only.

__experimental/block-types?namespace=<namespace> ie __experimental/block-types/?namespace=core . Get all blocks in a single name space.

I don't know what are patterns used for other endpoints in core, but I'm curious why not use __experimental/block-types/<namespace>?

@gziolo gziolo requested review from kadamwhite and hypest March 26, 2020 08:40
@TimothyBJacobs
Copy link
Member

I don't know what are patterns used for other endpoints in core, but I'm curious why not use __experimental/block-types/?

+1 to this pattern.

@spacedmonkey
Copy link
Member Author

@aduth @gziolo Can either of you review this PR. A lot of work as gone into it and no eyes are on it yet.

lib/class-wp-rest-block-types-controller.php Show resolved Hide resolved
lib/class-wp-rest-block-types-controller.php Outdated Show resolved Hide resolved
lib/class-wp-rest-block-types-controller.php Outdated Show resolved Hide resolved
lib/class-wp-rest-block-types-controller.php Outdated Show resolved Hide resolved
lib/class-wp-rest-block-types-controller.php Outdated Show resolved Hide resolved
lib/class-wp-rest-block-types-controller.php Show resolved Hide resolved
lib/class-wp-rest-block-types-controller.php Outdated Show resolved Hide resolved
lib/class-wp-rest-block-types-controller.php Outdated Show resolved Hide resolved
lib/class-wp-rest-block-types-controller.php Outdated Show resolved Hide resolved
lib/class-wp-rest-block-types-controller.php Outdated Show resolved Hide resolved
spacedmonkey and others added 4 commits May 7, 2020 16:50
Co-authored-by: Andrew Duthie <[email protected]>
Co-authored-by: Andrew Duthie <[email protected]>
Co-authored-by: Andrew Duthie <[email protected]>
Co-authored-by: Andrew Duthie <[email protected]>
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

@gziolo Anything you think is needed here, functionally-speaking?

lib/class-wp-rest-block-types-controller.php Outdated Show resolved Hide resolved
lib/class-wp-rest-block-types-controller.php Show resolved Hide resolved
@aduth
Copy link
Member

aduth commented May 8, 2020

Would we plan to include unit tests here? Or later in the experimental lifecycle?

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Anything you think is needed here, functionally-speaking?

I had some minor comments but nothing blocking. As far as I remember there was a follow-up on Track that adds the rest of block fields/properties.

lib/class-wp-rest-block-types-controller.php Outdated Show resolved Hide resolved
lib/class-wp-rest-block-types-controller.php Show resolved Hide resolved
lib/class-wp-rest-block-types-controller.php Show resolved Hide resolved
@TimothyBJacobs
Copy link
Member

Overall looks good, I'd really like to see tests on the prepare item with the different supported properties.

@spacedmonkey
Copy link
Member Author

Overall looks good, I'd really like to see tests on the prepare item with the different supported properties.

@TimothyBJacobs Do you need these now before approval or before core merge?

@TimothyBJacobs
Copy link
Member

I'd prefer to have them now, without them it is hard for me to tell if they are actually outputting what is expected.

@spacedmonkey
Copy link
Member Author

spacedmonkey commented May 25, 2020

I'd prefer to have them now, without them it is hard for me to tell if they are actually outputting what is expected.

@TimothyBJacobs Does this test cover prepare? https://github.com/WordPress/gutenberg/pull/21065/files#diff-c43bb246f80bf07e6c00d6fdb3427794R129-R149

@TimothyBJacobs
Copy link
Member

That covers when invalid data is provided, but not the positive case.

@spacedmonkey
Copy link
Member Author

That covers when invalid data is provided, but not the positive case.

Tests added for positive case. @TimothyBJacobs

@TimothyBJacobs TimothyBJacobs self-requested a review May 25, 2020 20:49
@spacedmonkey spacedmonkey merged commit 6b8e980 into WordPress:master May 26, 2020
@spacedmonkey spacedmonkey deleted the feautre/block-api branch May 26, 2020 13:39
@github-actions github-actions bot added this to the Gutenberg 8.3 milestone May 26, 2020
'additionalProperties' => array(
'type' => 'object',
),
'default' => array(),
Copy link
Member

Choose a reason for hiding this comment

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

I notice that, despite the fact the type is assigned as 'object', when the default value is returned by the endpoint, it is returned as a JSON array [] instead of {}. Having dealt with this in the past, I considered if casting it (object) array() would be enough, but it appears to make no difference.

Do you know if there's some way to force the default to return as an object?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah this is an aggravating issue and there isn't a great way to do it. What can be done is check if attributes is empty in prepare_item_for_response and set an empty object instead, but I really don't like that because it means that the attributes property will sometimes be an array and sometimes be an object.

It is something I'd like to fix at some point, I'll try taking a look at this for 5.5.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
REST API Interaction Related to REST API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Block API: Server-side awareness of block types
5 participants