Font Library: unregister font collection

[matiasbenedetto]

Closes #54697

What?

Adds a function that enable extenders to unregister a font collection from the library.

Why?

There could be cases where extenders need to make a font collection unavailable.

How?

By implementing the function wp_unregister_font_collection( $id ), extenders can make a particular font collection not loaded.

Testing instructions

Try disabling the default font collection by adding this in a plugin code or in your block theme functions.php:

wp_unregister_font_collection ( 'default-font-collection' );

Expected result:

The default font collection (Google Fonts) should not load in the frontend:

Before	After

[github-actions]

This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress.

If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged.

If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack.

Thank you! ❤️

View changed files

❔ phpunit/tests/fonts/font-library/wpFontLibrary/base.php
❔ phpunit/tests/fonts/font-library/wpFontLibrary/unregisterFontCollection.php
❔ lib/experimental/fonts/font-library/class-wp-font-library.php
❔ lib/experimental/fonts/font-library/font-library.php
❔ phpunit/tests/fonts/font-library/wpFontLibrary/getFontCollection.php
❔ phpunit/tests/fonts/font-library/wpFontLibrary/getFontCollections.php
❔ phpunit/tests/fonts/font-library/wpFontLibrary/getFontsDir.php
❔ phpunit/tests/fonts/font-library/wpFontLibrary/getMimeTypes.php
❔ phpunit/tests/fonts/font-library/wpFontLibrary/registerFontCollection.php
❔ phpunit/tests/fonts/font-library/wpFontLibrary/setUploadDir.php

[pbking]

This didn't seem to work as I expected and I'm not sure why.

The UI changed as desired. I no longer saw the 'Install Fonts' tab as expected. But only if I ran the code early enough (I tried putting it in an 'init' function and it didn't change, but putting it outside of any functions made it work as expected).

However, if I called WP_Font_Library::get_font_collections() immediately after I still got that collection from the list.

[costdev]

Curious about why this needs to be done in an init action within the function. Is this to ensure that WP_Font_Family is initialized beforehand? If so, it might be better that the docs just let extenders know this must occur in the init action or later, or even a _doing_it_wrong() if ! did_action( 'init' ) && ! doing_action( 'init' ).

[matiasbenedetto]

It's because we would need to be sure that all the font collections registered to call wp_register_font_collection() were already registered so we can unregister them using wp_unregister_font_collection(). Is there a better way to be sure that all the font collections are already registered?

[matiasbenedetto]

In the latest commit, I removed the reliance on the hooks by keeping track of the unregistered font collection using an array of collection IDs.

[matiasbenedetto]

Based on the @spacedmonkey comments, I changed the approach following this example.

[matiasbenedetto]

This didn't seem to work as I expected and I'm not sure why.
The UI changed as desired. I no longer saw the 'Install Fonts' tab as expected. But only if I ran the code early enough (I tried putting it in an 'init' function and it didn't change, but putting it outside of any functions made it work as expected).
However, if I called WP_Font_Library::get_font_collections() immediately after I still got that collection from the list.

Thanks for the review!

The latest changes should fix this. I removed the reliance on the init hook by keeping track of the unregistered font collection using an array of collection IDs. b7c9113

[spacedmonkey]

What happens if there are no collections registered? How is this handled? Can we have unit tests for that?

[spacedmonkey]

This feels like a weird pattern. Could we not just unset the value in self::$collections?

[spacedmonkey]

Example

https://github.com/WordPress/wordpress-develop/blob/ce8aed4698117fd0f8c362825e62d01586c8799d/src/wp-includes/class-wp-block-patterns-registry.php#L130-L153

[matiasbenedetto]

Thank you for the example. Very useful. I implemented that in the latest commits.

[matiasbenedetto]

What happens if there are no collections registered? How is this handled? Can we have unit tests for that?

When no collections are registered, get_font_collections() returns an empty array. This is the unit test:

gutenberg/phpunit/tests/fonts/font-library/wpFontLibrary/getFontCollections.php

Lines 31 to 35 in d9ee03a

	public function test_should_get_an_empty_list() {
	$font_collections = WP_Font_Library::get_font_collections();
	$this->assertEmpty( $font_collections, 'Should return an empty array.' );
	}

[creativecoder]

This looks to be working well for me

• ✅ I can unregister the default collection with wp_unregister_font_collection ( 'default-font-collection' );
  • The "Install Fonts" tab no longer displays in the Fonts modal
  • WP_Font_Library::get_font_collections() returns an empty array
• ✅ If I try to unregister a non-existent collection, I see a notice in the PHP error log
• ✅ If I try to register a collection with an existing id, I see a notice in the PHP error log

I left some suggestions for the tests, otherwise I think this is good to go.

[matiasbenedetto]

The PR seems ready to go, please could you give it another review?

[jffng]

Verified:

• Unregister an existing collection
• Warnings are thrown when attempting to register an existing collection or unregister a non-existing collection
• Unit tests incorporate the feedback