Block inserter: prevent editor from crashing if blockType.parent is a string
#66234
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
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
oandregal
changed the title
parent is a stringblockType.parent is a string
oandregal
requested review from
youknowriad and
t-hamano
and removed request for
ellatrix
oandregal
added
[Type] Bug
|
Size Change: +26 B (0%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
|
@youknowriad does this need to be part of WordPress 6.7 as well? Given the timeline, I presume #65700 wasn't part of it? |
|
Flaky tests detected in 2ec7a03. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11402623235
|
youknowriad
reviewed
Oct 18, 2024
| @@ -1595,7 +1595,7 @@ const isBlockVisibleInTheInserter = ( | |||
| checkedBlocks.add( blockName ); | |||
|
|
|||
| // If parent blocks are not visible, child blocks should be hidden too. | |||
| if ( !! blockType.parent?.length ) { | |||
| if ( Array.isArray( blockType?.parent ) ) { | |||
There was a problem hiding this comment.
I think we should cast to array instead, otherwise, the check won't be applied if there's a single string parent.
There was a problem hiding this comment.
Can parent be a string? It doesn't seem so according to the docs. Not knowing all the details of this, I'm a bit reluctant to cast the type.
There was a problem hiding this comment.
I doubt it would work with a single string value correctly.
In the block.json schema there is also an array enforced:
gutenberg/schemas/json/block.json
Line 65 in fa12080
There was a problem hiding this comment.
After reading a bit. I’m inclined to follow the advice from @youknowriad and cast string values to array to make it backward compatible if there are existing blocks that used the string during the block type’s validation. It happens at the end of this file:
|
No need to include in 6.7 it seems. |
|
@youknowriad @gziolo how is this one looking? anything else to do? |
oandregal
force-pushed
the
fix/issue-with-parent-some
branch
from
a6425e6 to
91c2aad
Compare
youknowriad
reviewed
Oct 21, 2024
| blockType.parent instanceof String | ||
| ? [ blockType.parent ] | ||
| : blockType.parent; | ||
| if ( Array.isArray( parent ) ) { |
There was a problem hiding this comment.
Is this condition still necessary. I guess right now it's more if ( !! parent )
There was a problem hiding this comment.
I'm curious why we'd need both typeof blockType.parent === 'string' || blockType.parent instanceof String.. 🤔
There was a problem hiding this comment.
I'm curious why we'd need both typeof blockType.parent === 'string' || blockType.parent instanceof String..
Otherwise, we don't cover all potential issues. Note the issue was that we checked for .length in a type that wasn't an array (didn't have .some). That's the case for both literal & object strings.
There was a problem hiding this comment.
It's interesting, but it's possible to pass some constructs like:
I don't think it would ever happen, but the check will cover it.
youknowriad
approved these changes
Oct 21, 2024
karthick-murugan
pushed a commit
to karthick-murugan/gutenberg
that referenced
this pull request
Nov 13, 2024
… a string (WordPress#66234) Co-authored-by: oandregal <oandregal@git.wordpress.org> Co-authored-by: youknowriad <youknowriad@git.wordpress.org> Co-authored-by: gziolo <gziolo@git.wordpress.org>
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.
Potential fix for https://wordpress.org/support/topic/error-after-update-228/
What?
Fixes an error when blocks'
parentproperty is a string.Why?
In the forum, I ran into a report where users have the editor crash when trying to open the inserter. Environment conditions are WordPress 6.6.2 and Gutenberg 15.6. The first error hints at
TypeError: n.parent.some is not a function at ....Investigating a bit, it seems related to #65700 The report is recent (matches the Gutenberg version where that PR was introduced), the users report issues when inserting blocks, and there's no other place in the codebase that uses
parent.some. See conversation.While reproducing the exact user conditions is difficult (e.g.: asking them what 3rd party block libraries do they have, etc.), we can be more explicit about the check.
How?
Update the check for the parent so it verifies it's the type we want it to be: a non-empty array.
Testing Instructions
The best I could come up with is the following:
Using
trunk, the editor would crash. With this PR, it doesn't.Further work
As suggested in https://github.com/WordPress/gutenberg/pull/65700/files#r1806317308 we should look at enforcing the
blockType.parentproperty being an array if that is its only valid value. Given how long we haven't enforced it, doing that now may be side-effects and requires a longer investigation and testing period than this approach — hence why I prioritized preparing this hotfix.