close

Make WordPress Core

Opened 4 weeks ago

Closed 3 weeks ago

Last modified 2 weeks ago

#64651 closed task (blessed) (fixed)

Icons: Backport Icons Registry and wp/v2/icons endpoint from Gutenberg

Reported by: mcsf's profile mcsf Owned by: mcsf's profile mcsf
Milestone: 7.0 Priority: normal
Severity: normal Version:
Component: REST API Keywords: has-patch has-unit-tests gutenberg-merge
Focuses: Cc:

Description

Gutenberg PR: https://github.com/WordPress/gutenberg/pull/72215
Blocker for the Icon block: https://github.com/WordPress/gutenberg/pull/71227
Blessed in: https://github.com/WordPress/gutenberg/issues/16484

The work in Gutenberg introduced a new singleton registry class, WP_Icons_Registry, to mediate access to SVG icons. For 7.0, this registry should be closed to third parties and only serve a subset of the icons defined in @wordpress/icons. Starting with 7.1, or as soon as possible, the registry should be open to third parties to register custom icons.

As a companion to the registry class, a new REST API controller defined endpoints /wp/v2/icons and /wp/v2/icons/<slug>, meant only for retrieval.

Together, they are essential infrastructure to the Icon block, which is to be shipped in 7.0 too. After 7.0, that block should be able to allow users to add custom icons to their collection.

Change History (19)

Image

This ticket was mentioned in PR #10909 on WordPress/wordpress-develop by @mcsf.


4 weeks ago
#1

  • Keywords has-patch has-unit-tests added

#2 Image @ellatrix
4 weeks ago

  • Type changed from defect (bug) to task (blessed)

#3 Image @wildworks
3 weeks ago

  • Keywords gutenberg-merge added

Image

@wildworks commented on PR #10909:


3 weeks ago
#4

  • Copy all icons, but check whether the public field is true in the icon registry.
  • When copying icons, if the public field is not true, do not copy the icon. Additionally, remove icons not marked as 'public' => true from the manifest.php file.

Perhaps the latter approach is ideal if we don't want to ship internal icons entirely into core, but it may be a bit more complicated. What do you think?

@mcsf Since we're getting close to the Beta 1 release and the former approach is simpler, I’ve committed 970944b using that approach. I’d appreciate your thoughts.

Image

@mcsf commented on PR #10909:


3 weeks ago
#5

@mcsf Since we're getting close to the Beta 1 release and the former approach is simpler, I’ve committed 970944b using that approach. I’d appreciate your thoughts.

Sorry, I thought I had submitted a reply but apparently not. Yes, that's what I would prefer!

Image

@wildworks commented on PR #10909:


3 weeks ago
#6

Update:

  • 7b941e456b0c4e7e9603c13c6b8ee27f75cc8d82: I skipped the failing tests because there are no public icons in manifest.php yet. Once the Gutenberg commit hash is updated, we'll be able to re-enable the skipped tests. At first, I thought about registering a mock icon, but after reading this comment, I realized that that approach wasn't the right one.
  • 15a38c31b97e5988027998e2319e05e48d1434f9: I added a ticket tag.

Image

@mcsf commented on PR #10909:


3 weeks ago
#7

  • 7b941e4: I skipped the failing tests because there are no public icons in manifest.php yet. Once the Gutenberg commit hash is updated, we'll be able to re-enable the skipped tests. At first, I thought about registering a mock icon, but after reading this comment, I realized that that approach wasn't the right one.

Awesome, thanks. For the record, I just tested building WordPress with an updated Gutenberg ref, removed the tests' skip instructions, and I can confirm that the tests pass with the right manifest. 👍

Image

@mcsf commented on PR #10909:


3 weeks ago
#8

I think we should be good to go. What else is missing for this initial commit?

Image

@mcsf commented on PR #10909:


3 weeks ago
#9

It looks almost good, but I overlooked an important point: the text domain of the icon in the manifest.php file is gutenberg, so it will not be translatable in core. Would it be difficult to remove this text domain during the build process?

Great point. Will explore this after a call I'm on!

Image

@wildworks commented on PR #10909:


3 weeks ago
#10

@mcsf, I think I've probably addressed all of that, but feel free to update!

Image

@mcsf commented on PR #10909:


3 weeks ago
#11

@mcsf, I think I've probably addressed all of that, but feel free to update!

Oh, I didn't realise! I only realised you had already done this when I tried to push and had merge conflicts. 😅 In any case, thanks! Looks exactly like what I had.

#12 Image @mcsf
3 weeks ago

  • Owner set to mcsf
  • Resolution set to fixed
  • Status changed from new to closed

In 61674:

Icons library: Add WP_Icons_Registry, core icon set and REST endpoint

Introduces a registry class to mediate access to a library of SVG icons. These
icons can be queried by directly interfacing with the singleton class
WP_Icons_Registry, or via the REST API using the following GET endpoints:

  • /wp/v2/icons
  • /wp/v2/icons/$name, e.g. /wp/v2/icons/core/audio

Modifies the Gutenberg-to-Core copy process to copy from @wordpress/icons:

  • the icons (SVG files) to wp-includes/icons/library/*.svg
  • the manifest file to wp-includes/icons/manifest.php

For 7.0, the registry remains closed to third-party icons, serving only core
icons per the manifest file.

Together, these APIs power the new Icon block.

Developed in https://github.com/WordPress/gutenberg/pull/72215
Developed in https://github.com/WordPress/gutenberg/pull/74943
Developed in https://github.com/WordPress/wordpress-develop/pull/10909

Props mcsf, wildworks, fabiankaegy, joen, jorgefilipecosta, ntsekouras.
Fixes #64651.

Image

@mcsf commented on PR #10909:


3 weeks ago
#13

Sorry for the confusion! But I think this PR is ready 👍

P.S. I'm planning to make some adjustments to how manifest.php works to avoid shipping unnecessary icon sets in core. I'll be submitting a PR in Gutenberg soon.

I wanted to get this big patch committed once and for all, but I have since reviewed your manifest.php PR, so this is a heads-up that I'll commit a follow-up to remove the ! ( $icon_data['public'] ?? false ) guard.

Image

@wildworks commented on PR #10909:


3 weeks ago
#14

this is a heads-up that I'll commit a follow-up to remove the "! ( $icon_datapublic? ?? false )" guard.

Thank you! I was going to let you know that too 😄

#15 Image @mcsf
3 weeks ago

In 61675:

Icons library: Register all icons found in manifest

As of https://github.com/WordPress/gutenberg/pull/75684, icons' manifest.php
file only lists "public" icons, and in the process the now-redundant `"public"
=> true` flag was dropped.

Accordingly, this removes the check for the public flag from the icon
registry's registration logic.

Follow-up to [61674].

Props mcsf, wildworks, ellatrix.
Fixes #64651.

#16 Image @sabernhardt
3 weeks ago

  • Milestone changed from Awaiting Review to 7.0

Image

This ticket was mentioned in PR #11047 on WordPress/wordpress-develop by @mcsf.


2 weeks ago
#17

Trac ticket: https://core.trac.wordpress.org/ticket/64651

Context: https://github.com/WordPress/gutenberg/pull/75878

  • I'm fairly confident about the switch to protected visibility. It would be nice to include in 7.0 Beta.
  • I'm less so about introducing a filter. We don't have to rush that.
  • Unsolved: can we do anything Core-side to make it easier to _replace WP_Icon_Registry entirely_? (See comment.)

Trac ticket:

## Use of AI Tools

None

#18 Image @mcsf
2 weeks ago

In 61748:

Icons library: Prefer 'protected' visibility in WP_Icons_Registry

Make it easier to iterate on this class in Gutenberg via class extension.

See proof of concept and discussion in:
https://github.com/WordPress/gutenberg/pull/75878

Follow-up to [61674].

Props mcsf.

See #64651.

Image

@wildworks commented on PR #11047:


2 weeks ago
#19

This PR was committed in changeset 61748.

Note: See TracTickets for help on using tickets.