Reusable blocks: prevent infinite recursion #28405
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This reverts commit c228df2.
mcsf
added
[Type] Bug
mcsf
commented
Jan 21, 2021
Would your solution also handle infinite loops that involve more than one block type? (e.g. a reusable block and a template part that both include each other) Tagging @bobbingwide |
6 tasks
I just tested this and seems to work well! I tested with many combinations, including:
|
ntsekouras
reviewed
Jan 22, 2021
|
Size Change: +554 B (0%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
Did you try?
I'd like to see these notes and test results. |
aristath
approved these changes
Jan 22, 2021
ntsekouras
reviewed
Jan 22, 2021
gziolo
reviewed
Jan 22, 2021
| const RenderedRefsContext = createContext( [] ); | ||
|
|
||
| export default function useNoRecursiveRenders( ref ) { | ||
| const previouslyRenderedRefs = useContext( RenderedRefsContext ); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Agreed, thanks for the note!
gziolo
reviewed
Jan 22, 2021
There was a problem hiding this comment.
It looks solid, I really like useNoRecursiveRenders hook with the accompanying tests at is could be reused with other blocks. Later (after this fix is done), we can consider moving it to the @wordpress/block-editor package.
It looks like it only needs some polishing on UI side. Great work @mcsf 👏🏻
Addresses feedback by ntsekouras
mcsf
added a commit
that referenced
this pull request
Jan 22, 2021
Originally introduced in #28405 to prevent Reusable Blocks from infinitely and fatally recurring, React hook `useNoRecursiveRenders` has a place in the block-editor package so that other block types susceptible to recursion can be fixed too.
6 tasks
|
Opened #28428 as a follow-up. |
gziolo
pushed a commit
that referenced
this pull request
Jan 25, 2021
* Block Editor: Add useNoRecursiveRenders Originally introduced in #28405 to prevent Reusable Blocks from infinitely and fatally recurring, React hook `useNoRecursiveRenders` has a place in the block-editor package so that other block types susceptible to recursion can be fixed too. * Update variables to reflect uses beyond Reusable Block * useNoRecursiveRenders: Use Set instead of Array * Add JSDoc comment.
|
I started playing with applying the same technique for the Template Part block that is a bit more tricky to handle as it doesn't provide a unique identifier. Anyway, when going through the PHP implementation I was worried that the change proposed prevents rendering the same Reusable block multiple times in one editor. I was able to confirm that it doesn't work, see: Editor: Front-end: While it works perfectly fine in the editor because it is using Context API, in PHP it uses |
6 tasks
#26951 solved this by unsetting the block from the static variable after the end of the render. It should be an easy fix I think. |
|
Thanks @gziolo for bringing this up! I'm a bit surprised that the issue wasn't caught by this test 🤔 |
Ah, disregard. The e2e test checks only the editor, not the frontend (PHP). |
mcsf
added a commit
that referenced
this pull request
Jan 25, 2021
Since #28405, `render_block_core_block` prevents fatal rendering loops in Reusable Blocks by aborting if a given block has already been rendered. This effectively prevents loops, but also unintentionally prevented the same reusable block from being rendered twice on the same page, even if one isn't nested in the other. This commit fixes this by explicitly forgetting a Reusable Block after it has finished rendering.
6 tasks
Thanks for the report! Opened a fix at #28461. |
mcsf
added a commit
that referenced
this pull request
Jan 25, 2021
Since #28405, `render_block_core_block` prevents fatal rendering loops in Reusable Blocks by aborting if a given block has already been rendered. This effectively prevents loops, but also unintentionally prevented the same reusable block from being rendered twice on the same page, even if one isn't nested in the other. This commit fixes this by explicitly forgetting a Reusable Block after it has finished rendering.
6 tasks
gziolo
added a commit
that referenced
this pull request
Mar 8, 2021
* Template Part: Prevent infinite recursion Based on #28405 from @mcsf. Tries to apply the same technique used for Reusable block to prevent infinite recursion when the same block is inserted into itself at any level of nesting. * Change the order of warning messages * Prevents a block from rendering itself only when the same block type * Fix styling issues in PHP file for Template Part block * Add fixes for issues discovered while testing * Ensure immutability in useNoRecursiveRenders * Rename ref to uniqueId in the test
7 tasks
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Alternative to: #27012, #26951
Fixes: #18704
In contrast with the aforementioned pull requests, this builds on the opinion that any block type capable of recursion should be responsible for its own infinite loops. In other words, it isn't up to the block editor framework to detect and prevent block loops.
On the front end. This is the simplest end of the fix. Thanks to PHP's single-pass execution, it is enough to keep a static array inside the
render_block_core_block. If a given Reusable Blockrefis already present in the array, prevent further rendering and issue a PHP warning (E_USER_WARNING).On the client. We resort to React context in order to identify any blocks that have already been rendered in the same render tree. The principle here is that each
ReusableBlockEditinstance first reads and then expands the context:This behaviour is encapsulated in the
useNoRecursiveRendershook, for which tests are provided.Notes
useNoRecursiveRendersis general enough that other block types. The Template Part block would be able to use it directly.block-library/src/blockbecause my priority is to prevent fatals reported out there in production, but once this PR is merged we can expand the fix to Template Part.useNoRecursiveRenders'spreviouslyRenderedRefscould be aSetinstead of anArray.How has this been tested?
useNoRecursiveRenderScreenshots
Types of changes
Bug fix
Checklist: