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

Validation issues with block.json in a few core blocks #35902

Closed
8 tasks done
mkaz opened this issue Oct 24, 2021 · 22 comments
Closed
8 tasks done

Validation issues with block.json in a few core blocks #35902

mkaz opened this issue Oct 24, 2021 · 22 comments
Labels
Good First Issue An issue that's suitable for someone looking to contribute for the first time [Package] Block library /packages/block-library [Type] Code Quality Issues or PRs that relate to code quality

Comments

@mkaz
Copy link
Member

mkaz commented Oct 24, 2021

What problem does this address?

When adding the schema definition to core blocks in #35900 it highlighted the following validation errors in a few core blocks.

I'm not sure if the issues are with the schema definition or invalid properties in the block.json each will need to be investigated, adding this ticket so they are not lost.

  • Button block - spacing - padding - horizontal - invalid value
  • Column block: templateLock - type missing
  • Cover block: templateLock - type missing
  • File block: viewScript - invalid property
  • Group block: templateLock - type missing
  • Navigation block: viewScript - invalid property
  • Post-Comment-content block - spacing - padding - horizontal - invalid value
  • Post-comments-form block - style - invalid type
@mkaz mkaz added [Type] Code Quality Issues or PRs that relate to code quality Good First Issue An issue that's suitable for someone looking to contribute for the first time labels Oct 24, 2021
@gziolo
Copy link
Member

gziolo commented Oct 25, 2021

It looks like changes proposed in #35900 helped to find valid issues. I can confirm that templateLock is missing a type. @oandregal, can you help validate the reported violation for the Global Styles related support?

viewScript is a new property waiting to be included in WordPress 5.9 and it still needs to be documented. The work on it is being tracked in #33542. We need to include it in the block schema. It is documented in the REST API endpoint for the block type:
https://github.com/WordPress/wordpress-develop/blob/2238c302f9598b6f8f4ab0c27b175fd9ca7370fd/src/wp-includes/rest-api/endpoints/class-wp-rest-block-types-controller.php#L529-L535
also in the WP_Block_Type class:
https://github.com/WordPress/wordpress-develop/blob/2238c302f9598b6f8f4ab0c27b175fd9ca7370fd/src/wp-includes/class-wp-block-type.php#L174-L180

Let me also highlight again my question from #35843 (comment):

Out of curiosity, how much work it would be to keep the schema in Gutenberg repository, possibly inside @wordpress/blocks package (or we could create another one with schemas to include also theme.json) and sync it somehow to another place that we could use instead?

I love the idea of having a schema that could be used for validation and I'd be in favor of tighter integration into the existing tooling. One immediate idea for a follow-up for this PR would be to run block.json validation on scaffolded test block on CI as part of:

https://github.com/WordPress/gutenberg/blob/trunk/bin/test-create-block.sh

Now, that we plan to add schema to all core blocks, we could also create an integration test that would validate all block.json files against the schema.

@gziolo gziolo added the [Package] Block library /packages/block-library label Oct 25, 2021
@gziolo
Copy link
Member

gziolo commented Oct 25, 2021

@ryanwelcher, this could be also useful to add the block.json schema to the blocks present in https://github.com/WordPress/gutenberg-examples.

@ryanwelcher
Copy link
Contributor

@ryanwelcher, this could be also useful to add the block.json schema to the blocks present in https://github.com/WordPress/gutenberg-examples

The schema property was added with WordPress/gutenberg-examples#167 - thanks @mkaz !

@mkaz
Copy link
Member Author

mkaz commented Oct 25, 2021

@gziolo I like the idea of having it internal to Gutenberg repo.
What do you recommend a "@wordpress/schemas" package?

We would also want to include the theme.json schema. We can mirror any changes to the SchemaStore URL so any old URLs would still work, plus get better exposure if people look there first,. But the master version would live in Gutenberg repo.

Also, in Gutenberg it would get automatic versioning with the tags/branches of each release,
Using a URL like one could validate against whatever version they were targeting:
https://github.com/WordPress/gutenberg/tree/wp/5.8/packages/schemas/block.json

@gziolo
Copy link
Member

gziolo commented Oct 25, 2021

@gziolo I like the idea of having it internal to Gutenberg repo. What do you recommend a "@wordpress/schemas" package?

That's one of the options that could help with maintenance. There are also i18n related config files for block.json and theme.json that we could move there:
https://github.com/WordPress/gutenberg/blob/trunk/packages/blocks/src/api/i18n-block.json
https://github.com/WordPress/gutenberg/blob/trunk/lib/theme-i18n.json

Unless @mkaz or @ajlende know how to annotate the translatable fields directly in the schema definition 😄

@ajlende
Copy link
Contributor

ajlende commented Oct 25, 2021

We could add custom fields to the schema file to indicate if the field is translatable or not. Some of the schemas in SchemaStore use a custom markdownDescription field (used by VS Code) that isn't part of the schema spec, so I'd expect that most validators would just ignore invalid values rather than halting.

@oandregal
Copy link
Member

oandregal commented Oct 25, 2021

Button block - spacing - padding - horizontal - invalid value

Checked this one, which was added at #33859 and it appears that horizontal & vertical are proper values. Checked the PR to schemastore and those values are also there. Unfamiliar with the schemastore syntax but probably we'd need to update the schema to allow the existing values (see comment).

@oandregal
Copy link
Member

Post-Comment-content block - spacing - padding - horizontal - invalid value

Same as button above.

@ajlende
Copy link
Contributor

ajlende commented Oct 25, 2021

  • Button block - spacing - padding - horizontal - invalid value
  • Post-Comment-content block - spacing - padding - horizontal - invalid value

Fix in SchemaStore/schemastore#1903

@ryanwelcher
Copy link
Contributor

I've also got a PR to address some items being moved into typopgraphy - SchemaStore/schemastore#1898

@gziolo
Copy link
Member

gziolo commented Nov 2, 2021

Post-comments-form block - style - invalid type

We need to extend the schema to allow string and string[] for style and editorStyle. @aristath is it something we will land in WordPress 5.9 or is it the plugin for the time being?

@gziolo
Copy link
Member

gziolo commented Nov 2, 2021

  • Column block: templateLock - type missing
  • Cover block: templateLock - type missing
  • Group block: templateLock - type missing

Fix in #36140

@aristath
Copy link
Member

aristath commented Nov 2, 2021

We need to extend the schema to allow string and string[] for style and editorStyle. @aristath is it something we will land in WordPress 5.9 or is it the plugin for the time being?

AFAIK this will land in 5.9 👍

@gziolo
Copy link
Member

gziolo commented Nov 3, 2021

I opened #36175 to add viewScript in schema and docs to the Gutenberg repo.

@gziolo
Copy link
Member

gziolo commented Nov 4, 2021

I opened #36175 to add viewScript in schema and docs to the Gutenberg repo.

I opened a follow-up PR SchemaStore/schemastore#1915.

I will take care of the last item now:

Post-comments-form block - style - invalid type.

@gziolo
Copy link
Member

gziolo commented Nov 4, 2021

I will take care of the last item now:

Post-comments-form block - style - invalid type.

PR ready to review: #36218. That is the last task which should conclude this issue 🎉

walbo pushed a commit that referenced this issue Nov 5, 2021
The `templateLock` attribute is mixed between boolean and string. Setting the type as string introduces an regression that will remove `false` on save.

The original PR (#36140) tried to fix a validation issue reported in #35902

The schema error is incorrect. The attribute definition is required to have either a `type` or an `enum`.
@walbo
Copy link
Member

walbo commented Nov 5, 2021

  • Column block: templateLock - type missing
  • Cover block: templateLock - type missing
  • Group block: templateLock - type missing

Fix in #36140

Created an PR (#36264) to fix a regression in that PR.

walbo pushed a commit that referenced this issue Nov 5, 2021
Part of #35902.

Update the block.json schema to require either a `type` or an `enum` prop.
fabiankaegy pushed a commit that referenced this issue Nov 5, 2021
Part of #35902.

Update the block.json schema to require either a `type` or an `enum` prop.
@gziolo
Copy link
Member

gziolo commented Nov 8, 2021

Great catch @walbo. I see that you already relaxed the schema validation in #36267. It looks like we need to allow also an array of strings as the type in block attributes. Anyone knows how to code it? 😃

@walbo
Copy link
Member

walbo commented Nov 8, 2021

Yes. I'll create a PR so type can both be a string or an array.

@gziolo
Copy link
Member

gziolo commented Nov 8, 2021

We still have some changes to sync with https://github.com/SchemaStore/schemastore/blob/master/src/schemas/json/block.json to be able to consider this issue as done.

andrewserong pushed a commit that referenced this issue Nov 10, 2021
* Revert "Block Library: Fix incorrect attributes definitions #36140"

The `templateLock` attribute is mixed between boolean and string. Setting the type as string introduces an regression that will remove `false` on save.

The original PR (#36140) tried to fix a validation issue reported in #35902

The schema error is incorrect. The attribute definition is required to have either a `type` or an `enum`.

* Spesify type as string or boolean

* Add unit test
noisysocks pushed a commit that referenced this issue Nov 10, 2021
* Revert "Block Library: Fix incorrect attributes definitions #36140"

The `templateLock` attribute is mixed between boolean and string. Setting the type as string introduces an regression that will remove `false` on save.

The original PR (#36140) tried to fix a validation issue reported in #35902

The schema error is incorrect. The attribute definition is required to have either a `type` or an `enum`.

* Spesify type as string or boolean

* Add unit test
andrewserong pushed a commit that referenced this issue Nov 10, 2021
* Revert "Block Library: Fix incorrect attributes definitions #36140"

The `templateLock` attribute is mixed between boolean and string. Setting the type as string introduces an regression that will remove `false` on save.

The original PR (#36140) tried to fix a validation issue reported in #35902

The schema error is incorrect. The attribute definition is required to have either a `type` or an `enum`.

* Spesify type as string or boolean

* Add unit test
@ajlende
Copy link
Contributor

ajlende commented Nov 14, 2021

With SchemaStore/schemastore#1930 merged, the SchemaStore file should reference this one. So we don't have to worry about opening a PR for both of them anymore, and I think this can be marked as complete 🙂

@ajlende ajlende closed this as completed Nov 14, 2021
@gziolo
Copy link
Member

gziolo commented Nov 15, 2021

@ajlende, thank you so much 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good First Issue An issue that's suitable for someone looking to contribute for the first time [Package] Block library /packages/block-library [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

No branches or pull requests

7 participants