-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Patterns (unsynced): prevent infinite loops due to recursive patterns #56511
Changes from 9 commits
014a684
62e3b0b
08546e7
2b7a974
109fcd8
06edbd2
559623e
ca82ca5
16a8fec
940e68b
18c8edc
d09b7c9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,103 @@ | ||
/** | ||
* THIS MODULE IS INTENTIONALLY KEPT WITHIN THE PATTERN BLOCK'S SOURCE. | ||
* | ||
* This is because this approach for preventing infinite loops due to | ||
* recursively rendering blocks is specific to the way that the `core/pattern` | ||
* block behaves in the editor. Any other block types that deal with recursion | ||
* SHOULD USE THE STANDARD METHOD for avoiding loops: | ||
* | ||
* @see https://github.com/WordPress/gutenberg/pull/31455 | ||
* @see packages/block-editor/src/components/recursion-provider/README.md | ||
*/ | ||
|
||
/** | ||
* @type {Map<string, Set<string>>} | ||
*/ | ||
const patternDependencies = new Map(); | ||
|
||
/** | ||
* Parse a given pattern and traverse its contents to detect any subsequent | ||
* patterns on which it may depend. Such occurrences will be added to an | ||
* internal dependency graph. If a circular dependency is detected, an | ||
* error will be thrown. | ||
* | ||
* @param {Object} pattern Pattern. | ||
* @param {string} pattern.name Pattern name. | ||
* @param {Array} pattern.blocks Pattern's block list. | ||
* | ||
* @throws {Error} If a circular dependency is detected. | ||
*/ | ||
export function parsePatternDependencies( { name, blocks } ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unfortunately, I don't think this function will detect all loops. Imagine a template part block or a synced pattern blocks. These will inject loops upon rendering and not in the initial blocks object as they first need to fetch the blocks using an API. there could be more blocks like that (query, third-party blocks...) In that sense, I'm not entirely sure that this is the best approach for the editor. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But have you tested? So far I’ve tried a few combinations, including with template parts, and the system has been able to prevent loops, thanks to the robustness of the pre-existing solution in template parts, synced patterns, etc. To be clear, I’m quite open to the idea that this approach needs to be changed, but before that I just wanted to see if we can identify a combination that bypasses the fix. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh right, so if it's not caught at the pattern level, it will be caught at the template part level ... So I think it's good enough you're right 👍 |
||
const queue = [ ...blocks ]; | ||
while ( queue.length ) { | ||
const block = queue.shift(); | ||
for ( const innerBlock of block.innerBlocks ?? [] ) { | ||
queue.unshift( innerBlock ); | ||
} | ||
if ( block.name === 'core/pattern' ) { | ||
registerDependency( name, block.attributes.slug ); | ||
} | ||
} | ||
} | ||
|
||
/** | ||
* Declare that pattern `a` depends on pattern `b`. If a circular | ||
* dependency is detected, an error will be thrown. | ||
* | ||
* Exported for testing purposes only. | ||
* | ||
* @param {string} a Slug for pattern A. | ||
* @param {string} b Slug for pattern B. | ||
* | ||
* @throws {Error} If a circular dependency is detected. | ||
*/ | ||
export function registerDependency( a, b ) { | ||
if ( ! patternDependencies.has( a ) ) { | ||
patternDependencies.set( a, new Set() ); | ||
} | ||
patternDependencies.get( a ).add( b ); | ||
|
||
if ( hasCycle( a ) ) { | ||
throw new TypeError( | ||
`Pattern ${ a } has a circular dependency and cannot be rendered.` | ||
); | ||
} | ||
} | ||
|
||
/** | ||
* Determine if a given pattern has circular dependencies on other patterns. | ||
* This will be determined by running a depth-first search on the current state | ||
* of the graph represented by `patternDependencies`. | ||
* | ||
* @param {string} slug Pattern slug. | ||
* @param {Set<string>} [visitedNodes] Set to track visited nodes in the graph. | ||
* @param {Set<string>} [currentPath] Set to track and backtrack graph paths. | ||
* @return {boolean} Whether any cycle was found. | ||
*/ | ||
function hasCycle( slug, visitedNodes = new Set(), currentPath = new Set() ) { | ||
visitedNodes.add( slug ); | ||
currentPath.add( slug ); | ||
|
||
const dependencies = patternDependencies.get( slug ) ?? new Set(); | ||
|
||
for ( const dependency of dependencies ) { | ||
if ( ! visitedNodes.has( dependency ) ) { | ||
if ( hasCycle( dependency, visitedNodes, currentPath ) ) { | ||
return true; | ||
} | ||
} else if ( currentPath.has( dependency ) ) { | ||
return true; | ||
} | ||
} | ||
|
||
// Remove the current node from the current path when backtracking | ||
currentPath.delete( slug ); | ||
return false; | ||
} | ||
|
||
/** | ||
* Exported for testing purposes only. | ||
*/ | ||
export function clearPatternDependencies() { | ||
patternDependencies.clear(); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,73 @@ | ||
/** | ||
* Internal dependencies | ||
*/ | ||
import { | ||
parsePatternDependencies, | ||
registerDependency, | ||
clearPatternDependencies, | ||
} from '../recursion-detector'; | ||
|
||
describe( 'core/pattern', () => { | ||
beforeEach( () => { | ||
clearPatternDependencies(); | ||
} ); | ||
|
||
describe( 'parsePatternDependencies', () => { | ||
it( "is silent for patterns that don't require other patterns", () => { | ||
const pattern = { | ||
name: 'test/benign-pattern', | ||
blocks: [ { name: 'core/paragraph' } ], | ||
}; | ||
expect( () => { | ||
parsePatternDependencies( pattern ); | ||
} ).not.toThrow(); | ||
} ); | ||
it( 'catches self-referencing patterns', () => { | ||
const pattern = { | ||
name: 'test/evil-pattern', | ||
blocks: [ { name: 'core/pattern', slug: 'test/evil-pattern' } ], | ||
}; | ||
expect( () => { | ||
parsePatternDependencies( pattern ); | ||
} ).toThrow(); | ||
} ); | ||
} ); | ||
|
||
describe( 'registerDependency', () => { | ||
it( 'is silent for patterns with no circular dependencies', () => { | ||
expect( () => { | ||
registerDependency( 'a', 'b' ); | ||
} ).not.toThrow(); | ||
} ); | ||
it( 'catches self-referencing patterns', () => { | ||
expect( () => { | ||
registerDependency( 'a', 'a' ); | ||
} ).toThrow(); | ||
} ); | ||
it( 'catches mutually-referencing patterns', () => { | ||
registerDependency( 'a', 'b' ); | ||
expect( () => { | ||
registerDependency( 'b', 'a' ); | ||
} ).toThrow(); | ||
} ); | ||
it( 'catches longer cycles', () => { | ||
registerDependency( 'a', 'b' ); | ||
registerDependency( 'b', 'c' ); | ||
registerDependency( 'b', 'd' ); | ||
expect( () => { | ||
registerDependency( 'd', 'a' ); | ||
} ).toThrow(); | ||
} ); | ||
it( 'catches any pattern depending on a tainted one', () => { | ||
registerDependency( 'a', 'b' ); | ||
registerDependency( 'b', 'c' ); | ||
registerDependency( 'b', 'd' ); | ||
expect( () => { | ||
registerDependency( 'd', 'a' ); | ||
} ).toThrow(); | ||
expect( () => { | ||
registerDependency( 'e', 'd' ); | ||
} ).toThrow(); | ||
} ); | ||
} ); | ||
} ); |
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 is a global variable, so if you have two different block editors rendered on the same page, they'll share the same variable, do you think we should tie this to the "block-editor" store or something?
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.
One "simple way" to do that is to check that we're using the same "Registry" object. you can use the registry object as a key in a weak map and you can retrieve it from the
useRegistry
hook.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.
The assumption I made to justify such a global was that patterns are uniquely named. So it shouldn't matter if there are more than one editor using different patterns.
I think it's good to take a step back and think about what we are guarding against. To break the assumption would mean that someone is somehow:
That said, I'm open to the idea that, in principle, the lifetime of
patternDependencies
should be tied to the corresponding pattern registry. I like the sound of your suggestion to use registries as keys in a weak map.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.
I think we've used that in other places (using registries as keys) and yeah you're right, they need to have the same names for different patterns.