Block API: Normalize function arguments

[gziolo]

Description

Closes #5227.

Based on proposal from @matias:

We have several functions that take arguments like blockType, attributes, innerHtml, but the order is not always consistent.

Since this is an often repeated pattern, we should consolidate usage.

Examples:

isValidBlock — innerHTML, blockType, attributes.
getBlockAttributes — blockType, innerHTML, attributes.
getAttributesFromDeprecatedVersion — blockType, innerHTML, attributes.
createBlock — name, attributes, innerBlocks.
getSaveContent — blockType, attributes.

I did the following refactorings:

• isValidBlock( innerHTML, blockType, attributes ) renamed to isBlockContentValid( blockType, attributes, innerHTML ).
• createBlock( name, blockAttributes = {}, innerBlocks = [] ) became createBlock( name, attributes = {}, innerBlocks = [] ).
• Added default value to the last param in getBlockAttributes( blockType, innerHTML, attributes = {} ) - this one is tricky as it often is used with only 2 params. We should discuss if it really makes sense to swap two last params.
• getCommentAttributes( allAttributes, blockType ) is now getCommentAttributes( blockType, attributes ) - this is internal function.
• getAttributesFromDeprecatedVersion from the list above no longer exists - no action required.
• Did some code cleanup whenever I discovered something suspicious.

It still differs from @aduth shared which I wanted to follow initially:

isValidBlock( name, attributes, innerHTML )
getBlockAttributes( name, attributes, innerHTML )
getAttributesFromDeprecatedVersion( name, attributes, innerHTML )
createBlock( name, attributes, innerBlocks )
getSaveContent( name, attributes, innerBlocks )

However after some time spent exploring the codebase, I find the fact that we use blockType very convinient in the context of unit tests. Should we allow using both string and object instead to leave the codebase unchanged but offer some more flexibility?

How has this been tested?

npm run test & npm run test-e2e

Types of changes

New deprecation is introduced.

Refactoring.

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.

[ellatrix]

I generally prefer using an object with named arguments if there is more than one argument. E.g. for createBlock it would be:

createBlock( {
  name,
  attributes = {},
  innerBlocks = [],
} )

The benefits are that the arguments are clearly named, the order doesn't matter, and you can easily omit optional arguments.

[aduth]

My main concern is consistency in argument ordering of the existing functions, and in how they can be called.

These are flagrantly inconsistent from other functions, for example:

isValidBlock — innerHTML, blockType, attributes
getAttributesFromDeprecatedVersion — blockType, innerHTML, attributes

On the point of name vs. type as an argument; I'd actually be okay with allowing the convenience here of specifying one or the other, as long as it's supported consistently across all functions. Maybe we need an internal function to normalize?

The ability to provide as an argument the type object implies that one might be able to call it a definition which is not actually registered. We should determine whether that's something we want, and if it even works at all.

I generally prefer using an object with named arguments if there is more than one argument.

I think there are pros and cons. I don't know that this is something we really want to change at this point though.

For posterity's sake, the original implementation may have been partly inspired by React's interface for createElement.

[aduth]

Nit: We have some precedent of the adjective coming first in boolean "is" functions, e.g. isReusableBlock, so for consistency's sake, isValidBlockContent might be the more consistent ordering.

[gziolo]

I see also isValidElement, let's rename. 👍

[aduth]

Why should we update this, but not unregisterBlockType( name ) -> unregisterBlockType( blockName ). What's the motivation for this change?

[gziolo]

I will revert. It's obvious here what the name is. Other examples where I changed bring more clarity as the function name isn't as specific. e.g.: setFreeformContentHandlerName( blockName ).

[gziolo]

I didn't understand your question properly, but I think I answered it anyway :)

It's obvious here what the name is.

name is also used in registerBlockType as the first param - so I wanted to have it consistent. In general I expanded to blockName in cases where the funcion name doesn't say anything about the block.

[aduth]

Maybe a hint to the changed argument order?

[aduth]

4.4 now?

[gziolo]

Yes, true.