Font Collections: update registration function signature and add caching #58363

creativecoder · 2024-01-29T03:10:19Z

What?

Updates wp_register_font_collection so that it takes a $slug and optional $arg array, but does not handle JSON
Adds wp_register_font_collection_from_json function specifically for registering a collection from a JSON file
Adds static caching for accessing all font collections plus transient caching for remote collections
Removes WP_Font_Collection::get_content in favor of accessing all properties directly (e.g. $collection->font_families)

Why?

Simplifies/standardizes how collection properties are accessed
Improves performance for accessing font collections from json with caching
Brings PHP function signatures to be more similar to other Core register_ functions

Testing Instructions

Ensure the google fonts collection works as expected (with modified json file from this PR ).
Try registering a font collection in PHP, and make sure it works as expected

Example PHP font collection for testing:

In a plugin:

wp_register_font_collection_from_json( __DIR__ . '/test-font-collection.json' );

test-font-collection.json

{
	"$schema": "https://schemas.wp.org/trunk/font-collection.json",
	"slug": "test-font-collection",
	"name": "Test Font Collection",
	"description": "A test font collection.",
	"font_families": [
		{
			"font_family_settings": {
				"name": "Open Sans",
				"fontFamily": "Open Sans, sans-serif",
				"slug": "open-sans",
				"fontFace": [
					{
						"src": "https://fonts.gstatic.com/s/opensans/v40/memSYaGs126MiZpBA-UvWbX2vVnXBbObj2OVZyOOSr4dVJWUgsiH0C4iY1M2xLER.woff2",
						"fontWeight": "300",
						"fontStyle": "normal",
						"fontFamily": "Open Sans",
						"preview": "https://s.w.org/images/fonts/17.6/previews/open-sans/open-sans-300-normal.svg"
					},
					{
						"src": "https://fonts.gstatic.com/s/opensans/v40/memSYaGs126MiZpBA-UvWbX2vVnXBbObj2OVZyOOSr4dVJWUgsjZ0C4iY1M2xLER.woff2",
						"fontWeight": "400",
						"fontStyle": "normal",
						"fontFamily": "Open Sans",
						"preview": "https://s.w.org/images/fonts/17.6/previews/open-sans/open-sans-400-normal.svg"
					},
					{
						"src": "https://fonts.gstatic.com/s/opensans/v40/memSYaGs126MiZpBA-UvWbX2vVnXBbObj2OVZyOOSr4dVJWUgsjr0C4iY1M2xLER.woff2",
						"fontWeight": "500",
						"fontStyle": "normal",
						"fontFamily": "Open Sans",
						"preview": "https://s.w.org/images/fonts/17.6/previews/open-sans/open-sans-500-normal.svg"
					},
					{
						"src": "https://fonts.gstatic.com/s/opensans/v40/memSYaGs126MiZpBA-UvWbX2vVnXBbObj2OVZyOOSr4dVJWUgsgH1y4iY1M2xLER.woff2",
						"fontWeight": "600",
						"fontStyle": "normal",
						"fontFamily": "Open Sans",
						"preview": "https://s.w.org/images/fonts/17.6/previews/open-sans/open-sans-600-normal.svg"
					},
					{
						"src": "https://fonts.gstatic.com/s/opensans/v40/memSYaGs126MiZpBA-UvWbX2vVnXBbObj2OVZyOOSr4dVJWUgsg-1y4iY1M2xLER.woff2",
						"fontWeight": "700",
						"fontStyle": "normal",
						"fontFamily": "Open Sans",
						"preview": "https://s.w.org/images/fonts/17.6/previews/open-sans/open-sans-700-normal.svg"
					},
					{
						"src": "https://fonts.gstatic.com/s/opensans/v40/memSYaGs126MiZpBA-UvWbX2vVnXBbObj2OVZyOOSr4dVJWUgshZ1y4iY1M2xLER.woff2",
						"fontWeight": "800",
						"fontStyle": "normal",
						"fontFamily": "Open Sans",
						"preview": "https://s.w.org/images/fonts/17.6/previews/open-sans/open-sans-800-normal.svg"
					},
					{
						"src": "https://fonts.gstatic.com/s/opensans/v40/memQYaGs126MiZpBA-UFUIcVXSCEkx2cmqvXlWq8tWZ0Pw86hd0Rk5hkaVIUwaERZjA.woff2",
						"fontWeight": "300",
						"fontStyle": "italic",
						"fontFamily": "Open Sans",
						"preview": "https://s.w.org/images/fonts/17.6/previews/open-sans/open-sans-300-italic.svg"
					},
					{
						"src": "https://fonts.gstatic.com/s/opensans/v40/memQYaGs126MiZpBA-UFUIcVXSCEkx2cmqvXlWq8tWZ0Pw86hd0Rk8ZkaVIUwaERZjA.woff2",
						"fontWeight": "400",
						"fontStyle": "italic",
						"fontFamily": "Open Sans",
						"preview": "https://s.w.org/images/fonts/17.6/previews/open-sans/open-sans-400-italic.svg"
					},
					{
						"src": "https://fonts.gstatic.com/s/opensans/v40/memQYaGs126MiZpBA-UFUIcVXSCEkx2cmqvXlWq8tWZ0Pw86hd0Rk_RkaVIUwaERZjA.woff2",
						"fontWeight": "500",
						"fontStyle": "italic",
						"fontFamily": "Open Sans",
						"preview": "https://s.w.org/images/fonts/17.6/previews/open-sans/open-sans-500-italic.svg"
					},
					{
						"src": "https://fonts.gstatic.com/s/opensans/v40/memQYaGs126MiZpBA-UFUIcVXSCEkx2cmqvXlWq8tWZ0Pw86hd0RkxhjaVIUwaERZjA.woff2",
						"fontWeight": "600",
						"fontStyle": "italic",
						"fontFamily": "Open Sans",
						"preview": "https://s.w.org/images/fonts/17.6/previews/open-sans/open-sans-600-italic.svg"
					},
					{
						"src": "https://fonts.gstatic.com/s/opensans/v40/memQYaGs126MiZpBA-UFUIcVXSCEkx2cmqvXlWq8tWZ0Pw86hd0RkyFjaVIUwaERZjA.woff2",
						"fontWeight": "700",
						"fontStyle": "italic",
						"fontFamily": "Open Sans",
						"preview": "https://s.w.org/images/fonts/17.6/previews/open-sans/open-sans-700-italic.svg"
					},
					{
						"src": "https://fonts.gstatic.com/s/opensans/v40/memQYaGs126MiZpBA-UFUIcVXSCEkx2cmqvXlWq8tWZ0Pw86hd0Rk0ZjaVIUwaERZjA.woff2",
						"fontWeight": "800",
						"fontStyle": "italic",
						"fontFamily": "Open Sans",
						"preview": "https://s.w.org/images/fonts/17.6/previews/open-sans/open-sans-800-italic.svg"
					}
				],
				"preview": "https://s.w.org/images/fonts/17.6/previews/open-sans/open-sans.svg"
			},
			"categories": ["sans-serif"]
		}
	],
	"categories": [
		{
			"name": "Sans Serif",
			"slug": "sans-serif"
		}
	]
}

