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

Introduce WP_Block and WP_Block_Factory PHP classes and remove globals #1321

Closed
felixarntz opened this issue Jun 20, 2017 · 6 comments
Closed

Comments

@felixarntz
Copy link
Member

At the moment the functions register_block_type() and unregister_block_type() simply store block arguments as an array of settings inside a global. This is an outdated practice and was recently improved in other similar areas of core. In particular, no new globals should be introduced.

Instead I'm suggesting the following:

  • Introduce a WP_Block class that represents a block. It should contain properties for the settings and also handle its own validation. This class can be implemented after other model classes like WP_Post_Type and WP_Taxonomy.
  • Introduce a WP_Block_Factory class that contains methods for registering, unregistering and retrieving blocks. The existing functions register_block_type() and unregister_block_type() could then become wrappers. This class allows us to get rid of the $wp_registered_blocks global and store the registered blocks in a class property. Since we at this point do not have a central service locator available in WordPress, let's store the canonical main instance of WP_Block_Factory in a static variable and make it available through a static get_instance() method.

I'll gladly provide the necessary code through a PR.

Beside the technical benefits, the above changes will also make it easier to work with blocks because they would then have proper documentation about their properties and functionality.

@westonruter
Copy link
Member

@felixarntz
Copy link
Member Author

@westonruter Thanks for pointing me there! Am I understanding correctly though that there is no PR for the classes yet?

Btw I agree that WP_Block_Type and WP_Block_Type_Registry are more suitable names for the classes. In particular the classes should carry the term "Block Type" instead of "Block", as that is what they actually represent.

@nylen
Copy link
Member

nylen commented Jun 20, 2017

Am I understanding correctly though that there is no PR for the classes yet?

Yes. Feel free to ping me for review on this one.

@westonruter
Copy link
Member

westonruter commented Jun 20, 2017

There is no existing PR, no, other than some PHP functions for managing block type assets (#1217).

And yes, WP_Block_Type_Registry could be a collection of WP_Block_Type instances, whereas there could then there could perhaps be WP_Block that represents a given instance of a type? For example, in addition to the current do_blocks() function which is a filter for the_content, I imagine there should also be a parse_blocks() which returns a list of all the blocks in the content, which could then be listed out in the REST API for example.

There are a couple other patterns in WordPress that come to mind that do things differently than the above. In the Customizer, you can register a control type via WP_Customize_Manager::register_control_type() and what you pass is the string for a class name that extends WP_Customize_Control. This has not been ideal because it does not facilitate dependency injection or dynamic control types.

Another existing pattern in WordPress is WP_Widget itself, which I consider the spiritual ancestor of blocks. There is a WP_Widget_Factory that manages the instances of the widget types, and subclasses of WP_Widget each represent a type of the widget. There is only ever one instance of each WP_Widget subclass, even though there may be any number of occurrences of a widget type on a site (e.g. 10 text widgets in a sidebar but only one instance of WP_Widget_Text). In this case, the underlying instance data is passed as props to the single PHP widget object. This works pretty well, but it also seems somewhat strange that you have to pass in the props for each instance in a widget instance, though then again, this is functional programming.

The register_widget() function allows you to pass a class name for a WP_Widget subclass or an instance of such a class.

@felixarntz
Copy link
Member Author

Due to the legacy implications of WP_Widget I'd prefer to not take this as example, although it is conceptually indeed close to a block type. WP_Post_Type is a better example and is in so far related, that it also represents a specific type of items which then themselves have specific functionality and should implement a class, so I agree that it could also be valuable to implement a WP_Block class here (which would be the equivalent of WP_Post for the above).

The lack of a distinction between an "entity" and "entity type" which WP_Widget does not really make (it represents an "entity type", but also contains functions to manage its own "entities") is the most important reason I consider WP_Widget a bad example for blocks and block types. A block should be accessible by itself and not only make sense when passed to its specific block type methods.
A positive aspect there however is that there is a WP_Widget_Factory class while posts don't currently have such a managing class and are simply stored in a global. I think the terminology of WP_Block_Type_Registry is a more suitable name than WP_Block_Type_Factory though.

@mtias
Copy link
Member

mtias commented Aug 18, 2017

Closing as implemented.

@mtias mtias closed this as completed Aug 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants