Icons API: Support searching in labels; extend classes post-7.0 work

[mcsf]

What?

Enhance the Icons registry via compat classes to:

• Also search within icon labels, not just icon names
• Point to Gutenberg's own manifest.php file rather than Core's

This PR started as a prototype to gauge what would be needed for future extensibilty. Through it, we've already switched the visibilty of the methods and attributes in Core's WP_Icons_Registry from private to protected. Thanks to that, class extension is much simpler (1b9874e). See the edit history of this PR description for more.

Why?

• Searching in labels is an improvement in itself.
• This change sets the stage for further iteration on these classes (Icons API: Support searching in labels; extend classes post-7.0 work #75878 (comment)).

How?

• Define and load derived class Gutenberg_Icons_Registry_7_1
  • In it, modify/extend as needed for the search feature
  • Also redefine $instance and get_instance() to avoid conflicting with the base registry class
• Define and load derived class Gutenberg_REST_Icons_Controller_7_1
  • In it, redefine the necessary methods so that the controller calls Gutenberg_Icons_Registry_7_1 instead of WP_Icons_Registry
• Hooking into rest_api_init with low priority to let Gutenberg_REST_Icons_Controller_7_1 override the REST routes registered by the base class

Testing Instructions

• Ensure the Icon block (New Block: Icon Block #71227) works normally
• By interacting with the REST API directly, try searching for icons by label and not just slug
  • /wp/v2/icons?search=%s
• The new phpunit test does this by searching for "@" and matching icon core/at-symbol

[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...? 🤔

[t-hamano]

P.S. I plan to move all the code in the constructor to a hook in the future, so that Gutenbeg can easily override the core icons.

It will probably look something like this. What do you think?

// Probably in WordPress 7.1
function _register_core_icons() {
	$icons_directory = __DIR__ . '/icons/';
	// ...
}
add_action( 'init', '_register_core_icons' );

// On the Gutenberg plugin side, remove the core hook and re-register the core icon
remove_action( 'init', '_register_core_icons' );
add_action( 'init', '_gutenberg_register_core_icons' );

function _gutenberg_register_core_icons() {
	// In WordPress 7.0, the core icon registration process is hard-coded
	// in the constructor, so we will explicitly unregister all of them here.
	$all_icons = WP_Icons_Registry::get_instance()->get_registered_icons();
	foreach ( $all_icons as $icon ) {
		WP_Icons_Registry::get_instance()->unregister( $icon['name'] );
	}

	// Re-register core icons using assets from Gutenberg
	$icons_directory = __DIR__ . '/../../../packages/icons/src/';
	// ...
}

[mcsf]

I think that would be alright. Assuming that we'll be ready to make ::register public in 7.1, we won't be as concerned with the exposure that this hook would entail. :)

[t-hamano]

@mcsf What do you think about shipping this PR? I think it might be less disruptive to ship this PR before moving forward with the following PR:

• Icons: Enforce strict name validation in register method #76079
• Icons Registry: improve SVG sanitizer #75550

[github-actions]

Flaky tests detected in 4445a89.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/22957926576
📝 Reported issues:

• [Flaky Test] User Mention: should cancel mention selection via Escape keypress event #59902 in /test/e2e/specs/editor/various/autocomplete-and-mentions.spec.js
• [Flaky Test] should undo bold #48558 in /test/e2e/specs/editor/various/undo.spec.js

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: mcsf <mcsf@git.wordpress.org>
Co-authored-by: t-hamano <wildworks@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[mcsf]

@mcsf What do you think about shipping this PR? I think it might be less disruptive to ship this PR before moving forward with the following PR:

• Icons Registry: Enforce strict name validation in register method #76079
• Icons Registry: improve SVG sanitizer #75550

Sounds good if you're good with the approach too, @t-hamano :)

[mcsf]

Sounds good if you're good with the approach too

I'll refresh it to account for the change from private to protected visibility in the Core classes, which will result in a smaller patch.

[mcsf]

Having synced and trimmed the classes and added a test case, I edited the PR title, description and commit message. Feel free to comment or follow up if I missed anything. :)

[t-hamano]

I opened a core ticket to ensure that all iterations added in Gutenberg are properly backported to core.

https://core.trac.wordpress.org/ticket/64847