Try: Extend classes of Icons APIs for post-7.0 work

[mcsf]

In this proof of concept, we enhance the Icons registry via compat classes to:

• Also search within icon labels, not just icon names
• Point at Gutenberg's own manifest.php file rather than Core's
• Edit the label of icon core/table to "Table Verse" in order to test searching for "verse" (which should return core/verse and core/table).

The body of Gutenberg_Icons_Registry_7_1 illustrates the downside of keeping most classes in WP_Icons_Registry and WP_REST_Icons_Controller private: almost everything needs to be redefined in the extended classp.

If we used protected instead, only the following would really need to be defined in Gutenberg_Icons_Registry_7_1:

• __construct: to point to the new manifest.php
• get_registered_icons: to extend searching to labels
• get_instance and $instance: to break away from the base class

But currently we need to add the following:

• register
• get_content
• sanitize_icon_content
• get_registered_icon
• is_registered

What?

Closes

Why?

How?

Testing Instructions

Testing Instructions for Keyboard

Screenshots or screencast

Before	After

[github-actions]

Warning: Type of PR label mismatch

To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.

• Required label: Any label starting with [Type].
• Labels found: .

Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task.

[t-hamano]

Thanks for working on this!

The body of Gutenberg_Icons_Registry_7_1 illustrates the downside of keeping most classes in WP_Icons_Registry and WP_REST_Icons_Controller private: almost everything needs to be redefined in the extended classp.

If we used protected instead, only the following would really need to be defined in Gutenberg_Icons_Registry_7_1:

Many of the methods will likely become public in the future, so changing them to protected should be fine.

Another approach would be to create a complete copy of the registry and controller for Gutenberg. This is already done for some classes and APIs. So, the configuration would look something like this:

lib/
　├ class-wp-icons-registry-gutenberg.php
　└ class-wp-rest-icons-controller-gutenberg.php

Theendpoints will always be overridden with the Gutenberg version, like this.

// lib/rest-api.php
function gutenberg_register_icon_controller_endpoints() {
	$icons_registry = new WP_REST_Icons_Controller_Gutenberg();
	$icons_registry->register_routes();
}
add_action( 'rest_api_init', 'gutenberg_register_icon_controller_endpoints' );

What do you think?

The only thing I can't solve is that registry overrides only take effect within the endpoint.

So, different registries are referenced when using the WP_Icons_Registry class directly and when referencing the wp/v2/icons endpoint. I can't think of a good way to solve this right now 🤔

[mcsf]

Another approach would be to create a complete copy of the registry and controller for Gutenberg. This is already done for some classes and APIs. So, the configuration would look something like this:

[…]

What do you think?

Either way seems fine to me. :) In theory the extends approach can give developers a better sense of what is already in Core and what is a modification, but that comes with a slight overhead, whereas keeping a whole "rolling copy" in Gutenberg is maybe easier. I suppose annotations in the code are in order to explain what deviates from Core, what will need a backport, etc.

The only thing I can't solve is that registry overrides only take effect within the endpoint.

So, different registries are referenced when using the WP_Icons_Registry class directly and when referencing the wp/v2/icons endpoint. I can't think of a good way to solve this right now 🤔

Right, exactly! Same question here.

So I did notice that, if the base class has everything protected, it's technically possible to define a derived class such that, in the end, both classes share a static $instance. In short, it looked a little bit like this:

class Gutenberg_Foo extends WP_Foo {
    public static hijack_instance() {
        self::$instance = new self();
    }
}
Gutenberg_Foo::hijack_instance();

echo WP_Foo::get_instance() === Gutenberg_Foo::get_instance(); // 1

… but this is very hacky. I don't know what unintended consequences it may have.

The other hacky way is using PHP reflection.

Finally, the least hacky way is to introduce a filter and do as we do in parse_blocks:

function parse_blocks( $content ) {
	$parser_class = apply_filters( 'block_parser_class', 'WP_Block_Parser' );
	$parser = new $parser_class();
	return $parser->parse( $content );
}

Should we aim to do the latter during this Beta cycle? Am I looking at things the wrong way? Let me know. :)

[t-hamano]

Finally, the least hacky way is to introduce a filter and do as we do in parse_blocks:

function parse_blocks( $content ) {
	$parser_class = apply_filters( 'block_parser_class', 'WP_Block_Parser' );
	$parser = new $parser_class();
	return $parser->parse( $content );
}

Should we aim to do the latter during this Beta cycle?

Sorry, I still don't understand this approach 🤔 If we were to apply this approach to the icon registry, what would the code look like specifically?

[mcsf]

Sorry, I still don't understand this approach 🤔 If we were to apply this approach to the icon registry, what would the code look like specifically?

Maybe by adding a filter inside get_instance...? 🤔