Skip to content

Add Server Side Render component. #5602

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.

Sign up for GitHub

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

Merged
pento merged 32 commits into from
Apr 26, 2018
Merged

Add Server Side Render component. #5602

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
cd58776
Add blocks-renderer API endpoint. Add skeleton for ServerSideRender c…
miina Mar 12, 2018
b0ede76
Update SSR preview when editing attributes.
miina Mar 13, 2018
340136e
Fix binding preview change.
miina Mar 13, 2018
514dd4f
Fix binding preview change.
miina Mar 13, 2018
a90f40d
Add tests. Fix permission check for get_item.
miina Mar 13, 2018
0c127e1
Remove debugger line.
miina Mar 14, 2018
b0b977a
Fix some phpcs.
miina Mar 14, 2018
395a168
Fix some jscs.
miina Mar 14, 2018
1d05c2d
Show 'loading' when changing response.
miina Mar 14, 2018
c40dd25
Use wp.apiRequest for consistency.
miina Mar 14, 2018
f323651
Update ServerSideRender readme to note intended usage
westonruter Mar 14, 2018
c4abc69
Rename classes, methods, and property to be more aligned with the res…
westonruter Mar 14, 2018
6557877
Register block-renderer endpoint for each dynamic block
westonruter Mar 14, 2018
228c756
Fixes to README. Use isEqual. Renaming method.
miina Mar 16, 2018
b48ef81
Fix checking for existing response.
miina Mar 16, 2018
e39332d
Add putting together query URL from object.
miina Mar 16, 2018
46506a9
Fix some jscs.
miina Mar 16, 2018
ce1f7c6
Fix using correct props with fetching.
miina Mar 16, 2018
e0fec83
Improve putting together query URL. Use attributes as separate param.
miina Mar 17, 2018
17e404a
Make path constant more readable.
miina Mar 17, 2018
39b867a
Add setting global if is present.
miina Mar 29, 2018
4c3129b
Merge branch 'master' of github.com:WordPress/gutenberg into add/780-…
miina Apr 6, 2018
3c60d4b
Update Readme.
miina Apr 6, 2018
a10bfac
Register post_id. Add context. Fix tests.
miina Apr 16, 2018
a166324
Fix CS.
miina Apr 16, 2018
f0f4a77
Merge remote-tracking branch 'origin/master' into add/780-server-side…
miina Apr 16, 2018
6d1ee65
Add more tests. Add exception for namespace to as ruleset.
miina Apr 25, 2018
2bbbeb7
Fix eslint.
miina Apr 25, 2018
6f7d62e
Merge branch 'master' of github.com:WordPress/gutenberg into add/780-…
miina Apr 25, 2018
67af6b7
Add context param to API request.
miina Apr 25, 2018
007fb1d
Add version numbers to the phpdocs
pento Apr 26, 2018
1c9ef56
Tweak the README a little
pento Apr 26, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Register block-renderer endpoint for each dynamic block 
* Supply block attributes schema as endpoint schema.
* Introduce attributes endpoint property and let REST API schema validate and sanitize them.
* Ensure that attribute values are sanitized in addition to validated.
  • Loading branch information
