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.

Sign up for GitHub

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

Blocks: Add enum validation to browser block parser #14810

Merged
merged 3 commits into from
Apr 10, 2019
+128 −4
Merged

Blocks: Add enum validation to browser block parser #14810

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
72cd97d
Blocks: Add enum validation to browser block parser
aduth Apr 3, 2019
c1730cc
Block API: Remove array type in isValidByEnum JSDoc
aduth Apr 3, 2019
72441d3
Block API: Fix typo in test description expectation
aduth Apr 3, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 34 additions & 4 deletions packages/blocks/src/api/parser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,36 @@ export function isOfTypes( value, types ) {
return types.some( ( type ) => isOfType( value, type ) );
}

/**
* Returns true if value is valid per the given block attribute schema type
* definition, or false otherwise.
*
* @link https://json-schema.org/latest/json-schema-validation.html#rfc.section.6.1.1
*
* @param {*} value Value to test.
* @param {?(Array<string>|string)} type Block attribute schema type.
*
* @return {boolean} Whether value is valid.
*/
export function isValidByType( value, type ) {
return type === undefined || isOfTypes( value, castArray( type ) );
}

/**
* Returns true if value is valid per the given block attribute schema enum
* definition, or false otherwise.
*
* @link https://json-schema.org/latest/json-schema-validation.html#rfc.section.6.1.2
*
* @param {*} value Value to test.
* @param {?Array} enumSet Block attribute schema enum.
*
* @return {boolean} Whether value is valid.
*/
export function isValidByEnum( value, enumSet ) {
return ! Array.isArray( enumSet ) || enumSet.includes( value );
}

/**
* Returns true if the given attribute schema describes a value which may be
* an ambiguous string.
Expand Down Expand Up @@ -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;
Copy link
Member Author

Choose a reason for hiding this comment

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

In case you're wondering:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Lexical_grammar#Future_reserved_keywords

let value;

switch ( attributeSchema.source ) {
Expand All @@ -262,9 +292,9 @@ export function getBlockAttribute( attributeKey, attributeSchema, innerHTML, com
break;
}

if ( type !== undefined && ! isOfTypes( value, castArray( type ) ) ) {
// 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
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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:

https://github.com/WordPress/WordPress/blob/2887cea50f2a717782a6adb5922cb272d3c4bc10/wp-includes/class-wp-block-type.php#L153-L156

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?

Copy link
Member

Choose a reason for hiding this comment

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

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:

  • withSelect
  • withDispatch
  • withFilters
  • PluginArea

Copy link

Choose a reason for hiding this comment

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

Hi @gziolo , @aduth ,

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.

// value ensures the default is respected, if applicable.
value = undefined;
}

Expand Down
94 changes: 94 additions & 0 deletions packages/blocks/src/api/test/parser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ import {
toBooleanAttributeMatcher,
isOfType,
isOfTypes,
isValidByType,
isValidByEnum,
} from '../parser';
import {
registerBlockType,
Expand Down Expand Up @@ -180,6 +182,42 @@ describe( 'block parser', () => {
} );
} );

describe( 'isValidByType', () => {
it( 'returns true if type undefined', () => {
expect( isValidByType( null ) ).toBe( true );
} );

it( 'returns false if value is not one of types array', () => {
expect( isValidByType( null, [ 'string' ] ) ).toBe( false );
} );

it( 'returns false if value is one of types array', () => {
expect( isValidByType( null, [ 'string', 'null' ] ) ).toBe( true );
} );

it( 'returns false if value is not of type string', () => {
expect( isValidByType( null, 'string' ) ).toBe( false );
} );

it( 'returns true if value is type string', () => {
expect( isValidByType( null, 'null' ) ).toBe( true );
} );
} );

describe( 'isValidByEnum', () => {
it( 'returns true if enum set undefined', () => {
expect( isValidByEnum( 2 ) ).toBe( true );
} );

it( 'returns false if value is not of enum set', () => {
expect( isValidByEnum( 2, [ 1, 3 ] ) ).toBe( false );
} );

it( 'returns true if value is of enum set', () => {
expect( isValidByEnum( 2, [ 1, 2, 3 ] ) ).toBe( true );
} );
} );

describe( 'parseWithAttributeSchema', () => {
it( 'should return the matcher’s attribute value', () => {
const value = parseWithAttributeSchema(
Expand Down Expand Up @@ -260,6 +298,62 @@ describe( 'block parser', () => {
expect( value ).toBe( 10 );
} );

it( 'should reject type-invalid value, with default', () => {
const value = getBlockAttribute(
'number',
{
type: 'string',
default: 5,
},
'',
{ number: 10 }
);

expect( value ).toBe( 5 );
} );

it( 'should reject type-invalid value, without default', () => {
const value = getBlockAttribute(
'number',
{
type: 'string',
},
'',
{ number: 10 }
);

expect( value ).toBe( undefined );
} );

it( 'should reject enum-invalid value, with default', () => {
const value = getBlockAttribute(
'number',
{
type: 'number',
enum: [ 4, 5, 6 ],
default: 5,
},
'',
{ number: 10 }
);

expect( value ).toBe( 5 );
} );

it( 'should reject enum-invalid value, without default', () => {
const value = getBlockAttribute(
'number',
{
type: 'number',
enum: [ 4, 5, 6 ],
},
'',
{ number: 10 }
);

expect( value ).toBe( undefined );
} );

it( "should return the matcher's attribute value", () => {
const value = getBlockAttribute(
'content',
Expand Down