Try fixing Heading block validation errors on native

[Tug]

Description

This is a WIP. I'm trying to fix the block validation errors we're seeing on Gutenberg RN. Also experimenting with (auto?-)migration

Block validation: Block validation failed for `core/heading` ({name: "core/heading", save: ƒ, category: "common", attributes: {…}, title: "Heading", …}).

Content generated by `save` function:

<h2 class="has-text-align-left">Meet your new best friends, Blocks</h2>

Content retrieved from post body:

<h2 style="text-align:left">Meet your new best friends, Blocks</h2>

How has this been tested?

TBD

Types of changes

React Native

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.

[gziolo]

What kind of error do you see in React Native? There is no such attribute like style for this block so I don't think this proposal will work as expected.

[Tug]

@gziolo The error is mentioned in the PR description.
Indeed the isEligible change isn't useful I'm still experimenting but I think I understood how it works now. Tell me if I'm wrong:

• Blocks markup is parsed, and for each block we run block validation first
• If block validation fails we run deprecated blocks alternatives with the parsed attributes and check if there is an eligible one. If there is one it replaces the default one?
• If no eligible block is found we still try to create the default one using the parsed attributes.
• In the case of a change in markup, because of a change in implementation in the block save function (like you did in Block library: Refactor Heading block to use class names for text align #16035), the new markup is created and the post content is automatically saved, without asking for user consent.

[Tug]

The problem we're having in RN is those console.error calls in https://github.com/WordPress/gutenberg/blob/master/packages/blocks/src/api/validation.js#L635
They show an error screen on RN which can be dismissed of course, but also confused with a real error. The ideal would be to have those displayed as errors when block validation fails and they cannot be created/migrated automatically. Otherwise a console.warn would be more appropriate, what do you think?

[gziolo]

The problem we're having in RN is those console.error calls in https://github.com/WordPress/gutenberg/blob/master/packages/blocks/src/api/validation.js#L635
They show an error screen on RN which can be dismissed of course, but also confused with a real error. The ideal would be to have those displayed as errors when block validation fails and they cannot be created/migrated automatically. Otherwise a console.warn would be more appropriate, what do you think?

Yes, everything makes sense in what you said. In general, the block you shared gets migrated to the latest version when you use it in the web context, but the error message is confusing. It gives a false impression that something went wrong. In fact, I would be in favor of using console.info in the case when migration was successful to let the developer know that deprecation logic was triggered and let them debug which one was exactly applied.

I think we can update handling of the following error as well:

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

Lines 641 to 647 in 00c82b4

	log.error(
	'Block validation failed for `%s` (%o).\n\nContent generated by `save` function:\n\n%s\n\nContent retrieved from post body:\n\n%s',
	blockType.name,
	blockType,
	generatedBlockContent,
	originalBlockContent
	);

In both cases, if there is any deprecated version which matches the content saved, we should never print an error to the console.

[gziolo]

• In the case of a change in markup, because of a change in implementation in the block save function (like you did in Block library: Refactor Heading block to use class names for text align #16035), the new markup is created and the post content is automatically saved, without asking for user consent.

Yes, this is automated as it's developer's role to provide migration logic which ensures there is no content loss. The assumption is that the developer knows what to do :)

[getdave]

I'm also seeing this error on web.

[gziolo]

I have PR started which will be less dramatic when the block is updated based on the deprecation logic:
#16862

It needs some further refinements but overall it shows the direction.

[gziolo]

I have PR started which will be less dramatic when the block is updated based on the deprecation logic:
#16862

Can we consider this PR as resolved now that #16862 was merged? It should warn now less frequently and in more appropriate cases. There still might be some duplication of messages for nested blocks. I plan to tackle it later.