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

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

Merged
merged 3 commits into from
Feb 19, 2019
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
41221b2
Use attributes as parsed when determining a block migration, not the …
talldan Dec 10, 2018
c60132f
Updated tests with new migratedBlocks signature and add new test to c…
talldan Dec 11, 2018
ec0e490
Fix typo with attribute type in test
talldan Feb 8, 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
16 changes: 9 additions & 7 deletions packages/blocks/src/api/parser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,26 +304,28 @@ export function getBlockAttributes( blockTypeOrName, innerHTML, attributes = {}
* deprecated migrations applied, or the original block if it was both valid
* and no eligible migrations exist.
*
* @param {WPBlock} block Original block object.
* @param {WPBlock} block Original block object.
* @param {Object} parsedAttributes Attributes as parsed from the initial
* block markup.
*
* @return {WPBlock} Migrated block object.
*/
export function getMigratedBlock( block ) {
export function getMigratedBlock( block, parsedAttributes ) {
const blockType = getBlockType( block.name );

const { deprecated: deprecatedDefinitions } = blockType;
if ( ! deprecatedDefinitions || ! deprecatedDefinitions.length ) {
return block;
}

const { originalContent, attributes, innerBlocks } = block;
const { originalContent, innerBlocks } = block;

for ( let i = 0; i < deprecatedDefinitions.length; i++ ) {
// A block can opt into a migration even if the block is valid by
// defining isEligible on its deprecation. If the block is both valid
// and does not opt to migrate, skip.
const { isEligible = stubFalse } = deprecatedDefinitions[ i ];
if ( block.isValid && ! isEligible( attributes, innerBlocks ) ) {
if ( block.isValid && ! isEligible( parsedAttributes, innerBlocks ) ) {
continue;
}

Expand All @@ -338,7 +340,7 @@ export function getMigratedBlock( block ) {
let migratedAttributes = getBlockAttributes(
deprecatedBlockType,
originalContent,
attributes
parsedAttributes
);

// Ignore the deprecation if it produces a block which is not valid.
Expand All @@ -364,7 +366,7 @@ export function getMigratedBlock( block ) {
const { migrate } = deprecatedBlockType;
if ( migrate ) {
( [
migratedAttributes = attributes,
migratedAttributes = parsedAttributes,
migratedInnerBlocks = innerBlocks,
] = castArray( migrate( migratedAttributes, innerBlocks ) ) );
}
Expand Down Expand Up @@ -468,7 +470,7 @@ export function createBlockWithFallback( blockNode ) {
// invalid, or future serialization attempt results in an error.
block.originalContent = innerHTML;

block = getMigratedBlock( block );
block = getMigratedBlock( block, attributes );

return block;
}
Expand Down
78 changes: 62 additions & 16 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 @@ -367,23 +367,25 @@ describe( 'block parser', () => {

describe( 'getMigratedBlock', () => {
it( 'should return the original block if it has no deprecated versions', () => {
const parsedAttributes = {};
const block = deepFreeze( {
name: 'core/test-block',
attributes: {},
attributes: parsedAttributes,
originalContent: '<span class="wp-block-test-block">Bananas</span>',
isValid: false,
} );
registerBlockType( 'core/test-block', defaultBlockSettings );

const migratedBlock = getMigratedBlock( block );
const migratedBlock = getMigratedBlock( block, parsedAttributes );

expect( migratedBlock ).toBe( block );
} );

it( 'should return the original block if no valid deprecated version found', () => {
const parsedAttributes = {};
const block = deepFreeze( {
name: 'core/test-block',
attributes: {},
attributes: parsedAttributes,
originalContent: '<span class="wp-block-test-block">Bananas</span>',
isValid: false,
} );
Expand All @@ -398,20 +400,22 @@ describe( 'block parser', () => {
],
} );

const migratedBlock = getMigratedBlock( block );
const migratedBlock = getMigratedBlock( block, parsedAttributes );

expect( migratedBlock ).toBe( block );
expect( console ).toHaveErrored();
expect( console ).toHaveWarned();
} );

it( 'should return with attributes parsed by the deprecated version', () => {
const parsedAttributes = {};
const block = deepFreeze( {
name: 'core/test-block',
attributes: {},
attributes: parsedAttributes,
originalContent: '<span>Bananas</span>',
isValid: false,
} );

registerBlockType( 'core/test-block', {
...defaultBlockSettings,
save: ( props ) => <div>{ props.attributes.fruit }</div>,
Expand All @@ -429,15 +433,16 @@ describe( 'block parser', () => {
],
} );

const migratedBlock = getMigratedBlock( block );
const migratedBlock = getMigratedBlock( block, parsedAttributes );

expect( migratedBlock.attributes ).toEqual( { fruit: 'Bananas' } );
} );

it( 'should be able to migrate attributes and innerBlocks', () => {
const parsedAttributes = {};
const block = deepFreeze( {
name: 'core/test-block',
attributes: {},
attributes: parsedAttributes,
originalContent: '<span>Bananas</span>',
isValid: false,
} );
Expand Down Expand Up @@ -467,7 +472,7 @@ describe( 'block parser', () => {
],
} );

const migratedBlock = getMigratedBlock( block );
const migratedBlock = getMigratedBlock( block, parsedAttributes );

expect( migratedBlock.attributes ).toEqual( { newFruit: 'Bananas' } );
expect( migratedBlock.innerBlocks ).toHaveLength( 1 );
Expand All @@ -476,11 +481,10 @@ describe( 'block parser', () => {
} );

it( 'should ignore valid uneligible blocks', () => {
const parsedAttributes = { fruit: 'Bananas' };
const block = deepFreeze( {
name: 'core/test-block',
attributes: {
fruit: 'Bananas',
},
attributes: parsedAttributes,
originalContent: 'Bananas',
isValid: true,
} );
Expand All @@ -499,17 +503,16 @@ describe( 'block parser', () => {
],
} );

const migratedBlock = getMigratedBlock( block );
const migratedBlock = getMigratedBlock( block, parsedAttributes );

expect( migratedBlock.attributes ).toEqual( { fruit: 'Bananas' } );
} );

it( 'should allow opt-in eligibility of valid block', () => {
const parsedAttributes = { fruit: 'Bananas' };
const block = deepFreeze( {
name: 'core/test-block',
attributes: {
fruit: 'Bananas',
},
attributes: parsedAttributes,
originalContent: 'Bananas',
isValid: true,
} );
Expand All @@ -529,10 +532,53 @@ describe( 'block parser', () => {
],
} );

const migratedBlock = getMigratedBlock( block );
const migratedBlock = getMigratedBlock( block, parsedAttributes );

expect( migratedBlock.attributes ).toEqual( { fruit: 'Bananas!' } );
} );

it( 'allows a default attribute to be deprecated', () => {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

// The block's default fruit attribute has been changed from 'Bananas' to 'Oranges'.
registerBlockType( 'core/test-block', {
...defaultBlockSettings,
attributes: {
fruit: {
type: 'string',
default: 'Oranges',
},
},
deprecated: [
{
attributes: {
fruit: {
type: 'string',
default: 'Bananas',
},
},
save: defaultBlockSettings.save,
},
],
} );

// Because the fruits attribute is not sourced, when the block content was parsed no value for the
// fruit attribute was found.
const parsedAttributes = {};

// When the block was created, it was given the new default value for the fruit attribute of 'Oranges'.
// This is because unchanged default values are not saved to the comment delimeter attributes.
// Validation failed because this block was saved when the old default was 'Bananas' as reflected by the originalContent.
const block = deepFreeze( {
name: 'core/test-block',
attributes: { fruit: 'Oranges' },
originalContent: 'Bananas',
isValid: false,
} );

// The migrated block successfully falls back to the old value of 'Bananas', allowing the block to
// continue to be used.
const migratedBlock = getMigratedBlock( block, parsedAttributes );
expect( migratedBlock.attributes ).toEqual( { fruit: 'Bananas' } );
} );
} );

describe( 'createBlockWithFallback', () => {
Expand Down