Icons: Ship all SVG icons to WordPress core#79102
Conversation
|
Size Change: 0 B Total Size: 8.46 MB |
Co-Authored-By: Claude <noreply@anthropic.com>
The show_in_rest filter belonged in the REST controller, not the registry. Filtering inside get_registered_icons() prevented every internal caller from retrieving the full set of registered icons, which contradicts the intent of shipping all icons while exposing only a subset through REST. - get_registered_icons() now returns all icons regardless of show_in_rest. - The REST list and single-item endpoints filter by show_in_rest, so non-public icons are no longer leaked via GET /icons/<name>. - Mirror show_in_rest handling in the WP 7.0 base registry (constructor, allowed keys, boolean validation) so the controller filter works even when the Gutenberg registry override is not active. - Preserve show_in_rest when replaying non-core icons during the registry swap. Co-Authored-By: Claude <noreply@anthropic.com>
…ller The REST filtering was only applied in the WP 7.0 compat controller, which is guarded by class_exists() and skipped whenever WordPress core already defines WP_REST_Icons_Controller. In that case the route is served by WP_REST_Icons_Controller_Gutenberg, which inherited the unfiltered core get_items() and exposed every registered icon regardless of show_in_rest. Override get_items() and get_icon() in the always-loaded Gutenberg controller so non-public icons are excluded from both the list and the single-item endpoints independently of the WordPress version. The registry keeps returning all icons, since server-side rendering looks up icons by name via get_registered_icon() and must resolve content even for icons that are not exposed through REST. Co-Authored-By: Claude <noreply@anthropic.com>
Restore the cosmetic Markdown changes (emphasis style, link format, and the "Code is Poetry" footer) that were unrelated to this PR, keeping only the showInRest documentation relevant to the change. Co-Authored-By: Claude <noreply@anthropic.com>
7db08d2 to
9fdaf8b
Compare
|
Thanks for all the help with icons. Just to be sure, does this PR make the icons public in the Icons block? I'd prefer if we still show only a limited subset there, because once they are available to that block, they are much harder to deprecate, which is something being discussed in context of the fresh energy there is in the icon space. |
No, if it hasn't been displayed in the icon block before, it won't be displayed as before.
|
|
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Thanks for the clarification, I missed the detail 🙏 |
|
Flaky tests detected in 617f209. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/27330510023
|
|
Hey, @t-hamano! I'm worried by this change. We introduced the But now I don't understand the reasoning behind hiding them in the controller while showing them in the registry. The registry is accessible by any third party. And so will IMO, this change entails a change in "policy" around the core icon library, but as far as I can tell this fact isn't made explicit. If we're to proceed, shouldn't we audit the library to make sure we actually want to "promote" all the icons to registry-accessible? |
| const key = formatPHPKey( item.slug, maxKeyLength ); | ||
| const label = escapePHPString( item.label ); | ||
| const filePath = escapePHPString( item.filePath ); | ||
| const showInRest = item.showInRest ? 'true' : 'false'; |
There was a problem hiding this comment.
Noting that the icon manifest obviously grew a lot with this change. That's likely fine on its own, but I'm also considering the changes in #79100 where we're now essentially hydrating the content for every icon whose content is on the filesystem. That might mean hundreds of disk I/O operations; is it possible this will lead to a performance regression?
I'm not sure what the solution is, but maybe it makes sense to consider lazy hydrating the icon content, or maybe introducing some filtering so we don't call get_content() on icons that we won't really need yet.
| 'file_path' => $icons_directory . $icon_data['filePath'], | ||
| 'label' => $icon_data['label'], | ||
| 'file_path' => $icons_directory . $icon_data['filePath'], | ||
| 'show_in_rest' => ! empty( $icon_data['showInRest'] ), |
There was a problem hiding this comment.
Should we add test for the show_in_rest REST API exclusion behavior?
| public function get_icon( $name ) { | ||
| $icon = parent::get_icon( $name ); | ||
|
|
||
| if ( ! is_wp_error( $icon ) && empty( $icon['show_in_rest'] ) ) { |
| } | ||
|
|
||
| if ( isset( $icon_properties['show_in_rest'] ) && ! is_bool( $icon_properties['show_in_rest'] ) ) { | ||
| _doing_it_wrong( |
| * @param WP_REST_Request $request Full details about the request. | ||
| * @return WP_REST_Response|WP_Error Response object on success, or WP_Error object on failure. | ||
| */ | ||
| public function get_items( $request ) { |
There was a problem hiding this comment.
This seems to be fully reimplementing the parent class method. Should we try to reuse it as much as possible and just sprinkle the show_in_rest filter logic on top?
| public function get_icon( $name ) { | ||
| $icon = parent::get_icon( $name ); | ||
|
|
||
| if ( ! is_wp_error( $icon ) && empty( $icon['show_in_rest'] ) ) { |
There was a problem hiding this comment.
Actually, what's the scenario to this path of the code? Won't parent::get_icon() already return a 404 for non-REST icons?
| return `${ key } => array( | ||
| 'label' => _x( '${ label }', 'icon label', 'gutenberg' ), | ||
| 'filePath' => '${ filePath }', | ||
| 'label' => _x( '${ label }', 'icon label', 'gutenberg' ), |
There was a problem hiding this comment.
When are we actually going to surface the translations for icons that we don't show in the REST API? It seems a bit wasteful to localize all those strings and add hundreds of new non-translated strings to core if we're never going to surface them to end users.
| * If not provided, the content will be retrieved from the `file_path` if set. | ||
| * If both `content` and `file_path` are not set, the icon will not be registered. | ||
| * @type string $file_path Optional. The full path to the file containing the icon content. | ||
| * @type bool $show_in_rest Optional. Whether the icon is exposed through the REST API. |
There was a problem hiding this comment.
Note that spacing will need to be aligned here.
file_pathkey in icon registry #79100What?
Previously, only icons marked as "public" within the
iconspackage were available in the icon registry. This has been achieved through the following mechanism:manifest.json, add the field"public": trueto the icons we want to make public.manifest.phpfrommanifest.json, only icons with"public": truedefined are included.manifest.php.This PR removes this restriction, making all icons defined in
manifest.jsonavailable in the icon registry. However, icons that were previously public are now marked with"show_in_rest": trueand are only available via the REST API. This ensures that only the restricted core set of icons remains available in the Icon block as before.How?
"public": truewith"showInRest": trueinmanifest.json.manifest.php, always include the"show_in_rest": true or falsefield.manifest.jsonormanifest.phpand shipped to the core, it can never be removed. See this document for more details. In the future, I might consider implementing a mechanism to deprecate icons in some way, for example, like this:[ { "slug": "clock", "label": "Clock", "filePath": "library/align-right.svg" }, { "slug": "time", "label": "Time", "deprecated": true, "alias": "clock" } ]Testing Instructions
Icon block
Confirm all registered icons
Check the registered icons using the following code.
Use of AI Tools
Although most of the code was written by Claude, I reviewed all of it myself.