Try: Fix issue where block fails validation when a default attribute is deprecated by talldan · Pull Request #12757 · WordPress/gutenberg

talldan · 2018-12-10T10:55:37Z

Description

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?

Add a columns block to a new post (keeping the number of columns at the default of 2) and save the post
Modify the code of the columns block, changing the columns attribute default from 2 to 3.
Load the post created in step 1 and observe that the columns block failed validation.
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.
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 · 2018-12-10T13:24:56Z

There appear to be some legitimate test failures here.

talldan · 2018-12-11T05:12:14Z

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

talldan · 2019-01-02T03:14:31Z

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 · 2019-02-06T21:04:46Z

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 · 2019-02-06T21:12:28Z

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

 			expect( migratedBlock.attributes ).toEqual( { fruit: 'Bananas!' } );
 		} );
+
+		it( 'allows a default attribute to be deprecated', () => {


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.

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 · 2019-02-08T07:05:21Z

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.

…attributes from the failed created block

…over deprecated default use case

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 👍

…is deprecated (#12757) * Use attributes as parsed when determining a block migration, not the attributes from the failed created block * Updated tests with new migratedBlocks signature and add new test to cover deprecated default use case * Fix typo with attribute type in test

talldan added [Type] Bug An existing feature does not function as intended [Feature] Block API API that allows to express the block paradigm. [Package] Blocks /packages/blocks labels Dec 10, 2018

talldan self-assigned this Dec 10, 2018

talldan requested a review from a team December 10, 2018 11:01

talldan added the Needs Technical Feedback Needs testing from a developer perspective. label Jan 2, 2019

gziolo requested review from aduth, mcsf and youknowriad February 6, 2019 12:46

gziolo added this to the 5.2 (Gutenberg) milestone Feb 6, 2019

aduth reviewed Feb 6, 2019

View reviewed changes

talldan added 3 commits February 8, 2019 16:36

Use attributes as parsed when determining a block migration, not the …

41221b2

…attributes from the failed created block

Updated tests with new migratedBlocks signature and add new test to c…

c60132f

…over deprecated default use case

Fix typo with attribute type in test

ec0e490

talldan force-pushed the try/fix-default-attribute-deprecation branch from 059edef to ec0e490 Compare February 8, 2019 08:44

talldan requested review from gziolo and noisysocks as code owners February 8, 2019 08:44

mcsf approved these changes Feb 15, 2019

View reviewed changes

aduth approved these changes Feb 18, 2019

View reviewed changes

talldan removed the Needs Technical Feedback Needs testing from a developer perspective. label Feb 19, 2019

talldan merged commit 0254aaf into master Feb 19, 2019

talldan deleted the try/fix-default-attribute-deprecation branch February 19, 2019 09:02

ellatrix mentioned this pull request Mar 4, 2019

Bump plugin version to 5.2.0-rc.1 #14210

Merged

talldan mentioned this pull request May 7, 2019

Setting the alignment property of the "Media & Text" block to "wide" does not add block attribute #15476

Closed

This was referenced Jun 27, 2019

Group blocks won't validate after update to 6.0 #16328

Closed

Fix group block deprecation #16346

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

Try: Fix issue where block fails validation when a default attribute is deprecated#12757
talldan merged 3 commits intomasterfrom
try/fix-default-attribute-deprecation

talldan commented Dec 10, 2018 •

edited

Loading

Uh oh!

aduth commented Dec 10, 2018

Uh oh!

talldan commented Dec 11, 2018

Uh oh!

talldan commented Jan 2, 2019 •

edited

Loading

Uh oh!

aduth commented Feb 6, 2019

Uh oh!

aduth Feb 6, 2019

Uh oh!

talldan Feb 8, 2019

Uh oh!

talldan commented Feb 8, 2019

Uh oh!

mcsf left a comment

Uh oh!

aduth left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	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;
	}

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

Conversation

talldan commented Dec 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How has this been tested?

Diagnosis

Fix

Types of changes

Checklist:

Uh oh!

aduth commented Dec 10, 2018

Uh oh!

talldan commented Dec 11, 2018

Uh oh!

talldan commented Jan 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aduth commented Feb 6, 2019

Uh oh!

aduth Feb 6, 2019

Choose a reason for hiding this comment

Uh oh!

talldan Feb 8, 2019

Choose a reason for hiding this comment

Uh oh!

talldan commented Feb 8, 2019

Uh oh!

mcsf left a comment

Choose a reason for hiding this comment

Uh oh!

aduth left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

talldan commented Dec 10, 2018 •

edited

Loading

talldan commented Jan 2, 2019 •

edited

Loading