@westonruter
westonruter committed Mar 14, 2018
commit 655787740e2d21e57142642f3b0cece83c3691d7
4 changes: 2 additions & 2 deletions lib/class-wp-block-type-registry.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ final class WP_Block_Type_Registry {
*
* @since 0.6.0
* @access private
* @var array
* @var WP_Block_Type[]
*/
private $registered_block_types = array();

Expand Down Expand Up @@ -142,7 +142,7 @@ public function get_registered( $name ) {
* @since 0.6.0
* @access public
*
* @return array Associative array of `$block_type_name => $block_type` pairs.
* @return WP_Block_Type[] Associative array of `$block_type_name => $block_type` pairs.
*/
public function get_all_registered() {
return $this->registered_block_types;
Expand Down
2 changes: 1 addition & 1 deletion lib/class-wp-block-type.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ public function prepare_attributes_for_render( $attributes ) {
if ( isset( $attributes[ $attribute_name ] ) ) {
$is_valid = rest_validate_value_from_schema( $attributes[ $attribute_name ], $schema );
if ( ! is_wp_error( $is_valid ) ) {
$value = $attributes[ $attribute_name ];
$value = rest_sanitize_value_from_schema( $attributes[ $attribute_name ], $schema );
}
}

Expand Down
88 changes: 37 additions & 51 deletions lib/class-wp-rest-block-renderer-controller.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,30 +28,44 @@ public function __construct() {
}

/**
* Registers the necessary REST API routes.
* Registers the necessary REST API routes, one for each dynamic block.
*
* @access public
*/
public function register_routes() {

// @codingStandardsIgnoreLine - PHPCS mistakes $this->namespace for the namespace keyword.
register_rest_route( $this->namespace, '/' . $this->rest_base . '/(?P<name>[\w-]+\/[\w-]+)', array(
'args' => array(
'name' => array(
'description' => __( 'Unique registered name for the block.', 'gutenberg' ),
'type' => 'string',
$block_types = WP_Block_Type_Registry::get_instance()->get_all_registered();
foreach ( $block_types as $block_type ) {
if ( ! $block_type->is_dynamic() ) {
continue;
}

// @codingStandardsIgnoreLine - PHPCS mistakes $this->namespace for the namespace keyword.
Copy link
Member

@danielbachhuber danielbachhuber Apr 24, 2018

Choose a reason for hiding this comment

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

Ditto above.

Copy link
Contributor Author

@miina miina Apr 25, 2018

Choose a reason for hiding this comment

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

Added an exception for this file within 6d1ee65.

register_rest_route( $this->namespace, '/' . $this->rest_base . '/(?P<name>' . $block_type->name . ')', array(
'args' => array(
'name' => array(
'description' => __( 'Unique registered name for the block.', 'gutenberg' ),
'type' => 'string',
),
),
),
array(
'methods' => WP_REST_Server::READABLE,
'callback' => array( $this, 'get_item' ),
'permission_callback' => array( $this, 'get_item_permissions_check' ),
'args' => array(
'context' => $this->get_context_param( array( 'default' => 'view' ) ),
array(
'methods' => WP_REST_Server::READABLE,
'callback' => array( $this, 'get_item' ),
'permission_callback' => array( $this, 'get_item_permissions_check' ),
'args' => array(
'context' => $this->get_context_param( array( 'default' => 'view' ) ),
'attributes' => array(
/* translators: %s is the name of the block */
'description' => sprintf( __( 'Attributes for %s block', 'gutenberg' ), $block_type->name ),
'type' => 'object',
'additionalProperties' => false,
'properties' => $block_type->attributes,
),
),
),
),
'schema' => array( $this, 'get_public_item_schema' ),
) );
'schema' => array( $this, 'get_public_item_schema' ),
) );
}
}

/**
Expand All @@ -60,9 +74,10 @@ public function register_routes() {
* @since ?
* @access public
*
* @param WP_REST_Request $request Request.
* @return true|WP_Error True if the request has read access, WP_Error object otherwise.
*/
public function get_item_permissions_check() {
public function get_item_permissions_check( $request ) {
if ( ! current_user_can( 'edit_posts' ) ) {
Copy link
Member

@westonruter westonruter Mar 19, 2018

Choose a reason for hiding this comment

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

Something that comes to mind is that some blocks will want to render differently depending on their post context. In that case, there should be an edit_post check for a given post ID.

At least this is the case for shortcodes, for example a [gallery] will render the images attached to the current post if no ids attribute is present. A similar use case seems similar for dynamic blocks. This could be particularly powerful for re-usable blocks. For instance a nav menu (#5036) will render a menu item differently when that item is the one currently being viewed. When a dynamic block is used as a reusable block this would allow for its post context to be variable.

The author bio block in #3250 is another example for where for there to be a post context to render.

Should the post dependency be formalized as a new attribute source type to make the explicit and declarative that the block depends on a post context to be present?

In the case of the author bio block, this could be formalized as follows instead of relying on getEditedPostAttribute directly:

attributes: {
	author: {
		type: 'int',
		source: 'post-context',
		field: 'author'
	},
},

This would allow for the attribute's source to be matched in a server-side context as well.

Copy link
Contributor Author

@miina miina Mar 22, 2018

Choose a reason for hiding this comment

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

@westonruter To see if I understand the thoughts correctly:
Are you bringing out the issue that depending on the block the edit_posts permission might not be enough since the block might involve other posts? For example in case of nav menus, we should check if the user has the sufficient permissions for the nav menu items? And in case of shortcodes (e.g. gallery) if the user can edit the specific post (check by existing ID/ postId in the request)?

On the new source -- not sure I understand completely. Are you thinking that the post-context could be used in case we need to get the object data loaded ( e.g. in this case author user object )? Or would it just be to note that the block depends on the author?

Copy link
Member

@westonruter westonruter Mar 22, 2018

Choose a reason for hiding this comment

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

I commented on the edit_posts cap check because it was related but not the primary concern. The first concern is that a dynamic block may depend on a post for context in order to display properly. For example, the aforementioned author bio block requires a $post->post_author global context to display. In the same way, the gallery widget depends on a global $post to find attachments to display if there are no ids present. Blocks could reply on postmeta to display, but if there is no $post context then it wouldn't know from where to get the postmeta.

I see this being analogous to the parse-embed Ajax action. Notice how it allows for a post to be supplied in the request for global context.

So then the topic of the new attribute source type is to formalize the relationship between a block to having a post as context. In a server-side rendering context, this would normally be implemented by setting the $post global. In a client-side context, it could be realized as shown in the author bio block:

postAuthorId: getEditedPostAttribute( state, 'author' ),

The impact of the $post context could also be directly visible in the nav menu block (#5036) since nav menu items have classes that vary based on the $post context, as can be seen in core's _wp_menu_item_classes_by_context function.

Copy link
Contributor Author

@miina miina Mar 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Okay, now I think I understand -- the issue of not having $post set in case of blocks which are actually dependent on the $post being set in case of using SSR.

In the context of SSR -- I guess we would then need to check if there are attributes that require $post being set and then in case of ID existing set the $post and otherwise return error. Hmm, however, I guess SRR wouldn't get the information about the source of the attributes and would just need to set the $post in case of existing ID.

Copy link
Contributor Author

@miina miina Apr 6, 2018

Choose a reason for hiding this comment

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

@westonruter Added setting the global $post in case the param is set, let me know if you have any thoughts on what else might be missing from the SSR component side.

return new WP_Error( 'gutenberg_block_cannot_read', __( 'Sorry, you are not allowed to read Gutenberg blocks as this user.', 'gutenberg' ), array(
'status' => rest_authorization_required_code(),
Expand All @@ -82,43 +97,14 @@ public function get_item_permissions_check() {
* @return WP_REST_Response|WP_Error Response object on success, or WP_Error object on failure.
*/
public function get_item( $request ) {
if ( ! isset( $request['name'] ) ) {
return new WP_Error( 'rest_block_invalid_name', __( 'Invalid block name.', 'gutenberg' ), array( 'status' => 404 ) );
}

$registry = WP_Block_Type_Registry::get_instance();
$block = $registry->get_registered( $request['name'] );
Copy link
Member

@danielbachhuber danielbachhuber Apr 14, 2018

Choose a reason for hiding this comment

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

Can you return an error here if the block is invalid?


if ( ! $block || ! $block instanceof WP_Block_Type ) {
return new WP_Error( 'rest_block_invalid_name', __( 'Invalid block name.', 'gutenberg' ), array( 'status' => 404 ) );
}

$atts = $this->prepare_attributes( $request->get_params() );

$data = array(
'rendered' => $block->render( $atts ),
$data = array(
'rendered' => $block->render( $request->get_param( 'attributes' ) ),
Copy link
Contributor Author

@miina miina Mar 15, 2018

Choose a reason for hiding this comment

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

@westonruter This isn't working with the current SSR component code since the attributes are all sent as separate params and not as one attributes array:

                const apiURL = addQueryArgs( '/gutenberg/v1/block-renderer/' + block, {
			...attributes,
			_wpnonce: wpApiSettings.nonce,
		} );

Just sending attributes: attributes as param would fail the request, is there an existing method that creates an acceptable query string for the API (e.g. ?attributes[key1]=value1&attributes[key2]=value2... etc? If not then perhaps we could still pass all the attributes as separate params and modify the endpoint accordingly. Thoughts?

Copy link
Member

@westonruter westonruter Mar 15, 2018

Choose a reason for hiding this comment

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

That's a good question. I think it's important for attributes to be under an attributes query parameter because that will prevent conflicts with block attributes. Like if a block has a name or a context these will conflict with the REST API args. It seems like this should be allowed:

const apiURL = addQueryArgs( '/gutenberg/v1/block-renderer/' + block, {
	attributes,
	_wpnonce: wpApiSettings.nonce,
} );

In other words, I think addQueryArgs needs to be able to properly convert arg values that are objects.

Copy link
Contributor Author

@miina miina Mar 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Couldn't find an existing method at this moment, apparently query-string is intentionally not supporting nested attributes and suggests sending the object as a JSON string.

Added a custom method for now to the class to put together the query string supporting objects (ServerSideRender.getQueryUrlFromObject).

Copy link
Member

@danielbachhuber danielbachhuber Apr 24, 2018

Choose a reason for hiding this comment

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

We may also be passing the full block content to the render callback in #6239

Rather than needing to remember passing all supported arguments to the render() callback, it would be useful to have a helper function for this.

);
return rest_ensure_response( $data );
}

/**
* Fix potential boolean value issues. The values come as strings and "false" and "true" might generate issues if left like this.
*
* @param array $attributes Attributes.
* @return mixed Attributes.
*/
public function prepare_attributes( $attributes ) {
foreach ( $attributes as $key => $value ) {
if ( 'false' === $value ) {
$attributes[ $key ] = false;
} elseif ( 'true' === $value ) {
$attributes[ $key ] = true;
}
}

return $attributes;
}

/**
* Retrieves block's output schema, conforming to JSON Schema.
*
Expand All @@ -130,11 +116,11 @@ public function prepare_attributes( $attributes ) {
public function get_item_schema() {
return array(
'$schema' => 'http://json-schema.org/schema#',
'title' => 'block-renderer',
'title' => 'rendered-block',
'type' => 'object',
'properties' => array(
'rendered' => array(
'description' => __( 'The block\'s output.', 'gutenberg' ),
'description' => __( 'The rendered block.', 'gutenberg' ),
'type' => 'string',
'required' => true,
Copy link
Member

@danielbachhuber danielbachhuber Apr 14, 2018

Choose a reason for hiding this comment

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

context is missing from this schema attribute. Because this is only used in the editor, it should be array( 'edit' )

Copy link
Contributor Author

@miina miina Apr 16, 2018

Choose a reason for hiding this comment

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

Added within a10bfac.

),
Expand Down
Loading