Try: Fix issue where block fails validation when a default attribute is deprecated

[talldan]

Description

Fixes #11705

Block implementors are unable update their code to change the default value for an attribute, even when adding a deprecation. Block validation fails for posts that contain blocks still using an old default.

Note - this is only for attributes that aren't sourced

How has this been tested?

1. Add a columns block to a new post (keeping the number of columns at the default of 2) and save the post
2. Modify the code of the columns block, changing the columns attribute default from 2 to 3.
3. Load the post created in step 1 and observe that the columns block failed validation.
4. Add a deprecation for the columns block to try to solve the failed validation, ensure this new deprecation still has the old default value for the columns attribute of 2.
5. Load the post created in step 1.

Expected Result
The deprecation with the column count of 2 should be applied.

Actual Result
The block fails validation.

Diagnosis

The bug occurs because the attempt to apply the deprecation fails validation here and so the deprecation is skipped (isValid is false):

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

Lines 338 to 353 in 40d1282

	let migratedAttributes = getBlockAttributes(
	deprecatedBlockType,
	originalContent,
	attributes
	);
	// Ignore the deprecation if it produces a block which is not valid.
	const isValid = isValidBlockContent(
	deprecatedBlockType,
	migratedAttributes,
	originalContent
	);
	if ( ! isValid ) {
	continue;
	}

This is happening because the attributes variable in the above call to getBlockAttributes has the default value from the failed initial attempt to create the block (in case of the columns example above, that would be the new default of 3). Because of that the deprecation's default is not applied (as there's already an existing value).

Fix

Instead of using the attributes from the failed first attempt to create the block when validating a deprecation, this fix instead uses the attributes as they were when the block markup was parsed (those initially passed into createBlockWithFallback):

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

Lines 386 to 392 in 40d1282

	export function createBlockWithFallback( blockNode ) {
	const { blockName: originalName } = blockNode;
	let {
	attrs: attributes,
	innerBlocks = [],
	innerHTML,
	} = blockNode;

This means that a deprecation receives the same attributes that normal block creation would receive, which to me seems logical.

Types of changes

Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.

[aduth]

There appear to be some legitimate test failures here.

[talldan]

Oh, sorry about that, updated tests and added a test case to cover this fix.

[talldan]

Would like some developer feedback on this. I realise this introduces the need to deprecate getMigratedBlock due to the extra param, which is less than ideal.

One way around that could be to store the original attributes on the block in the same way as originalContent is here before the call to getMigratedBlock:

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

Lines 469 to 471 in 6262b60

	block.originalContent = innerHTML;
	block = getMigratedBlock( block );

I wanted to avoid mutation and that data being kept around longer than necessary.

Also, let me know if I need to clarify anything about this bug, it's not the easiest issue to explain.

[aduth]

I realise this introduces the need to deprecate getMigratedBlock due to the extra param, which is less than ideal.

Can you elaborate why a deprecation would occur, if getMigratedBlock is not part of any public interface?

[aduth]

Being a bit confused about what actually changed as part of the implementation aside from moving attributes into an argument rather than having it be destructured, I copied this test and this test only into master. It passed. I can't imagine that's expected? I'd expect the test case would be one which only passes in the branch, and fails on master.

[talldan]

Thanks for the catch - looks like I made a mistake in the test with attribute type, which was causing a false positive (the types didn't match so that attribute was being reset). Fixed here:
ec0e490

Being a bit confused about what actually changed as part of the implementation aside from moving attributes into an argument rather than having it be destructured.

It's really hard to explain!

The difference is that before this change, the attributes may have had defaults applied from the main block type:
https://github.com/WordPress/gutenberg/blob/master/packages/blocks/src/api/parser.js#L455

If the main block type fails validation, the deprecations are attempted, but the default value from main block type is passed in when testing the deprecations, which doesn't give the deprecations a chance to apply their own defaults.

My change instead passes the original attributes from before those defaults from the main block type have been applied into getMigratedBlock, which gives the deprecations a chance to use their defaults.

[talldan]

Can you elaborate why a deprecation would occur, if getMigratedBlock is not part of any public interface?

I think that's down to me misinterpreting what's public and what's internal. Thanks for the clarification.

[mcsf]

The change is well reasoned and tests well for me. Thanks for the fix, LGTM.

@aduth, did Dan clarify things for you?

[aduth]

Thanks for the explanation 👍