Block API: Registration with "empty" defaults from rWP47875 conflict with block defaults

[aduth]

Related: https://core.trac.wordpress.org/changeset/47875
Related Slack conversation (link requires registration): https://wordpress.slack.com/archives/C02QB2JS7/p1591045633488200

The core commit r47875 introduces an incompatibility with how block default values are considered.

With this commit, many fields now assign a default "empty" value, either empty string, empty array, or null.

This can conflict with default block registration will merge default values, since these defaults (even null) will take precedent over the fallback value:

gutenberg/packages/blocks/src/api/registration.js

Lines 143 to 151 in 281464a

	settings = {
	name,
	icon: blockDefault,
	attributes: {},
	keywords: [],
	save: () => null,
	...get( serverSideBlockDefinitions, name ),
	...settings,
	};

Some validation may also conflict with the new defaults.

Example: Empty string '' default category may be flagged as invalid:

gutenberg/packages/blocks/src/api/registration.js

Lines 206 to 211 in 281464a

	if (
	'category' in settings &&
	! some( select( 'core/blocks' ).getCategories(), {
	slug: settings.category,
	} )
	) {

Example: Empty string '' default icon may be flagged as invalid:

gutenberg/packages/blocks/src/api/registration.js

Lines 230 to 237 in 281464a

	settings.icon = normalizeIconObject( settings.icon );
	if ( ! isValidIcon( settings.icon.src ) ) {
	console.error(
	'The icon passed is invalid. ' +
	'The icon should be a string, an element, a function, or an object following the specifications documented in https://developer.wordpress.org/block-editor/developers/block-api/block-registration/#icon-optional'
	);
	return;
	}

The latter of these is currently causing failed builds on the master branch:

    Expected mock function not to be called but it was called with:
    ["The icon passed is invalid. The icon should be a string, an element, a function, or an object following the specifications documented in https://developer.wordpress.org/block-editor/developers/block-api/block-registration/#icon-optional"],["The icon passed is invalid. The icon should be a string, an element, a function, or an object following the specifications documented in https://developer.wordpress.org/block-editor/developers/block-api/block-registration/#icon-optional"],["The icon passed is invalid. The icon should be a string, an element, a function, or an object following the specifications documented in https://developer.wordpress.org/block-editor/developers/block-api/block-registration/#icon-optional"],["The icon passed is invalid. The icon should be a string, an element, a function, or an object following the specifications documented in https://developer.wordpress.org/block-editor/developers/block-api/block-registration/#icon-optional"]

https://travis-ci.com/github/WordPress/gutenberg/jobs/342186917

Possible Solutions:

Short-term, the builds should be resolved by one of:

• Temporarily skip failing test cases.
• Temporarily loosen restrictions applied to block properties during registration, or reset new empty default values back to assumed block defaults for missing property.

Long-term, empty values should be reevaluated by one of:

• Some empty values should be treated more as an "unset" value, either null or some equivalent of unset server-side
  • Important Note: If the object value is null when performing the settings merge during client-side block registration, the null value will take precedence. Previously, it would rely on the fact that the value was unset (undefined).
• Client-side registration should accommodate these empty values, optionally treating as equivalent to what was previously considered as the default unset value.

[aduth]

Temporary fix at #22811

[aduth]

The temporary fix at #22811 was merged. For the fix to be reflected in pull requests, it may be necessary for branches to be rebased. Alternatively, depending if a subsequent change is made in core codebase, it should begin to take effect more immediately (once a new nightly is built).

[gziolo]

I wasn't aware that we use https://github.com/WordPress/wordpress-develop for e2e testing but I think it's a valid approach that helps to catch issues like this immediately 👍

With this commit, many fields now assign a default "empty" value, either empty string, empty array, or null.

@aduth, thank you for flagging the root cause and providing the temporary fix that gives us some time to discuss the long-term solution. As explained in #22812, we should refactor codebase to start using the new block types REST API endpoint to fetch the list of the blocks from the server. https://core.trac.wordpress.org/changeset/47875 was based on the implementation of the endpoint. It's quite difficult to navigate between PHP and JS because block types were initially client-side only. It looks like there are a few things we could improve:

• attributes on the server for backward compatibility might need to allow null to account for the logic that skips validations for attributes when it isn't an array, on the client we should override null with an empty object ({})
• category is interesting, historically it was mandatory, but it looks like we are leaning towards making it optional as discussed during the implementation of the latest changes to a revised list of categories (Blocks: Update default block categories #19279) or when developing categories for Block Patterns (Add Block Pattern Categories support #22164). Based on the actual implementation it should be neither null or ''.
• icon is tricky as well because the default value assigned on the client-side uses SVG element (React object instance) that can't be defined as is on the server-side.

So the question here is whether we should make category and icon default to null in WP_Block_Type? For the category alone, should it error when it has falsy value? In general, it would be good to revisit how data from the client and the server are combined together. We should also account for the case when the block is registered only on the client and use more default values, not only keywords: [] but also styles: [] or supports: {} (there is more).

[aduth]

• category is interesting, historically it was mandatory, but it looks like we are leaning towards making it optional as discussed during the implementation of the latest changes to a revised list of categories (Blocks: Update default block categories #19279) or when developing categories for Block Patterns (Add Block Pattern Categories support #22164). Based on the actual implementation it should be neither null or ''.

Yes, as of #22280 the category field is now optional. That said, the empty string '' wouldn't have been valid anyways.

So the question here is whether we should make category and icon default to null in WP_Block_Type?

If it's approached from the perspective of how it had worked previously, the client-side registration relies on the absence of the value to determine if fallbacks should apply. I assume this could need to change if server-side (and at least from the REST endpoint), we want to have a consistent set of properties, even if those properties are "empty". So I think that null would get us the closest compromise between what is being treated as "unset".

This distinction of registration vs. registry could be important, and also affect how we consider block type storage in the client. I think it'd be reasonable to assume that registration in both PHP and JavaScript support the omission of certain optional properties, but for how that type is represented in the registry of block types, we'd expect that the properties always be assigned, even if to some "empty" default.

For me, I see a few specific points of action:

1. I do think icon and category should default to null
   • In the former, null could be considered a valid and handled case for rendering an icon. But the client expects that if given a string, the string is of a set of Dashicon slugs, which the empty string '' is not.
   • Likewise, if category is a string, it's assumed to be one of a set of valid categories, which the empty string '' is not.
2. Especially considering Replace unstable__bootstrapServerSideBlockDefinitions with call to block type REST API #22812, we could expect that we will want to pipe output of a block registry containing all possible block properties into the client-side registration. This could affect one of the following:
   1. Block registration should anticipate null and treat it equivalent to undefined in how it considers fallback / default values.
   2. Whichever mechanism responsible for piping that data should unset null values before providing it to the registration function.
3. To bring consistency between how block types are stored server-side and client-side, the client-side should also always assign all known properties of the block type in storage (or at least block type selectors), if not already done.

While it doesn't affect the client-side implementation very much, I'd also wonder about editor_script, script, editor_style, and style being assigned to empty strings as defaults. In general, it feels that null is the better representation of an empty value. As seen already, if provided as the same type in which its non-empty behavior is expected (e.g. a script handle), it can cause some breakage (e.g. this issue with icon as empty string).

[gziolo]

I have proposal that should address all points we discussed in #22849. It should follow up with PHP changes in WordPress core.

[aduth]

Removing "[Priority] High" label, since #22811 should at least give some breathing room for working to a complete resolution.