Isn't the parent prop in metadata (if an object is passed)? If that's the case and the metadata are not passed we should get them and make the check.

Additionally can we simplify the checks here to explicitly check what we want? Something like:

if (
	blockNameOrMetadata.parent &&
	! Array.isArray( blockNameOrMetadata.parent ) &&
	typeof blockNameOrMetadata.parent === 'string'
) {
	blockNameOrMetadata.parent = [ blockNameOrMetadata.parent ];
}

Isn't the parent prop in metadata (if an object is passed)? If that's the case and the metadata are not passed we should get them and make the check.

Oh, I missed that, thanks for bringing it up! I'm a bit confused about this for some reasons (existing code, docs and other implementations tell otherwise), but specially because the PR worked in my tests.

Additionally can we simplify the checks here to explicitly check what we want?

What we want to do is "if the parent is a string, normalize it to an array". In my view, the current code is actually more explicit, simpler (why to check both "is it not an array" and "it is a literal string?"), and more comprehensive (String objects).

So, we actually have to use settings, just not in registerBlockType but in processBlockType. This is the data flow for block registration:

• registerBlockType( nameOrMetadata, settings )
  • addBootstrappedBlockType => if metadata provided, creates a new tree in the store with that data
  • addUnprocessedBlockType
    • calls processBlockType => takes the existing block metadata & merges it with the settings
      • merges metadata & settings into blockType
      • dispatch 'blocks.registerBlockType' filter with blockType
      • validates the blockType after the filter has been dispatched
    • creates a new tree in the store with the blockType data
  • return blockType (settings & metadata consolidated)

I've moved it all to processBlockType (including existing validation for the parent that was done too early) in 002ac7cec3337f5f5d56c9f154a3e337b890f3c4.

After having looked at this more deeply, there's a situation in which this could be considered a breaking change:

• 3rd party code registers the blockType with parent as a string.
  • Before: parent will be a string in the store.
  • After: parent will be an array in the store.

Note that I intentionally added the normalization code after the filter, as to not interfere with 3rd party code before they executed their callback. However, if the 3rd party code queries the store for the parent after registration to do something with the parent, it will no longer be a string.

I don't see a way out of this and it seems to me that this PR fixes an important issue (being able to add a child block within itself). This PR will also be merged very early in the WordPress 6.8 cycle, so we'll have time to gather feedback.

Thoughts?

which this could be considered a breaking change

That's true, but I'm not sure if we should consider a breaking change something that was used in a wrong way and differently than documentation. We had a similar issue with description in the past, where some plugins would provide a component and we handled that the same way as you did (with a small difference deprecated vs warning).

Additionally, it sounds unusual to use parent from store and this PR seems good to me regarding this aspect.

As long as we log the offence, I'm ok with this. Any third-party logic interested in monitoring blocks' parent setting would quickly notice that core blocks use arrays, unless that logic were really built very specifically for a third-party block using parent as a string. It's possible that someone is doing that, but not too likely.

We can play it safe and communicate this as a breaking change in dev notes, even though it isn't.

We can play it safe and communicate this as a breaking change in dev notes, even though it isn't.

Added the label and some text for the devnote.

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.

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?

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

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.

Valid point!