github-actions · 2024-01-29T03:11:44Z

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/wpFontCollection/loadFromJson.php
❔ lib/experimental/fonts/font-library/class-wp-font-collection.php
❔ lib/experimental/fonts/font-library/class-wp-font-library.php
❔ lib/experimental/fonts/font-library/class-wp-rest-font-collections-controller.php
❔ lib/experimental/fonts/font-library/class-wp-rest-font-families-controller.php
❔ lib/experimental/fonts/font-library/font-library.php
❔ phpunit/tests/fonts/font-library/wpFontCollection/__construct.php
❔ phpunit/tests/fonts/font-library/wpFontLibrary/getFontCollection.php
❔ phpunit/tests/fonts/font-library/wpFontLibrary/getFontCollections.php
❔ phpunit/tests/fonts/font-library/wpFontLibrary/registerFontCollection.php
❔ phpunit/tests/fonts/font-library/wpFontLibrary/unregisterFontCollection.php
❔ phpunit/tests/fonts/font-library/wpRestFontCollectionsController.php

github-actions · 2024-01-29T03:51:10Z

Flaky tests detected in efce06e19986b9df5a4a6bb61d3aab785b34a69c.
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/7703293902
📝 Reported issues:

[Flaky Test] directives inside islands should not be hydrated twice #58332 in /test/e2e/specs/interactivity/tovdom-islands.spec.ts

