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 proper PHP classes for interacting with block types #1322

Merged
merged 20 commits into from
Jul 26, 2017
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
ce45293
Introduce WP_Block_Type and WP_Block_Type_Registry classes.
Jun 20, 2017
d213309
Use WP_Block_Type_Registry class to manage registered block types.
Jun 20, 2017
9aa5fde
Use proper Block Type terminology in warnings instead of simply Block.
Jun 20, 2017
a84b787
Adjust version numbers for WP_Block_Type and WP_Block_Type_Registry c…
Jun 20, 2017
8e379bf
Remove register_block_type_args filter.
Jun 20, 2017
a408132
Fix typo in WP_Block_Type_Registry::get_registered() docs.
Jun 23, 2017
6542555
Use correct __METHOD__ constant instead of __FUNCTION__ in WP_Block_T…
Jun 23, 2017
e0539c7
Implement method to get all registered block types.
Jun 23, 2017
ecc6dbb
Add tests for WP_Block_Type_Registry and adjust existing tests to work.
Jun 23, 2017
c8e1464
Add missing doc comment for block type registry test property.
Jun 23, 2017
d45da39
Merge remote-tracking branch 'upstream/master' into add/1321-block-cl…
Jul 2, 2017
547b89b
Introduce WP_Block_Type::render() method and support passing block at…
Jul 2, 2017
75a065a
Fix coding standards.
Jul 2, 2017
ca02a61
Add support for passing a WP_Block_Type instance to register_block_ty…
Jul 2, 2017
11c47bc
Adjust version numbers.
Jul 2, 2017
6b3ec26
Rename the WP_Block_Type render property to render_callback.
Jul 3, 2017
d45a92a
Merge remote-tracking branch 'upstream/master' into add/1321-block-cl…
Jul 10, 2017
c296165
Merge remote-tracking branch 'upstream/master' into add/1321-block-cl…
Jul 26, 2017
b7d1824
Update PHP block type version numbers.
Jul 26, 2017
21d8ee6
Support passing a WP_Block_Type instance to unregister_block_type().
Jul 26, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions gutenberg.php
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
require_once dirname( __FILE__ ) . '/lib/init-checks.php';
if ( gutenberg_can_init() ) {
// Load API functions, register scripts and actions, etc.
require_once dirname( __FILE__ ) . '/lib/class-wp-block-type.php';
require_once dirname( __FILE__ ) . '/lib/class-wp-block-type-registry.php';
require_once dirname( __FILE__ ) . '/lib/blocks.php';
require_once dirname( __FILE__ ) . '/lib/client-assets.php';
require_once dirname( __FILE__ ) . '/lib/i18n.php';
Expand Down
81 changes: 26 additions & 55 deletions lib/blocks.php
Original file line number Diff line number Diff line change
Expand Up @@ -9,64 +9,34 @@
die( 'Silence is golden.' );
}

$wp_registered_blocks = array();

