Skip to content

Commit

Permalink
Try: Fix issue where block fails validation when a default attribute …
Browse files Browse the repository at this point in the history 
…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
  • Loading branch information
@talldan @youknowriad
talldan authored and youknowriad committed Mar 6, 2019
1 parent aaa63a8 commit 0d02d3b
Show file tree
Hide file tree
Showing 2 changed files with 71 additions and 23 deletions.
16 changes: 9 additions & 7 deletions packages/blocks/src/api/parser.js
View file
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
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', () => {
// 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

0 comments on commit 0d02d3b

Please sign in to comment.