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.

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

Reusable blocks: prevent infinite recursion #28405

Merged
merged 12 commits into from
Jan 22, 2021
2 changes: 1 addition & 1 deletion packages/block-editor/src/components/warning/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
line-height: $default-line-height;
font-family: $default-font;
font-size: $default-font-size;
color: $gray-900;
color: $gray-900 !important;
ntsekouras marked this conversation as resolved.
Show resolved Hide resolved
margin: 0;
}

Expand Down
60 changes: 38 additions & 22 deletions packages/block-library/src/block/edit.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,12 @@ import { store as reusableBlocksStore } from '@wordpress/reusable-blocks';
/**
* Internal dependencies
*/
import useNoRecursiveRenders from './use-no-recursive-renders';

export default function ReusableBlockEdit( { attributes: { ref }, clientId } ) {
const [ hasAlreadyRendered, RecursionProvider ] = useNoRecursiveRenders(
ref
);
const { isMissing, hasResolved } = useSelect(
( select ) => {
const persistedBlock = select( coreStore ).getEntityRecord(
Expand Down Expand Up @@ -83,6 +87,16 @@ export default function ReusableBlockEdit( { attributes: { ref }, clientId } ) {

const blockProps = useBlockProps();

if ( hasAlreadyRendered ) {
return (
<div { ...blockProps }>
<Warning>
{ __( 'Block cannot be rendered inside itself.' ) }
</Warning>
</div>
);
}

if ( isMissing ) {
return (
<div { ...blockProps }>
Expand All @@ -104,28 +118,30 @@ export default function ReusableBlockEdit( { attributes: { ref }, clientId } ) {
}

return (
<div { ...blockProps }>
<BlockControls>
<ToolbarGroup>
<ToolbarButton
onClick={ () => convertBlockToStatic( clientId ) }
>
{ __( 'Convert to regular blocks' ) }
</ToolbarButton>
</ToolbarGroup>
</BlockControls>
<InspectorControls>
<PanelBody>
<TextControl
label={ __( 'Name' ) }
value={ title }
onChange={ setTitle }
/>
</PanelBody>
</InspectorControls>
<div className="block-library-block__reusable-block-container">
{ <div { ...innerBlocksProps } /> }
<RecursionProvider>
<div { ...blockProps }>
<BlockControls>
<ToolbarGroup>
<ToolbarButton
onClick={ () => convertBlockToStatic( clientId ) }
>
{ __( 'Convert to regular blocks' ) }
</ToolbarButton>
</ToolbarGroup>
</BlockControls>
<InspectorControls>
<PanelBody>
<TextControl
label={ __( 'Name' ) }
value={ title }
onChange={ setTitle }
/>
</PanelBody>
</InspectorControls>
<div className="block-library-block__reusable-block-container">
{ <div { ...innerBlocksProps } /> }
</div>
</div>
</div>
</RecursionProvider>
);
}
17 changes: 17 additions & 0 deletions packages/block-library/src/block/index.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
* @return string Rendered HTML of the referenced block.
*/
function render_block_core_block( $attributes ) {
static $seen_refs = array();

if ( empty( $attributes['ref'] ) ) {
return '';
}
Expand All @@ -22,6 +24,21 @@ function render_block_core_block( $attributes ) {
return '';
}

if ( in_array( $attributes['ref'], $seen_refs, true ) ) {
trigger_error(
ntsekouras marked this conversation as resolved.
Show resolved Hide resolved
sprintf(
// translators: %s is the user-provided title of the reusable block.
__( 'Could not render Reusable Block <strong>%s</strong>: blocks cannot be rendered inside themselves.' ),
$reusable_block->post_title
),
E_USER_WARNING
);

return '[block rendering halted]';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thoughts on leaving this here or returning ''?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd go for returning it, as it can be overlooked that there is a problem, by having the warnings not visible.

Copy link
Member

Choose a reason for hiding this comment

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

I don't know if that is helpful for someone browsing the website. It's not that far to displaying an error when PHP can't connect with the database. You could display it maybe when WP_DEBUG is enabled but maybe with a ton of surrounding red colors 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

}

$seen_refs[] = $attributes['ref'];

if ( 'publish' !== $reusable_block->post_status || ! empty( $reusable_block->post_password ) ) {
return '';
}
Expand Down
100 changes: 100 additions & 0 deletions packages/block-library/src/block/test/use-no-recursive-renders.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
/**
* External dependencies
*/
import { render } from '@testing-library/react';

/**
* WordPress dependencies
*/
import { Fragment } from '@wordpress/element';

// Mimics a block's Edit component, such as ReusableBlockEdit, which is capable
// of calling itself depending on its `ref` attribute.
function Edit( { attributes: { ref } } ) {
const [ hasAlreadyRendered, RecursionProvider ] = useNoRecursiveRenders(
ref
);

if ( hasAlreadyRendered ) {
return <div className="wp-block__reusable-block--halted">Halt</div>;
}

return (
<RecursionProvider>
<div className="wp-block__reusable-block">
{ ref === 'SIMPLE' && <p>Done</p> }
{ ref === 'SINGLY-RECURSIVE' && (
<Edit attributes={ { ref } } />
) }
{ ref === 'MUTUALLY-RECURSIVE-1' && (
<Edit attributes={ { ref: 'MUTUALLY-RECURSIVE-2' } } />
) }
{ ref === 'MUTUALLY-RECURSIVE-2' && (
<Edit attributes={ { ref: 'MUTUALLY-RECURSIVE-1' } } />
) }
</div>
</RecursionProvider>
);
}

/**
* Internal dependencies
*/
import useNoRecursiveRenders from '../use-no-recursive-renders';

describe( 'useNoRecursiveRenders', () => {
it( 'allows a single block to render', () => {
const { container } = render(
<Edit attributes={ { ref: 'SIMPLE' } } />
);
expect(
container.querySelectorAll( '.wp-block__reusable-block' )
).toHaveLength( 1 );
expect(
container.querySelectorAll( '.wp-block__reusable-block--halted' )
).toHaveLength( 0 );
} );

it( 'allows equal but sibling blocks to render', () => {
const { container } = render(
<Fragment>
<Edit attributes={ { ref: 'SIMPLE' } } />
<Edit attributes={ { ref: 'SIMPLE' } } />
</Fragment>
);
expect(
container.querySelectorAll( '.wp-block__reusable-block' )
).toHaveLength( 2 );
expect(
container.querySelectorAll( '.wp-block__reusable-block--halted' )
).toHaveLength( 0 );
} );

it( 'prevents a block from rendering itself', () => {
const { container } = render(
<Fragment>
<Edit attributes={ { ref: 'SINGLY-RECURSIVE' } } />
</Fragment>
);
expect(
container.querySelectorAll( '.wp-block__reusable-block' )
).toHaveLength( 1 );
expect(
container.querySelectorAll( '.wp-block__reusable-block--halted' )
).toHaveLength( 1 );
} );

it( 'prevents mutual recursion between two blocks', () => {
const { container } = render(
<Fragment>
<Edit attributes={ { ref: 'MUTUALLY-RECURSIVE-1' } } />
</Fragment>
);
expect(
container.querySelectorAll( '.wp-block__reusable-block' )
).toHaveLength( 2 );
expect(
container.querySelectorAll( '.wp-block__reusable-block--halted' )
).toHaveLength( 1 );
} );
} );
29 changes: 29 additions & 0 deletions packages/block-library/src/block/use-no-recursive-renders.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/**
* WordPress dependencies
*/
import {
createContext,
useCallback,
useContext,
useMemo,
} from '@wordpress/element';

const RenderedRefsContext = createContext( [] );

export default function useNoRecursiveRenders( ref ) {
const previouslyRenderedRefs = useContext( RenderedRefsContext );
Copy link
Member

Choose a reason for hiding this comment

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

Not a blocker for this PR. I wanted to share my thought on how to make it general enough to use with other blocks that require the same guard (Post Content or Template Part). It looks like it would be enough to rename ref to something more generic like uniqueId and use Set rather than array of strings for checks to support compound values for more complex blocks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, thanks for the note!

const hasAlreadyRendered = previouslyRenderedRefs.includes( ref );
const newRenderedRefs = useMemo( () => [ ...previouslyRenderedRefs, ref ], [
ref,
previouslyRenderedRefs,
] );
const Provider = useCallback(
( { children } ) => (
<RenderedRefsContext.Provider value={ newRenderedRefs }>
{ children }
</RenderedRefsContext.Provider>
),
[ newRenderedRefs ]
);
return [ hasAlreadyRendered, Provider ];
}