-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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.
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
Block Example: Avoid a crash when block is not registered #55686
Conversation
Size Change: +60 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
@@ -2276,4 +2277,33 @@ describe( 'block factory', () => { | |||
expect( isContainerGroupBlock( 'core/group' ) ).toBe( false ); | |||
} ); | |||
} ); | |||
|
|||
describe( 'getBlockFromExample', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is similar to this test:
gutenberg/packages/blocks/src/api/test/templates.js
Lines 209 to 219 in b6b4cf6
it( 'should replace unregistered blocks from template with core/missing block', () => { | |
const template = [ | |
[ 'core/test-block' ], | |
[ 'core/test-block-2' ], | |
[ 'core/test-faker' ], | |
]; | |
expect( | |
synchronizeBlocksWithTemplate( [], template )[ 2 ].name | |
).toEqual( 'core/missing' ); | |
} ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @t-hamano!
We could also catch the error coming from the createBlock
. What do you think?
Proposed change
export const getBlockFromExample = ( name, example ) => {
try {
return createBlock(
name,
example.attributes,
( example.innerBlocks ?? [] ).map( ( innerBlock ) =>
getBlockFromExample( innerBlock.name, innerBlock )
)
);
} catch {
return createBlock( 'core/missing', {
originalName: name,
originalContent: '',
originalUndelimitedContent: '',
} );
}
};
Cool! I updated it like that in b5cd693. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @t-hamano!
Changes look good to me!
* Block Example: Avoid a crash when block is not registered * Use a try-catch statement
* Block Example: Avoid a crash when block is not registered * Use a try-catch statement
Fixe #54786
What?
This PR fixes a critical error that occurs when
getBlockFromExample()
is passed an unregistered block.Why?
This is because the
createBlock()
function running internally throws an exception if the block does not exist.How?
As far as reading this comment, I think that throwing an exception when the block does not exist is the intended behavior. I think we should check if the block exists before running
createBlock()
and replace it with acore/missing
block as a fallback if it doesn't exist, as shown in this code.Another approach might be to implicitly transform the unregistered block passed to the
createBlock()
function into acore/missing
block instead of throwing an exception. However, I personally find this approach a bit unnatural.Testing Instructions
This PR should fix critical bugs that occur in the following four cases. In each case, I assume a scenario where the Columns block contains an Image block that is not registered in the example.
Preview in block inserter
wp.blocks.unregisterBlockType('core/image')
from the browser console.Preview block style
wp.blocks.unregisterBlockType('core/image')
from the browser console.wp.blocks.registerBlockStyle('core/columns',{name:'test',label:'Test'});
from the browser console.Style Book
wp.blocks.unregisterBlockType('core/image')
from the browser console.Preview in the Global Styles
wp.blocks.unregisterBlockType('core/image')
from the browser console.