/**
* Registers a block.
* Registers a block type.
*
* @param string $name Block name including namespace.
* @param array $settings Block settings.

* @return array The block, if it has been successfully registered.
* @since 0.1.0
*
* @param string $name Block type name including namespace.
* @param array $args {
* Array of block type arguments. Any arguments may be defined, however the following
* ones are supported by default.
*
* @type callable $render Callback used to render blocks of this block type.
* }
* @return WP_Block_Type|false The registered block type on success, or false on failure.
*/
function register_block_type( $name, $settings ) {
global $wp_registered_blocks;

if ( ! is_string( $name ) ) {
$message = __( 'Block names must be strings.' );
_doing_it_wrong( __FUNCTION__, $message, '0.1.0' );
return false;
}

$name_matcher = '/^[a-z0-9-]+\/[a-z0-9-]+$/';
if ( ! preg_match( $name_matcher, $name ) ) {
$message = __( 'Block names must contain a namespace prefix. Example: my-plugin/my-custom-block' );
_doing_it_wrong( __FUNCTION__, $message, '0.1.0' );
return false;
}

if ( isset( $wp_registered_blocks[ $name ] ) ) {
/* translators: 1: block name */
$message = sprintf( __( 'Block "%s" is already registered.' ), $name );
_doing_it_wrong( __FUNCTION__, $message, '0.1.0' );
return false;
}

$settings['name'] = $name;
$wp_registered_blocks[ $name ] = $settings;

return $settings;
function register_block_type( $name, $args ) {
return WP_Block_Type_Registry::get_instance()->register( $name, $args );
}

/**
* Unregisters a block.
* Unregisters a block type.
*
* @param string $name Block name.
* @return array The previous block value, if it has been
* successfully unregistered; otherwise `null`.
* @since 0.1.0
*
* @param string $name Block type name including namespace.
Copy link
Member

Choose a reason for hiding this comment

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

Should this accept a WP_Block_Type instance like register_block_type does?

* @return WP_Block_Type|false The unregistered block type on success, or false on failure.
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

@GaryJones GaryJones Jun 23, 2017

Choose a reason for hiding this comment

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

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?

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 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.

*/
function unregister_block_type( $name ) {
global $wp_registered_blocks;
if ( ! isset( $wp_registered_blocks[ $name ] ) ) {
/* translators: 1: block name */
$message = sprintf( __( 'Block "%s" is not registered.' ), $name );
_doing_it_wrong( __FUNCTION__, $message, '0.1.0' );
return false;
}
$unregistered_block = $wp_registered_blocks[ $name ];
unset( $wp_registered_blocks[ $name ] );

return $unregistered_block;
return WP_Block_Type_Registry::get_instance()->unregister( $name );
}

/**
Expand Down Expand Up @@ -99,33 +69,34 @@ function parse_block_attributes( $attr_string ) {
* @return string Updated post content.
*/
function do_blocks( $content ) {
global $wp_registered_blocks;
$registry = WP_Block_Type_Registry::get_instance();

// Extract the blocks from the post content.
$matcher = '#' . join( '', array(
'(?P<opener><!--\s*',
'wp:(?P<block_name>[a-z](?:[a-z0-9/]+)*)\s+',
'wp:(?P<block_type_name>[a-z](?:[a-z0-9/]+)*)\s+',
'(?P<attributes>(?:(?!-->).)*)',
'\s*/?-->\n?)',
'(?:',
'(?P<content>.*?)',
'(?P<closer><!--\s*/wp:\g{block_name}\s+-->\n?)',
'(?P<closer><!--\s*/wp:\g{block_type_name}\s+-->\n?)',
')?',
) ) . '#s';
preg_match_all( $matcher, $content, $matches, PREG_OFFSET_CAPTURE );

$new_content = $content;
$offset_differential = 0;
foreach ( $matches[0] as $index => $block_match ) {
$block_name = $matches['block_name'][ $index ][0];
$block_type_name = $matches['block_type_name'][ $index ][0];
$block_type = $registry->get_registered( $block_type_name );

$output = '';
if ( isset( $wp_registered_blocks[ $block_name ] ) ) {
if ( null !== $block_type ) {
$block_attributes_string = $matches['attributes'][ $index ][0];
$block_attributes = parse_block_attributes( $block_attributes_string );

// Call the block's render function to generate the dynamic output.
$output = call_user_func( $wp_registered_blocks[ $block_name ]['render'], $block_attributes );
$output = call_user_func( $block_type->render, $block_attributes );
} elseif ( isset( $matches['content'][ $index ][0] ) ) {
$output = $matches['content'][ $index ][0];
}
Expand Down
148 changes: 148 additions & 0 deletions lib/class-wp-block-type-registry.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,148 @@
<?php
/**
* Blocks API: WP_Block_Type_Registry class
*
* @package gutenberg
* @since 0.2.0
*/

/**
* Core class used for interacting with block types.
*
* @since 0.2.0
*/
final class WP_Block_Type_Registry {
/**
* Registered block types, as `$name => $instance` pairs.
*
* @since 0.2.0
* @access private
* @var array
*/
private $registered_block_types = array();

/**
* Container for the main instance of the class.
*
* @since 0.2.0
* @access private
* @static
* @var WP_Block_Type_Registry|null
*/
private static $instance = null;
Copy link
Member

Choose a reason for hiding this comment

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

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

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 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.


/**
* Registers a block type.
*
* @since 0.2.0
* @access public
*
* @param string $name Block type name including namespace.
* @param array $args {
* Array of block type arguments. Any arguments may be defined, however the following
* ones are supported by default.
*
* @type callable $render Callback used to render blocks of this block type.
* }
* @return WP_Block_Type|false The registered block type on success, or false on failure.
*/
public function register( $name, $args ) {
if ( ! is_string( $name ) ) {
$message = __( 'Block type names must be strings.' );
_doing_it_wrong( __FUNCTION__, $message, '0.1.0' );
return false;
}

$name_matcher = '/^[a-z0-9-]+\/[a-z0-9-]+$/';
if ( ! preg_match( $name_matcher, $name ) ) {
$message = __( 'Block type names must contain a namespace prefix. Example: my-plugin/my-custom-block-type' );
_doing_it_wrong( __FUNCTION__, $message, '0.1.0' );
return false;
}

if ( $this->is_registered( $name ) ) {
/* translators: 1: block name */
$message = sprintf( __( 'Block type "%s" is already registered.' ), $name );
_doing_it_wrong( __FUNCTION__, $message, '0.1.0' );
return false;
}

$block_type = new WP_Block_Type( $name, $args );

$this->registered_block_types[ $name ] = $block_type;

return $block_type;
}

/**
* Unregisters a block type.
*
* @since 0.2.0
* @access public
*
* @param string $name Block type name including namespace.
Copy link
Member

Choose a reason for hiding this comment

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

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

* @return WP_Block_Type|false The unregistered block type on success, or false on failure.
*/
public function unregister( $name ) {
if ( ! $this->is_registered( $name ) ) {
/* translators: 1: block name */
$message = sprintf( __( 'Block type "%s" is not registered.' ), $name );
_doing_it_wrong( __FUNCTION__, $message, '0.1.0' );
return false;
}

$unregistered_block_type = $this->registered_block_types[ $name ];
unset( $this->registered_block_types[ $name ] );

return $unregistered_block_type;
}

/**
* Retrieves a registered block type.
*
* @since 0.2.0
* @access public
*
* @param string $name Block type name including namespace.
* @return WP_Block_Type|null The unregistered block type, or null if it is not registered.
Copy link
Member

Choose a reason for hiding this comment

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

Typo: unregistered => registered.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in a408132

*/
public function get_registered( $name ) {
if ( ! $this->is_registered( $name ) ) {
return null;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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).

}

return $this->registered_block_types[ $name ];
}

/**
* Checks if a block type is registered.
*
* @since 0.2.0
* @access public
*
* @param tring $name Block type name including namespace.
* @return bool True if the block type is registered, false otherwise.
*/
public function is_registered( $name ) {
return isset( $this->registered_block_types[ $name ] );
}

/**
* Utility method to retrieve the main instance of the class.
*
* The instance will be created if it does not exist yet.
*
* @since 0.2.0
* @access public
* @static
*
* @return WP_Block_Type_Registry The main instance.
*/
public static function get_instance() {
if ( null === self::$instance ) {
self::$instance = new self();
}

return self::$instance;
}
}
74 changes: 74 additions & 0 deletions lib/class-wp-block-type.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
<?php
/**
* Blocks API: WP_Block_Type class
*
* @package gutenberg
* @since 0.2.0
*/

/**
* Core class representing a block type.
*
* @since 0.2.0
*
* @see register_block_type()
*/
final class WP_Block_Type {
/**
* Block type key.
*
* @since 0.2.0
* @access public
* @var string
*/
public $name;

/**
* Block type render callback.
*
* @since 0.2.0
* @access public
* @var callable
*/
public $render;
Copy link
Member

Choose a reason for hiding this comment

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

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


/**
* Constructor.
*
* Will populate object properties from the provided arguments.
*
* @since 0.2.0
* @access public
*
* @see register_block_type()
*
* @param string $block_type Block type name including namespace.
* @param array|string $args Optional. Array or string of arguments for registering a block type.
* Default empty array.
*/
public function __construct( $block_type, $args = array() ) {
$this->name = $block_type;

$this->set_props( $args );
}

/**
* Sets block type properties.
*
* @since 0.2.0
* @access public
*
* @param array|string $args Array or string of arguments for registering a block type.
*/
public function set_props( $args ) {
$args = wp_parse_args( $args, array(
'render' => null,
) );

$args['name'] = $this->name;

foreach ( $args as $property_name => $property_value ) {
Copy link
Member

Choose a reason for hiding this comment

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

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

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 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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

$this->$property_name = $property_value;
}
}
}