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

REST API: Improve the block type schema for the name field #5278

Closed

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Sep 22, 2023

Trac ticket: https://core.trac.wordpress.org/ticket/59346

The goal of this PR is to align the schema between block.json and the REST API endpoint for block types. It looks like the name field isn't validated in all places and if it uses pattern matching in the REST API code, then it's slightly different.

https://github.com/WordPress/gutenberg/blob/aa938166f28bcfff6c7b8d1caac5a95f78852f97/schemas/json/block.json#L26

https://github.com/WordPress/gutenberg/blob/f6d16f0b9f0a71d51c7178d35902c078b0d33c08/packages/blocks/src/api/registration.js#L231

I opted for using the same pattern as during block registration in JavaScript:

'^[a-z][a-z0-9-]*/[a-z][a-z0-9-]*$'

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

@gziolo gziolo self-assigned this Sep 22, 2023
@spacedmonkey
Copy link
Member

@gziolo
Copy link
Member Author

gziolo commented Sep 22, 2023

@gziolo Don't we need to regenerate this line -https://github.com/WordPress/wordpress-develop/blob/trunk/tests/qunit/fixtures/wp-api-generated.js

Do you mean we should also update the pattern used with URLs, like:

"/wp/v2/block-types/(?P<namespace>[a-zA-Z0-9_-]+)": {

or

"/wp/v2/block-types/(?P<namespace>[a-zA-Z0-9_-]+)/(?P<name>[a-zA-Z0-9_-]+)": {

I executed all unit tests and this file hasn't changed. The comment included explains that the file is auto-generated with test_build_wp_api_client_fixtures.

@ockham
Copy link
Contributor

ockham commented Sep 25, 2023

Thank you! I'll have a closer look in a bit, but one thing I was wondering -- could we express the fact that the keys in blockHooks are block names (as defined earlier in the schema) even more semantically (rather than using the same RegEx as a constant)? I've seen there's something called $ref in the JSON schema spec -- think that could work here? 🤔

@gziolo
Copy link
Member Author

gziolo commented Sep 25, 2023

I've seen there's something called $ref in the JSON schema spec -- think that could work here? 🤔

There is a code comment included in the same method which suggests it can't be used:

// rest_validate_value_from_schema doesn't understand $refs, pull out reused definitions for readability.

Copy link
Member

@spacedmonkey spacedmonkey left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@gziolo
Copy link
Member Author

gziolo commented Sep 25, 2023

Committed with 56676.

@gziolo gziolo closed this Sep 25, 2023
@gziolo gziolo deleted the update/rest-api-block-type-name-pattern branch September 25, 2023 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants