Introduce proper PHP classes for interacting with block types

[felixarntz]

This is a PR for #1321.

It currently does the following:

• Introduce a very basic WP_Block_Type class that follows the models of other core classes for entity types, such as WP_Post_Type and WP_Taxonomy.
• Introduce a WP_Block_Type_Registry class as a container for WP_Block_Type objects, including methods to register, unregister and retrieve block types. A static get_instance() method is provided for access to the main class instance.
• The functions register_block_type() and unregister_block_type() are now wrappers for methods of the WP_Block_Type_Registry class. The do_blocks() function has been adjusted to use that class as well. The previous $wp_registered_blocks global no longer exists.
• In the warning messages when registering or unregistering block types, it changes the terminology from "Block" to "Block Type" because that is what the developer is actually dealing with.

Further ideas/observations:

• At the moment there is no mechanism in WP_Block_Type or WP_Block_Type_Registry to ensure the render argument is set. We could either continue to have it optional and introduce a check whether it is set, or alternatively require it and not register a block type without the argument. I think I'm leaning towards leaving it optional because otherwise it should not be part of $args, but a required parameter.
• It may be useful to also introduce a WP_Block class that represents an actual block parsed from the content. I'd rather discuss this in a separate issue / PR though.
• Instead of using _doing_it_wrong() it might be a better idea to return a WP_Error object if something goes wrong when registering or unregistering a block type.
• The doc blocks for parse_block_attributes() and do_blocks() do not follow the WordPress documentation standards. We can fix this here, but it could also be part of another PR if that's preferred.

[felixarntz]

I've not yet adjusted unit tests to the changes. Will do that later this week.

[westonruter]

What's the use case for this?

[felixarntz]

I liked the idea of opening up block types for extensions. A similar filter is also available when registering post types, and it could be used here to add custom block type arguments that could be pre-populated by other default arguments.

If you think we should leave this out for now, I'm not opposed. It's probably too early to figure out a specific use-case where this can come in handy.

[westonruter]

I think it would be better to only add the filter when we know we need it, rather than add filters up front when we may not know that they will ever be used.

[westonruter]

This will set props even if they are undeclared. Would that be desired? Should something like this be done instead: https://github.com/WordPress/wordpress-develop/blob/4.8.0/src/wp-includes/class-wp-customize-setting.php#L179-L184

[felixarntz]

I think it should, in order to allow setting properties that are unsupported by core, but possibly are by plugins doing something custom with Gutenberg. WP_Post_Type and WP_Taxonomy work the same way, and it has proven to make sense there.

[westonruter]

In that case would it not be better to allow WP_Block_Type to be extensible so that the subclasses could be passed to the register method as opposed to just the args? This would be similar to how WP_Customize_Manager::add_control() allows you to pass the ID and args, or just a previously instantiated WP_Customize_Control.

[felixarntz]

That might be a better idea, but I'm not entirely sure. Allowing all $args to be set as property especially makes sense with a filter such as register_block_type_args (which per the above we're gonna remove for now, but might re-add it again at some point).

I like the idea of passing a custom instance directly (to support sub-classes), but supporting any $args key to be set as property makes it possible (in collaboration with a filter) to provide extension points that could affect all block types, regardless of their class. Let's wait for more opinions on this, I would say.

[westonruter]

Is this going to facilitate testing since the instance can't be cleared?

[felixarntz]

I think testing should be possible as we could manually spin up instances of the WP_Block_Type_Registry class for most test cases, as we wouldn't need to use the main instance. Once tests have been added, we'll know for sure.

[BE-Webdesign]

Hi @felixarntz,

Great work so far! I like that we are moving towards a more class based approach, and it seems to be moving quick! A quick suggestion: it could be an interesting approach to change the WP_Block_Type_Registry->register() signature from ( $name, $args ) to just ( $block_type ), then the registry could act as more of a collection of the blocks that are being registered. This way the registry is not concerned about what a block is and more with whether it has been registered or not. This makes adding a helper method of WP_Block_Type_Registry->register_block_types() easy to implement as well. ArrayAccess could also potentially be used instead of WP_Block_Type_Registry->registered_block_types, the only caveat being that ArrayAccess is finicky in PHP. Let me know your thoughts!

[westonruter]

When introducing a WP_Block it should look the same as blocks parsed in JS, with the attributes, content, and children.

[felixarntz]

@BE-Webdesign I generally like the idea of providing a block type in register_block_type() (and WP_Block_Type_Registry::register()), however it would be a bit confusing to require a WP_Block_Type there while requiring a block type name for the unregistering method. Not heavily against this, but it could be confusing to WordPress developers who're used to other core APIs.

Given that we're not used to this behavior of functions, I rather suggest simply supporting a WP_Block_Type instance being passed in the registration method (instead of requiring it) - basically if ( is_a( $name, 'WP_Block_Type' ) ) .... This gives us the best of both worlds.

Supporting registration of multiple block types in one might be useful, we could add a simple wrapper for the singular registration method to support this. Not entirely sure if we need to have this though.

@westonruter That makes sense. As said before, I'd prefer if we worked on this in another PR after this one is ready.

[GaryJones]

What benefit is there to returning a WP_Block_Type object just after it has been unregistered?
In case someone wants to amend it and re-register it?

[westonruter]

It's a shortcut to avoid having to do a separate getter call, yes. This was also implemented in the Customizer for calls like WP_Customize_Manager::add_setting(). Previously it returned void but not it returns the WP_Customize_Setting instance which is particularly useful if you provided an $args array instead of a subclassed instance of WP_Customize_Setting. See https://core.trac.wordpress.org/ticket/34596

[westonruter]

