Blocks: Add enum validation to browser block parser #14810
Conversation
aduth
added
[Type] Enhancement
aduth
requested review from
ellatrix,
gziolo,
noisysocks and
youknowriad
as code owners
| @@ -242,7 +272,7 @@ export function parseWithAttributeSchema( innerHTML, attributeSchema ) { | |||
| * @return {*} Attribute value. | |||
| */ | |||
| export function getBlockAttribute( attributeKey, attributeSchema, innerHTML, commentAttributes ) { | |||
| const { type } = attributeSchema; | |||
| const { type, enum: enumSet } = attributeSchema; | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
Nice improvement. I like the implementation covered with all additional tests. It made it easier to review. I left a comment about the lack of feedback for developers when trying to build blocks and they manage to provide some corrupted data in the block instance.
| // Reject the value if it is not valid of type. Reverting to the | ||
| // undefined value ensures the default is restored, if applicable. | ||
| if ( ! isValidByType( value, type ) || ! isValidByEnum( value, enumSet ) ) { | ||
| // Reject the value if it is not valid. Reverting to the undefined |
There was a problem hiding this comment.
Should it warn in development mode? I guess the answer should be yes. What's the reason we silently reject it? I don't see it as a blocker but I would like to see a roadmap for making extensibility easier by providing good feedback for developers.
There was a problem hiding this comment.
Should it warn in development mode? I guess the answer should be yes. What's the reason we silently reject it? I don't see it as a blocker but I would like to see a roadmap for making extensibility easier by providing good feedback for developers.
I agree on all accounts. I'm inclined to merge this as-is with consideration that it follows the same precedent established by type validation. I agree we should probably include some feedback here, and will plan to create a follow-up issue.
There was a problem hiding this comment.
In fact, I think it's quite similar to the issue at #7653 , which had been closed (as it impacted the server implementation, which we're mirroring now in the client).
Which is worth highlighting: Server-side we silently drop the attributes as well:
Whatever we choose to do, I'd suggest it be made consistent across the two runtimes.
Do you think it'd make sense to reopen #7653 for this? Or create a separate issue?
There was a problem hiding this comment.
We rather open a ticket in Trac and new issue on GitHub as those are related things but still different. I think that validation and error handling in Gutenberg are rather randomly added and it should be something that should become the focus. Ideally, there is RFC created with a project-wide proposal rather than trying to fix it case by case.
There is this related work needed to implement errors catching in code registered by plugins:
withSelectwithDispatchwithFiltersPluginArea
There was a problem hiding this comment.
I'm new to WordPress and was debugging someone else's custom block that was failing validation on reloading the editor. The silent failure here made it very difficult to understand why the block would fail validation upon reloading the editor and I ended up having to use a debugger and trace/read the Gutenberg source back to this section of the code. Was a ticket ever created to track better logging/feedback for errors? I haven't been able to find it.
Longer explanation on why I feel this is needed:
I had initially missed this bit of explanation in the Block documentation:
Lastly, make sure that you respect the data’s type when setting attributes, as the framework does not automatically perform type casting of meta. Incorrect typing in block attributes will result in a post remaining dirty even after saving
Combined with the silent validation failures made me believe that the information was indeed being saved as expected. I validated this by confirming that the expected data was saved in wp_posts. Since I was in development mode and saw no other errors/warnings, there was no good place for a dev to being investigation. Any amount of feedback while developing would have drastically cut my investigation time.
Also, with regards to the Block documentation, I would argue that if no validation feedback will be added, then the above explanation should be moved out of Considerations and more prominently explained at the top of the Attributes section as it's a requirement to making custom blocks work.
* Blocks: Add enum validation to browser block parser * Block API: Remove array type in isValidByEnum JSDoc * Block API: Fix typo in test description expectation
Related: #14689, #10338
This pull request seeks to implement missing JSON schema
enumvalidation for block attributes parsing. A value will be rejected if it is not one of theenumset, if anenumis provided with the block attributes schema. This results in either the value being reset toundefined, or thedefaultprovided for the attribute if specified. This is identical behavior to bothtypeparsing validation implemented in #10338, and existingenumserver-side parsing validation (reference).Testing instructions:
Verify unit tests pass:
Verify by explicitly assigning a
directioncomment attribute in a paragraph block (optionally via UI) to a value other thanltrorrtl, the value will be removed during parse stage. The paragraph block includes anenum-specified value, despite not having been previous enforced (see previous discussion).cc @igmoweb