Patterns (unsynced): prevent infinite loops due to recursive patterns #56511
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
* Rename PatternsDependencyGraph to PatternRecursionDetector * Refactor module function hasCircularDependency to class private method * Export singleton PatternsDependencyGraph instance rather than class
mcsf
added
[Type] Bug
mcsf
commented
Nov 24, 2023
|
Size Change: +442 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
|
Added more thoughts under section Caveats. |
mcsf
force-pushed
the
fix/patterns-prevent-recursion
branch
from
f438e26 to
16a8fec
Compare
|
Flaky tests detected in d09b7c9. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7224667414
|
youknowriad
reviewed
Nov 30, 2023
| * | ||
| * @throws {Error} If a circular dependency is detected. | ||
| */ | ||
| export function parsePatternDependencies( { name, blocks } ) { |
There was a problem hiding this comment.
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.
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.
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 👍
youknowriad
reviewed
Nov 30, 2023
| /** | ||
| * @type {Map<string, Set<string>>} | ||
| */ | ||
| const patternDependencies = new Map(); |
There was a problem hiding this comment.
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.
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.
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:
- loading two block editors simultaneously, and
- registering different patterns with those block editors, and
- creating naming conflicts between those patterns
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.
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.
youknowriad
approved these changes
Nov 30, 2023
youknowriad
reviewed
Nov 30, 2023
I think it's worth adding E2E, but just one test case for the simplest case (self-referencing pattern). |
mcsf
commented
Dec 14, 2023
Comment on lines
16
to
18
| import { useRegistry } from '@wordpress/data'; | ||
|
|
||
| const patternDependenciesRegistry = new WeakMap(); |
There was a problem hiding this comment.
With the introduction of the data registry stuff, this nomenclature is getting confusing.
Also, I ended up minimising what we store in the weak map, so instead of storing some kind of instance (e.g. new PatternDependencies()), we just store a bound function (parsePatternDependencies.bind( null, new Map() )). So it's even less of a "registry".
Suggestions? Examples:
const validators = new WeakMap();
const patternParsers = new WeakMap();
// or simply...
const cache = new WeakMap();There was a problem hiding this comment.
I'm the wrong person to ask about naming stuff
mcsf
force-pushed
the
fix/patterns-prevent-recursion
branch
from
6434535 to
982591d
Compare
mcsf
force-pushed
the
fix/patterns-prevent-recursion
branch
from
982591d to
d09b7c9
Compare
youknowriad
reviewed
Dec 15, 2023
| ); | ||
| await expect( warning ).toBeVisible(); | ||
| } ); | ||
| } ); |
There was a problem hiding this comment.
This reads well. Nice test.
bph
added
the
[Feature] Patterns
artemiomorales
pushed a commit
that referenced
this pull request
Jan 4, 2024
…#56511) * Prevent infinite loops due to recursive patterns * Add tests for cycle detection * Add disclaimer about detection method * Add E2E guarding against pattern recursion
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.
Partially fixes #56455.
What?
If a pattern directly or indirectly contains itself, detect this circularity and prevent the editor from falling into an infinite loop trying to render the recursive blocks.
Such malformed patterns can either be referencing themselves directly:
or they could be doing so indirectly, through circular dependencies:
Why?
Other special blocks have already had to deal with this problem: Post Content, Template Parts and Synced Patterns (formerly Reusable Blocks). The approach was two-pronged:
useHasRecursionand its accompanying provider,RecursionProvider.However, non-synced patterns are a different beast, because this block,
wp:pattern, is a virtual block that replaces itself in the editor canvas with the contents of the pattern. This means that:@wordpress/block-editor, there is nowhere to inject an instance ofRecursionProviderthat will stick;I have explored a few different approaches, none of which pleased me much.
How?
This approach relies on the fact that patterns, regardless of where they are rendered, all bear a unique identifier (attributes.slug). This assumption allows the Pattern block to keep a singleton registry of patterns that tracks their dependency on other patterns.The Pattern block's
recursion-detectormodule keeps a "registry of patterns" as aMapthat tracks their dependency on other patterns. Thus, we have the means to detect circular dependencies between patterns and prevent infinite recursion.In order to gracefully support multiple editing contexts,
useRegistryis used internally to keep track of the different dependency maps depending on the context.To do
X-> Template PartY-> PatternX. Are these a dealbreaker for this approach, or can we expand the registry to accommodate them?Caveats
This approach can be described as preemptive or analytical, whereas the ideal approach would rely on the actual rendering stack. That means that, if some rogue pattern manages to fool the detector, there is no reactive system to stop the recursion. I don't like that.
Testing Instructions
Mutual dependency between pattern A and B
There are other ways to test, but below is what I have been doing. It's easier to work with the theme's pattern rather than locally saved patterns, because the former cannot be automatically overwritten by the editor's Pattern block.
templates/404.htmland add the following line:<!-- wp:pattern {"slug":"evil/a"} /-->gutenberg.phpfile:Other scenarios
Please test extensively and imaginatively. Other combinations:
Testing Instructions for Keyboard
Screenshots or screencast