Oh, woops. I missed that this is in the unregister call. Nevertheless, seems useful to return. As essentially if false is the return value then nothing was unregistered, whereas if it is truthy (an instance of WP_Block_Type then the unregistration was successful.

[GaryJones]

I get the value of something truthy being returned to indicate a successful unregister, but why not true? i.e. what could a developer be reasonably expected to do with this object (other than tweak and re-register)? Is there any drawback compared to just returning true? Do other situations in WP return something more than a boolean when something has been unregistered?

[felixarntz]

I think it's a good practice to return the data that has been removed/unregistered, to allow further actions with it, for example tweaking and re-registering or passing it to a custom hook.

It's a bit similar to how several PHP array functions work like array_shift(), array_pop() or array_splice(): The moment you remove something, also return what has been removed on success.

[GaryJones]

Typo: unregistered => registered.

[felixarntz]

Fixed in a408132

[felixarntz]

In ecc6dbb, I added a new test class specific to methods of WP_Block_Type_Registry which we can test unaware of WordPress' global state. Most of the tests in there were previously part of the Registration_Test class and have been migrated over to using the new class methods directly. The Registration_Test class now only contains tests to ensure that the register_block_type() and unregister_block_type() functions properly wrap the respective methods of the WP_Block_Type_Registry main instance.

In the Dynamic_Blocks_Render_Test I just made a few tweaks to adjust to the new structure and added a missing call to parent::tearDown() which caused unexpected test behavior.

[felixarntz]

What's left here? Should we add support for passing a WP_Block_Type to the registration method in this PR, or should it be discussed separately afterwards? And do we want to have more methods and argument validation in WP_Block_Type?

[GaryJones]

Instead of having mixed return types, is there an advantage to returning an instance of a WP_Null_Block_Type (not yet defined)?

[felixarntz]

That is a good question. Generally returning something like you say (implementing a general block type interface) makes sense, however these practices have never been followed by WordPress core. I'm wary of introducing such a pattern here as it contradicts of what people are used from working with core. I think such a topic belongs into a discussion of a bigger scope, which I'd love to have a working group for at some point (we had a similar discussion at WCEU).

[westonruter]

Should we add support for passing a WP_Block_Type to the registration method in this PR, or should it be discussed separately afterwards? And do we want to have more methods and argument validation in WP_Block_Type?

I think passing subclasses of WP_Block_Type should be supported up front.

[felixarntz]

d45da39 fixes the merge conflicts that have come up with recent Gutenberg upstream changes.

547b89b introduces a WP_Block_Type::render() method. This provides a clear separation of concerns and allows overriding the method directly if necessary (for example in a WP_Block_Type sub-class). Furthermore the method now handles the case properly that a render argument is not provided, which would previously result in a fatal error. It now returns the raw content instead, which also fixes #1631 as it makes that data available to the actual render callback as well. Several unit tests for that functionality as well as for the set_props() method have been added as well.

[felixarntz]

ca02a61 adds support for passing a WP_Block_Type instance (or sub-class) to register_block_type(). When the function is used like that, the second parameter will be ignored. Documentation has been adjusted accordingly, and a unit test has been added.

I think once these changes have been reviewed and accepted, the PR should be good. I'd prefer if we work on further enhancements to the new infrastructure through separate issues / PRs.

[westonruter]

This should perhaps be called render_callback to help disambiguate from render the method. This is similar to what WP_Customize_Control does with its active method and active_callback member variable.

See https://github.com/WordPress/wordpress-develop/blob/a16d16449ddee6a3c13a65fd02536a6071c04d70/src/wp-includes/class-wp-customize-control.php#L271-L273

[felixarntz]

6b3ec26 renames the render property to render_callback.

[westonruter]

For some more thoughts on how the PHP API should look, see https://twitter.com/Josh412/status/883061703004631041

[felixarntz]

@westonruter I don't think it makes sense to suddenly start using such patterns in a feature project, as part of a relatively unrelated PR.
I certainly prefer the approach @Shelob9 suggests, but as much as I don't like this argument this is not the current "WordPress way". I'm all in for discussing this as a general best practice to enforce and require on future code, but that should happen in a broader scope, for example in the new WordPress PHP Slack channel.

In short: Introducing a WordPress container for a random PR in Gutenberg seems wrong to me. Introducing it after a more thorough general discussion, not connected to a specific code change, makes much more sense I think.

[coveralls]

Coverage remained the same at 17.816% when pulling d45a92a on felixarntz:add/1321-block-classes into d157b5e on WordPress:master.

[felixarntz]

@westonruter What is left to do here to get ready for merge?

[westonruter]

Probably a 👍 from @mtias.

Note that I was suggesting a similar in the JS API, but it was not endorsed: #362 (comment)

[felixarntz]

I'm by far not a JS expert, but there are probably reasons for it not to work the same as in PHP. All I know is that in PHP these classes are a much better practice than globals and random arrays. :)

@mtias What are your thoughts? Any objections, suggestions?

[nylen]

Let's merge this soon; thank you @felixarntz for staying on top of it. The existing global variable was written because it was the quickest way to get the code working; this approach is much better.

Regarding differences between JS and PHP APIs: I think the main benefit of this approach is encapsulation, which ES modules also give us. PHP and JS (especially ES6) are very different languages, and these differences should imply different coding styles.

It looks like there are a few minor things that need to be updated. I'll leave review comments.

[nylen]

Like register does, should this function also accept a WP_Block_Type object?

[nylen]

Should this accept a WP_Block_Type instance like register_block_type does?

[nylen]

Should be @since 0.6.0.

[nylen]

Should be @since 0.6.0.

[nylen]

Should be @since 0.6.0.

[nylen]

Should be @since 0.6.0.

[nylen]

Should be @since 0.6.0.

[nylen]

Should be @since 0.6.0.

[nylen]

Should be @since 0.6.0.

[nylen]

Should be @since 0.6.0.

[mtias]

I haven't had a chance to read this one with attention, but I hope to get to it in the next couple days.

[felixarntz]

@nylen Will make the version number changes later today!

[nylen]

👍 There was one other change in there too, about letting the unregister functions accept a WP_Block_Type instance as well. I thought it might be helpful to have each version number marked so the feedback will disappear when updated, but maybe that was a bit overkill.

[mtias]

Thanks for working on this one. With further structure in place, it begins to highlight a drift between editor-client and back-end regarding which blocks are registered or available at any given point in time (which is also not absolute) that may require us thinking at a bit higher level. In a way, behaviour is ok to be different since they are entirely separate paradigms, but presence may require us to look at a more abstracted way of defining a block—say, a json file with minimal configuration like block-type name, title, etc, that specifies entry paths for client and back-end files.

That may become more relevant if we want to have access to a reliable list of potentially available blocks on the back-end for something like storing a default block in Settings → Writing, or for constructing PHP templates declaring blocks that are static (meaning they don't have a representation on the back-end). But equally not sure if this is needed in any case, as there are ways to handle it.

In any case, let's merge this once the version number changes are in.

[felixarntz]

@nylen I updated the version numbers and also added support passing a WP_Block_Type to unregister_block_type().

[westonruter]

a more abstracted way of defining a block—say, a json file with minimal configuration like block-type name, title, etc, that specifies entry paths for client and back-end files.

This could tie in with using JSON schema to describe block attributes: #1905