matiasbenedetto · 2024-01-29T17:10:29Z

lib/experimental/fonts/font-library/class-wp-font-collection.php

+		if ( is_wp_error( $data ) ) {
+			return $data;
 		}

-		return true;
+		return $data;


This seems to be returning $data anyways. What's the purpose of the if() ?

Good point! Will remove the conditional.

matiasbenedetto

If I register a font collection with a wrong URL calling wp_register_font_collection like this:

wp_register_font_collection( 'https://example.com/test.json' );

I get a notice which seems correct:

Notice: Function WP_Font_Collection::__construct was called incorrectly. Font collection "https://example.com/test.json" does not contain any font families. Please see Debugging in WordPress for more information. (This message was added in version 6.5.0.) in /var/www/html/wp1/wp-includes/functions.php on line 6031

I get the collection with a collection with the url used as slug , which seems unexpected:

The UI looks like this, the error message does not describe the error accordingly.

I think this is caused by the use of $slug_or_file variable used to register a font collection.

gutenberg/lib/experimental/fonts/font-library/class-wp-font-library.php

Lines 65 to 76 in 4566ecf

    
           public static function register_font_collection( $slug_or_file, $args = array() ) { 
        
           	if ( file_exists( $slug_or_file ) || wp_http_validate_url( $slug_or_file ) ) { 
        
           		$args = WP_Font_Collection::load_from_json( $slug_or_file ); 
        
           		if ( is_wp_error( $args ) ) { 
        
           			return $args; 
        
           		} 
        
           		$slug = $args['slug']; 
        
           	} else { 
        
           		$slug = $slug_or_file; 
        
           	}

It seems like it's difficult to differentiate between a wrong url/filepath of a slug.

creativecoder · 2024-01-29T19:04:23Z

I get the collection with a collection with the url used as slug , which seems unexpected:

That seems strange. When I do wp_register_font_collection( 'https://example.com/test.json' ), it returns an error and no collection is registered.

matiasbenedetto · 2024-01-29T19:10:50Z

Can the problem described here: #58363 (review)

be solved by replacing this

gutenberg/lib/experimental/fonts/font-library/class-wp-font-library.php

Lines 55 to 75 in 4566ecf

    
           	/** 
        
           	 * Register a new font collection. 
        
           	 * 
        
           	 * @since 6.5.0 
        
           	 * 
        
           	 * @param string $slug_or_file Font collection slug or path to a JSON file containing the font collection config. 
        
           	 * @param array  $args         Font collection config options. 
        
           	 *                             See {@see wp_register_font_collection()} for the supported fields. 
        
           	 * @return WP_Font_Collection|WP_Error A font collection is it was registered successfully and a WP_Error otherwise. 
        
           	 */ 
        
           	public static function register_font_collection( $slug_or_file, $args = array() ) { 
        
           		if ( file_exists( $slug_or_file ) || wp_http_validate_url( $slug_or_file ) ) { 
        
           			$args = WP_Font_Collection::load_from_json( $slug_or_file ); 
        
           			if ( is_wp_error( $args ) ) { 
        
           				return $args; 
        
           			} 
        
           			$slug = $args['slug']; 
        
           		} else { 
        
           			$slug = $slug_or_file; 
        
           		}

with something like this?

/**
* Register a new font collection.
*
* @since 6.5.0
*
* @param string $slug         Font collection slug or path to a JSON file containing the font collection config.
* @param array  $file_or_args Path to a JSON file containing the font collection config o an array with the font collection config.
*                             See {@see wp_register_font_collection()} for the supported fields.
* @return WP_Font_Collection|WP_Error A font collection is it was registered successfully and a WP_Error otherwise.
*/
public static function register_font_collection( $slug, $file_or_args = array() ) {
if ( is_array( $file_or_args ) ) {
	$args = $file_or_args;
} else {
	$args = WP_Font_Collection::load_from_json( $file_or_args );
}

$new_collection = new WP_Font_Collection( $slug, $args );

creativecoder · 2024-01-29T19:52:47Z

I do see a problem when mistyping a url--since it's not a valid url or file path, we have to assume it's a slug, which may not be the case.

Seeing this, I think it's better if we use separate functions for php vs json registration, to make it clear what the intent is. I've updated the PR to include

wp_register_font_collection - for registering a collection with PHP properties
wp_register_font_collection_from_metadata (inspired by register_block_type_from_metadata) for registering a collection from a json file

matiasbenedetto · 2024-01-29T20:21:55Z

Testing the new function, If I set a non existing file I get no notice about it:

wp_register_font_collection_from_metadata( 'non-existing-file.json' );

I think a _doing_it_wrong kind of notice is expected here, right?

creativecoder · 2024-01-29T20:32:41Z

I think a _doing_it_wrong kind of notice is expected here, right?

Yes, that's a good point--you may not be looking at the output of the function to see the WP_Error, so a notice would aid in debugging. Will add that.

matiasbenedetto · 2024-01-29T20:34:46Z

The name wp_register_font_collection_from_metadata seems to be directly transposed from register_block_type_from_metadata. It makes sense to register blocks, because in that case, the file contains block 'metadata' (data about block description, as opposed to block content). In this case, we are registering the content of a font collection, so the name doesn't fit right.

creativecoder · 2024-01-29T20:37:19Z

@matiasbenedetto What do you think would be better? wp_register_font_collection_from_json ?

matiasbenedetto · 2024-01-29T20:41:13Z

@matiasbenedetto What do you think would be better? wp_register_font_collection_from_json ?

Yes, it seems more descriptive of the actual functionality.

creativecoder · 2024-01-29T22:28:39Z

I've updated the PR with the following:

Rename wp_register_font_collection_from_metadata to wp_register_font_collection_from_json
Return error and trigger notice when using invalid url, non-existing file path, or json is missing a slug
Adding proper tests for those errors and notices

matiasbenedetto · 2024-01-30T13:16:10Z

I made a mistake and tried to register a collection like this.

wp_register_font_collection( 'https://im-a-fake-url.com/fonts/font-collection.json' );
wp_register_font_collection( 'https://fakeurl.com/fonts/font-collection.json' );

I got this result:

API response:

UI:

It can be a common mistake among extenders, so the slug should probably be tested to assess if it's a valid slug or not.

…collection_from_json

…ous characters

creativecoder · 2024-01-30T17:36:46Z

@matiasbenedetto That's a good point. In addition to addressing potential user error, I think the slug should be sanitized since it's used as an identifier in the REST API url.

I've updated this in d8f63b4 to sanitize the slug and emit a notice if the given slug doesn't match the sanitized one. Let me know what you think.

matiasbenedetto

This is testing well on my end. I verified that:

wp_register_font_collection_from_json :

Loads properly formatted JSON files as expected.
Raise notices when the URL/file provided is existent or doesn't have the right content.

wp_register_font_collection :

Loads data as expected.
Raises notices when the PHP data provided has missing required arguments like font_families.

creativecoder added [Type] Enhancement A suggestion for improvement. [Feature] Font Library labels Jan 29, 2024

creativecoder requested review from pbking, matiasbenedetto and mikachan January 29, 2024 03:10

creativecoder requested a review from spacedmonkey as a code owner January 29, 2024 03:10

creativecoder self-assigned this Jan 29, 2024

creativecoder requested a review from ajlende as a code owner January 29, 2024 03:10

creativecoder changed the title ~~Font Collection: update registration signature and add caching~~ Font Collections: update registration function signature and add caching Jan 29, 2024

creativecoder mentioned this pull request Jan 29, 2024

Font Library: Update function signature of wp register font collection #58269

Closed

matiasbenedetto reviewed Jan 29, 2024

View reviewed changes

matiasbenedetto mentioned this pull request Jan 29, 2024

Font Library: Update font collection json schema #58413

Merged

creativecoder added 5 commits January 30, 2024 11:31

Font Collection: update registration signature and add caching

03a23eb

Rename test file and correct comments

958be94

Removes redundant conditional

b82a9ab

Reorganizes tests to match WP_Font_Collection methods

b319f50

Truncate long urls in transient key

b387a13

creativecoder added 4 commits January 30, 2024 11:31

Use separate function for creating a font collection from a JSON file

e226f07

Add tests for errors and notices

aef489b

Rename wp_register_font_collection_from_metadata to wp_register_font_…

f8021db

…collection_from_json

Sanitize font collection slug and add notice if slug contains extrane…

d8f63b4

…ous characters

creativecoder force-pushed the add/font-collection-caching branch from d4bf565 to d8f63b4 Compare January 30, 2024 17:31

matiasbenedetto approved these changes Jan 31, 2024

View reviewed changes

creativecoder added 3 commits January 31, 2024 11:43

Updates class_exists checks to match trunk

01d8575

Merge branch 'trunk' into add/font-collection-caching

d44e12a

Fix comments

aaf63ee

creativecoder merged commit 552c76f into trunk Jan 31, 2024
55 checks passed

creativecoder deleted the add/font-collection-caching branch January 31, 2024 18:51

github-actions bot added this to the Gutenberg 17.7 milestone Jan 31, 2024

creativecoder mentioned this pull request Feb 1, 2024

Font Collections: lazy load json configuration for better performance #58530

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Font Collections: update registration function signature and add caching #58363

Font Collections: update registration function signature and add caching #58363

creativecoder commented Jan 29, 2024 •

edited

Loading

github-actions bot commented Jan 29, 2024 •

edited

Loading

github-actions bot commented Jan 29, 2024 •

edited

Loading

matiasbenedetto Jan 29, 2024

creativecoder Jan 29, 2024

matiasbenedetto left a comment •

edited

Loading

creativecoder commented Jan 29, 2024

matiasbenedetto commented Jan 29, 2024

creativecoder commented Jan 29, 2024

matiasbenedetto commented Jan 29, 2024 •

edited

Loading

creativecoder commented Jan 29, 2024

matiasbenedetto commented Jan 29, 2024 •

edited

Loading

creativecoder commented Jan 29, 2024

matiasbenedetto commented Jan 29, 2024

creativecoder commented Jan 29, 2024

matiasbenedetto commented Jan 30, 2024 •

edited

Loading

creativecoder commented Jan 30, 2024

matiasbenedetto left a comment •

edited

Loading

	public static function register_font_collection( $slug_or_file, $args = array() ) {
	if ( file_exists( $slug_or_file ) \|\| wp_http_validate_url( $slug_or_file ) ) {
	$args = WP_Font_Collection::load_from_json( $slug_or_file );
	if ( is_wp_error( $args ) ) {
	return $args;
	}

	$slug = $args['slug'];
	} else {
	$slug = $slug_or_file;
	}

Font Collections: update registration function signature and add caching #58363

Font Collections: update registration function signature and add caching #58363

Conversation

creativecoder commented Jan 29, 2024 • edited Loading

What?

Why?

Testing Instructions

github-actions bot commented Jan 29, 2024 • edited Loading

github-actions bot commented Jan 29, 2024 • edited Loading

matiasbenedetto Jan 29, 2024

Choose a reason for hiding this comment

creativecoder Jan 29, 2024

Choose a reason for hiding this comment

matiasbenedetto left a comment • edited Loading

Choose a reason for hiding this comment

creativecoder commented Jan 29, 2024

matiasbenedetto commented Jan 29, 2024

creativecoder commented Jan 29, 2024

matiasbenedetto commented Jan 29, 2024 • edited Loading

creativecoder commented Jan 29, 2024

matiasbenedetto commented Jan 29, 2024 • edited Loading

creativecoder commented Jan 29, 2024

matiasbenedetto commented Jan 29, 2024

creativecoder commented Jan 29, 2024

matiasbenedetto commented Jan 30, 2024 • edited Loading

creativecoder commented Jan 30, 2024

matiasbenedetto left a comment • edited Loading

Choose a reason for hiding this comment

creativecoder commented Jan 29, 2024 •

edited

Loading

github-actions bot commented Jan 29, 2024 •

edited

Loading

github-actions bot commented Jan 29, 2024 •

edited

Loading

matiasbenedetto left a comment •

edited

Loading

matiasbenedetto commented Jan 29, 2024 •

edited

Loading

matiasbenedetto commented Jan 29, 2024 •

edited

Loading

matiasbenedetto commented Jan 30, 2024 •

edited

Loading

matiasbenedetto left a comment •

edited

Loading