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

Block registration: normalize blockType.parent to array #66250

Merged
merged 13 commits into from
Oct 24, 2024
14 changes: 2 additions & 12 deletions packages/block-editor/src/store/selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -1628,18 +1628,8 @@ const isBlockVisibleInTheInserter = (
checkedBlocks.add( blockName );

// If parent blocks are not visible, child blocks should be hidden too.
//
// In some scenarios, blockType.parent may be a string.
// A better approach would be sanitize parent in all the places that can be modified:
// block registration, processBlockType, filters, etc.
// In the meantime, this is a hotfix to prevent the editor from crashing.
const parent =
typeof blockType.parent === 'string' ||
blockType.parent instanceof String
? [ blockType.parent ]
: blockType.parent;
if ( Array.isArray( parent ) ) {
return parent.some(
if ( Array.isArray( blockType.parent ) ) {
return blockType.parent.some(
oandregal marked this conversation as resolved.
Show resolved Hide resolved
( name ) =>
( blockName !== name &&
isBlockVisibleInTheInserter(
Expand Down
4 changes: 4 additions & 0 deletions packages/blocks/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

## Unreleased

### Breaking changes

- Normalize `blockType.parent` to be an array. While string values were never supported, they appeared to work with some unintended side-effects that have been fixed by [#66250](https://github.com/WordPress/gutenberg/pull/66250). For that reason, we've added some code that automatically migrates strings to arrays — though it still raises a warning.

## 13.10.0 (2024-10-16)

## 13.9.0 (2024-10-03)
Expand Down
9 changes: 0 additions & 9 deletions packages/blocks/src/api/registration.js
Original file line number Diff line number Diff line change
Expand Up @@ -232,15 +232,6 @@ export function registerBlockType( blockNameOrMetadata, settings ) {
return;
}

if ( 1 === settings?.parent?.length && name === settings.parent[ 0 ] ) {
warning(
'Block "' +
name +
'" cannot be a parent of itself. Please remove the block name from the parent list.'
);
return;
}

if ( ! /^[a-z][a-z0-9-]*\/[a-z][a-z0-9-]*$/.test( name ) ) {
warning(
'Block names must contain a namespace prefix, include only lowercase alphanumeric characters or dashes, and start with a letter. Example: my-plugin/my-custom-block'
Expand Down
33 changes: 33 additions & 0 deletions packages/blocks/src/api/test/registration.js
Original file line number Diff line number Diff line change
Expand Up @@ -742,6 +742,39 @@ describe( 'blocks', () => {
} );
} );

it( 'should transform parent string to array', () => {
const blockType = {
save: noop,
category: 'text',
title: 'block title',
parent: 'core/paragraph',
};
const block = registerBlockType(
'core/test-block-parent-string',
blockType
);
expect( console ).toHaveWarnedWith(
'Parent must be undefined or an array of strings (block types), but it is a string.'
);
expect( block ).toEqual( {
name: 'core/test-block-parent-string',
save: noop,
category: 'text',
title: 'block title',
icon: { src: BLOCK_ICON_DEFAULT },
attributes: {},
providesContext: {},
usesContext: [],
keywords: [],
selectors: {},
supports: {},
styles: [],
variations: [],
blockHooks: {},
parent: [ 'core/paragraph' ],
} );
} );

describe( 'applyFilters', () => {
afterEach( () => {
removeAllFilters( 'blocks.registerBlockType' );
Expand Down
36 changes: 36 additions & 0 deletions packages/blocks/src/store/process-block-type.js
Original file line number Diff line number Diff line change
Expand Up @@ -197,5 +197,41 @@ export const processBlockType =
return;
}

if (
typeof settings?.parent === 'string' ||
Copy link
Contributor

Choose a reason for hiding this comment

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

Copied the comment from the other PR:

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.

I still don't see why is needed. Can you give me an example?

In general my suggestion before for simplifying the checks by what we expect and not what we don't expect. With that code parent could be a boolean etc..

And if we wanted to be thorough we should also check if an array is provided, that consists of strings.

Copy link
Member Author

@oandregal oandregal Oct 22, 2024

Choose a reason for hiding this comment

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

Ok, these are the facts:

  • I discovered that we were trying to detect arrays the wrong way while responding to Gutenberg forum issues. We were checking for blockType.parent?.length, but that's not enough to detect arrays — it can be a string, any type of it, as it was. I prepared a hotfix but wanted to tackle the core issue: that we didn't normalize the parent data coming from 3rd parties — that's what this PR is for. I presume we have more of these in our codebase are causing issues for the same reason.

  • Strings can be literal or object. Both have the .length property and are boxed/unboxed depending on what you do. So

console.log( 'string'.length ); // Logs 6
console.log( new String( 'string' ).length ); // Logs 6

log the same.

Back to our problem. Block authors should have been using arrays to declare the parent, but, in some cases, they weren't — instead they used strings of any type. They can use either type, that's why we need to cover for both.

Does this help?

settings?.parent instanceof String
) {
settings.parent = [ settings.parent ];
warning(
'Parent must be undefined or an array of strings (block types), but it is a string.'
);
// Intentionally continue:
//
// While string values were never supported, they appeared to work with some unintended side-effects
// that have been fixed by [#66250](https://github.com/WordPress/gutenberg/pull/66250).
//
// To be backwards-compatible, this code that automatically migrates strings to arrays.
}

if (
! Array.isArray( settings?.parent ) &&
settings?.parent !== undefined
) {
warning(
'Parent must be undefined or an array of block types, but it is ',
settings.parent
);
return;
}

if ( 1 === settings?.parent?.length && name === settings.parent[ 0 ] ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand this check was carried from before, but why the check for 1? Wouldn't the following be better in all respects?

Suggested change
if ( 1 === settings?.parent?.length && name === settings.parent[ 0 ] ) {
if ( settings?.parent?.includes?.( name ) ) {

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure and it increases the surface area of this PR that we need to test, etc. — I'd rather do that separately. For reference, this is the issue and PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Valid point!

warning(
'Block "' +
name +
'" cannot be a parent of itself. Please remove the block name from the parent list.'
);
return;
}

return settings;
};
Loading