Font Library: unregister font collection by matiasbenedetto · Pull Request #54701 · WordPress/gutenberg

matiasbenedetto · 2023-09-21T17:07:30Z

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 · 2023-09-21T17:09:05Z

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 · 2023-09-21T20:00:25Z

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 · 2023-10-05T14:21:58Z

+	 */
+	function wp_unregister_font_collection( $collection_id ) {
+		add_action(
+			'init',


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' ).

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?

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.

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

Co-authored-by: Colin Stewart <79332690+costdev@users.noreply.github.com>

…/gutenberg into try/unregister-font-collection

matiasbenedetto · 2023-10-11T13:29:41Z

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 · 2023-10-11T13:30:50Z

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

…if all the font collections were unregistered

spacedmonkey · 2023-10-11T14:11:53Z

+	public static function unregister_font_collection( $collection_id ) {
+		self::$unregistered_collection_ids[] = $collection_id;
+	}


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

Example

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

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

matiasbenedetto · 2023-10-12T14:13:28Z

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.' ); 
        
           }

Co-authored-by: Vicente Canales <1157901+vcanales@users.noreply.github.com>

Co-authored-by: Grant Kinney <creativecoder@users.noreply.github.com>

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 · 2023-12-21T20:07:26Z

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

remove font collection

fb475a2

matiasbenedetto requested a review from spacedmonkey as a code owner September 21, 2023 17:07

matiasbenedetto marked this pull request as draft September 21, 2023 17:07

matiasbenedetto added 5 commits September 21, 2023 15:37

add wp_unregister_font_collection function

a50a2de

format php

8b90373

fix function comment

2968267

add tests for unregister_font_collection

42a61c5

removig unused variable

27102fa

matiasbenedetto marked this pull request as ready for review September 21, 2023 19:25

mikachan added [Feature] Typography Font and typography-related issues and PRs Needs PHP backport Needs PHP backport to Core [Type] Enhancement A suggestion for improvement. labels Sep 22, 2023

matiasbenedetto mentioned this pull request Oct 5, 2023

Font Library WordPress/wordpress-develop#5285

Closed