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 get_item_schema function to WP_REST_Widget_Areas_Controller #15981

Conversation

@jorgefilipecosta
Copy link
Member

commented Jun 4, 2019

Description

Fixes: #15902

This PR implements the get_item_schema function for WP_REST_Widget_Areas_Controller class.

This fixes a problem where warnings were being displayed during wp-cli usage.
Props to @n9yty, @TimothyBJacobs, and @lkraav for providing valuable information that allowed this fix.

How has this been tested?

I executed wp CLI shell again a WordPress instance running Gutenberg master. Using wp shell --path=/MY/WP/PATH
I verified no error were thrown on master I got:
Warning: Invalid argument supplied for foreach() in /Users/pc/dev/core/wordpres-develop/build/wp-includes/rest-api/endpoints/class-wp-rest-controller.php on line 287
Warning: Invalid argument supplied for foreach() in /Users/pc/dev/core/wordpres-develop/build/wp-includes/rest-api/endpoints/class-wp-rest-controller.php on line 287

Copy link
Member

left a comment

Since the endpoint now has a schema, the args should ideally be provided by ::get_endpoint_args_for_item_schema( WP_REST_Server::EDITABLE ) when registering the update_item route.

lib/class-wp-rest-widget-areas-controller.php Outdated Show resolved Hide resolved
lib/class-wp-rest-widget-areas-controller.php Outdated Show resolved Hide resolved
@jorgefilipecosta jorgefilipecosta force-pushed the add/get-item-schema-function-to-WP_REST_Widget_Areas_Controller branch from 6bcaa78 to 3145ffb Jun 4, 2019
@jorgefilipecosta jorgefilipecosta added this to Non UI Issues/Tasks in Widgets Jun 4, 2019
@aduth

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

@TimothyBJacobs Would you be able to give a fresh review here after the latest changes?

@TimothyBJacobs

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

The schema looks good to me, but the register_rest_route call should use that schema for the $args option when registering the edit route. This can be done by calling $this→get_endpoint_args_for_item_schema( WP_REST_Server::EDITABLE ).

@jorgefilipecosta jorgefilipecosta force-pushed the add/get-item-schema-function-to-WP_REST_Widget_Areas_Controller branch 3 times, most recently from 09b67ed to 79467ce Jul 2, 2019
@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

commented Jul 3, 2019

Hi @TimothyBJacobs thank you for the feedback your suggestion was applied 👍

@TimothyBJacobs

This comment has been minimized.

Copy link
Member

commented Jul 7, 2019

@jorgefilipecosta I think there is an issue with the update_item method call. The code only accounts for when content is a string, but it should also check whether it is an object with the raw property.

$sidebar_content = $request->get_param( 'content' );

An example from the posts controller.

if ( is_string( $request['content'] ) ) {
	$prepared_post->post_content = $request['content'];
} elseif ( isset( $request['content']['raw'] ) ) {
	$prepared_post->post_content = $request['content']['raw'];
}
@TimothyBJacobs

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

@jorgefilipecosta Do you want me to push the changes up for this?

@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

commented Aug 8, 2019

Hi @TimothyBJacobs sorry for the delay I had some AFK days, I will soon update this PR.

@jorgefilipecosta jorgefilipecosta force-pushed the add/get-item-schema-function-to-WP_REST_Widget_Areas_Controller branch from 79467ce to 46b93b7 Aug 20, 2019
@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

commented Aug 20, 2019

Hi @TimothyBJacobs, thank you for the review. The issue identified was fixed.
We now handle the raw property for content.
It is possible to test this using the following code:

wp.apiFetch({
    path: '/__experimental/widget-areas/sidebar-1',
    data: { content: { raw: '<!-- wp:columns --><div class="wp-block-columns has-2-columns"><!-- wp:column --><div class="wp-block-column"><!-- wp:paragraph --><p>1</p><!-- /wp:paragraph --></div><!-- /wp:column --><!-- wp:column --><div class="wp-block-column"><!-- wp:paragraph --><p>2</p><!-- /wp:paragraph --></div><!-- /wp:column --></div><!-- /wp:columns -->' } },
    method: 'POST',
}).then(console.log);
@jorgefilipecosta jorgefilipecosta dismissed TimothyBJacobs’s stale review Aug 20, 2019

Suggestions were applied.

@TimothyBJacobs TimothyBJacobs self-requested a review Aug 25, 2019
Copy link
Member

left a comment

Looks good to me!

@jorgefilipecosta jorgefilipecosta merged commit 2e45a8b into master Aug 26, 2019
4 checks passed
4 checks passed
Filter opened
Details
Filter opened
Details
Milestone It
Details
Travis CI - Pull Request Build Passed
Details
@jorgefilipecosta jorgefilipecosta deleted the add/get-item-schema-function-to-WP_REST_Widget_Areas_Controller branch Aug 26, 2019
donmhico added a commit to donmhico/gutenberg that referenced this pull request Aug 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Widgets
Non UI Issues/Tasks
3 participants
You can’t perform that action at this time.