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

Block editor settings endpoint - Remove experimental namespace #33128

Merged
merged 2 commits into from
Jul 2, 2021

Conversation

geriux
Copy link
Member

@geriux geriux commented Jul 1, 2021

Description

We've been testing this endpoint and it's working as expected. The only issue now is keeping the __experimental namespace, since we are implementing this in the mobile apps we'd like to have the final namespace.

How has this been tested?

  • Use the local instance of WordPress
  • Using POSTMAN with an application password
  • GET the following /wp-json/wp-block-editor/v1/settings
  • Expect to get a response

Screenshots

N/A

Types of changes

Update namespace

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • 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 (please manually search all *.native.js files for terms that need renaming or removal).

@@ -16,7 +16,7 @@ class WP_REST_Block_Editor_Settings_Controller extends WP_REST_Controller {
* Constructs the controller.
*/
public function __construct() {
$this->namespace = '__experimental/wp-block-editor/v1';
$this->namespace = 'wp-block-editor/v1';
Copy link
Member

Choose a reason for hiding this comment

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

I've checked some stable endpoints in the gutenberg and wordpress-develop codebase and this is what I see: wp/v2/pattern-directory, wp/v2/widget-types, wp/v2/widgets, wp/v2/block-directory, wp/v2/block-types, wp/v2/comments.

Following the convention of existing endpoints, I wonder if this should be wp/v2/block-editor-settings instead, wp/v2 as namespace and block-editor-settings as rest_base.

I'd welcome thoughts from people more experienced with the REST API naming schema, cc @TimothyBJacobs

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the suggestions @nosolosw!

From this comment it was suggested to use this naming instead of following the wp/v2/ convention, although it'd make things easier if it was using the same structure as the existing ones 😅

Copy link
Member

Choose a reason for hiding this comment

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

Yeah the intention to use the different namespace is that wp/v2 is for modeling data, whereas this is purely a block editor API returning configuration values.

There really is no plan to introduce a wp/v3 for numerous reasons, but putting this in a block editor namespace would allow for more freedom in introducing newer versions for breaking changes.

This would match what we did for Site Health in having a wp-site-health/v1 namespace. I think the only inconsistency here would be that ideally the block-directory and pattern-directory endpoints would've been part of this namespace as well.

Copy link
Member

Choose a reason for hiding this comment

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

I see, thanks for the context. Let's do this then.

As a data point for the future, I'd like to provide the thought that it's difficult to reason about why/when something is data or a setting from the perspective of a consumer of the API. By looking at the reference documentation, I see endpoints that are arguably config: post types, block types, site settings to name a few.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good! Let's keep the wp-site-health/v1 namespace 👍

Can we then proceed with removing the __experimental part? Thanks!

@oandregal oandregal assigned oandregal and geriux and unassigned oandregal Jul 1, 2021
@oandregal oandregal added Core REST API Task Task for Core REST API efforts REST API Interaction Related to REST API and removed Core REST API Task Task for Core REST API efforts labels Jul 1, 2021
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.

3 participants