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

creativecoder · 2024-02-01T03:02:43Z

What?

Restructures WP_Font_Collection to access font collection data using a ::get_data method rather than public properties so that we can lazy load collection json and avoid making http requests or file reads when not needed
Removes wp_register_font_collection_from_json in favor of providing a config array or a file path when using wp_register_font_collection
Moves the google-fonts collection registration into a hooked function on init rather than the global scope

Why?

#58363 caused a performance regression. Although it added caching, making the initial request for the font collection json on collection registration causes an unnecessary http request on every page load.

Testing Instructions

During a regular page load on the site, or in wp-admin, use logging or otherwise to see that WP_Font_Collection::load_from_json is not called
Load the Font Library and see that the Google Fonts collection works as expected

github-actions · 2024-02-01T03:04:16Z

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/getData.php
❔ lib/compat/wordpress-6.5/fonts/class-wp-font-collection.php
❔ lib/compat/wordpress-6.5/fonts/class-wp-font-library.php
❔ lib/compat/wordpress-6.5/fonts/class-wp-rest-font-collections-controller.php
❔ lib/compat/wordpress-6.5/fonts/class-wp-rest-font-faces-controller.php
❔ lib/compat/wordpress-6.5/fonts/class-wp-rest-font-families-controller.php
❔ lib/compat/wordpress-6.5/fonts/fonts.php
❔ phpunit/tests/fonts/font-library/wpFontCollection/__construct.php
❔ phpunit/tests/fonts/font-library/wpFontLibrary/registerFontCollection.php
❔ phpunit/tests/fonts/font-library/wpRestFontCollectionsController.php

lib/experimental/fonts/font-library/font-library.php

matiasbenedetto · 2024-02-01T14:44:13Z

If I register a font collection like this

wp_register_font_collection( 'https://dummyjson.com/products/1' );

I get 2 warnings. The second one seems unexpected.

Notice: Function WP_Font_Collection::validate_config was called incorrectly. Font collection "https-dummyjson-com-products-1" 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

Notice: Function WP_Font_Collection::__construct was called incorrectly. Font collection slug "https://dummyjson.com/products/1" is not valid. Slugs must use only alphanumeric characters, dashes, and underscores. 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

And the collection is listed like this in the REST API:

    {
       "slug": "https-dummyjson-com-products-1",
       "name": "Unnamed Font Collection",
       "description": ""
   },

If I register the collection using a hook:

function register_collections() {
    wp_register_font_collection( 'https://dummyjson.com/products/1' );
}
add_action( 'rest_api_init', 'register_collections' );

I get no warning and the same API response:

    {
       "slug": "https-dummyjson-com-products-1",
       "name": "Unnamed Font Collection",
       "description": ""
   },

matiasbenedetto · 2024-02-01T14:54:57Z

lib/experimental/fonts/font-library/font-library.php

-	 */
-	function wp_register_font_collection_from_json( $file_or_url ) {
-		return WP_Font_Library::register_font_collection_from_json( $file_or_url );
+	function wp_register_font_collection( $slug, $args_or_file = array() ) {


It seems like both arguments should be mandatory now because without $args_or_file, the collection is useless, and the data can't be added after the construction of the object.

Currently, you can call the function like this:

wp_register_font_collection( 'https://dummyjson.com/products/1' );

without getting any notice about the lack of the correct number of parameters.
I think a _doing_it_wrong notice is needed for the function to be called with only one parameter.
Apart from that, the collection should not be registered as is happening in this example:

#58530 (comment)

wp_register_font_collection and related functions have been updated to require both function parameters.

github-actions · 2024-02-01T21:26:48Z

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 props-bot label.

Core SVN

If you're a Core Committer, use this list when committing to wordpress-develop in SVN:

Props: grantmkin, pbking, youknowriad, mmaattiiaass.

GitHub Merge commits

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: creativecoder <grantmkin@git.wordpress.org>
Co-authored-by: pbking <pbking@git.wordpress.org>
Co-authored-by: youknowriad <youknowriad@git.wordpress.org>
Co-authored-by: matiasbenedetto <mmaattiiaass@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

creativecoder · 2024-02-02T03:17:51Z

This PR is now updated and should be ready for review.

youknowriad · 2024-02-02T09:15:11Z

lib/experimental/fonts/font-library/font-library.php

-wp_register_font_collection_from_json( 'https://raw.githubusercontent.com/WordPress/google-fonts-to-wordpress-collection/01aa57731575bd13f9db8d86ab80a2d74e28a1ac/releases/gutenberg-17.6/collections/google-fonts-with-preview.json' );
+function gutenberg_register_font_collections() {
+	// TODO: update to production font collection URL.
+	wp_register_font_collection( 'google-fonts', 'https://raw.githubusercontent.com/WordPress/google-fonts-to-wordpress-collection/01aa57731575bd13f9db8d86ab80a2d74e28a1ac/releases/gutenberg-17.6/collections/google-fonts-with-preview.json' );


What's the story of this random URL? Can we replace it with a build script or something? I'm sure that including this as is into WordPress Core is a no go.

This URL is provisional until the last version of the file is deployed WordPress.org CDN.

random URL?

Do you refer to the domain of the URL or the fact that is loading from an external URL?

If it's WordPress.org CDN, I'm ok with that, I believe we already use that CDN in a few places on WordPress already.

Should we just use the outdated version for now? I mean should we make the change in Gutenberg (and the Core patch)

Some fields were added to the data of the file. Until the current file version is deployed to the CDN, we must use a file with all the required fields. As soon as we get the new file deployed to the CDN, we can replace that URL.

@matiasbenedetto What's blocking the new file from being uploaded to the CDN?

The latest changes will be merged soon and after that we will request to deploy the changes in this track ticket: https://meta.trac.wordpress.org/ticket/7282#ticket

The deploy to the CDN was already requested here: https://meta.trac.wordpress.org/ticket/7282#comment:17

For now, I've updated the url to the release version of the collection in Github.

Reverted, I did that too soon, as the preview files aren't loaded at s.w.org yet.

youknowriad

I see that the performance numbers are back to normal. Thanks for the fix. The code here is in general easier to follow for me as well.

matiasbenedetto · 2024-02-02T15:04:25Z

I tested again the latest changes.
There's an unwanted behavior when a font collection doesn't have the correct data. If you register a font collection with wrong data but have other font collections configured as expected, the /font-collections endpoint fails with error 500, breaking all the collections.

examples:

wp_register_font_collection( 'collection-with-empty-data' , array() );

wp_register_font_collection( 'collection-with-fake-url' , 'https://dummyjson.com/products/1' );

API response

{
    "code": "font_collection_missing_property",
    "message": "Font collection \"collection-with-empty-data\" has missing or empty property: \"name.\"",
    "data": {
        "status": 500
    }
}

We should output the collections that are working fine to avoid the error. Otherwise, one failing collection breaks all the other collections.

…r returns

…ions

creativecoder · 2024-02-02T16:25:47Z

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

-				// translators: %s: File path or url to font collection json file.
-				$message = sprintf( __( 'Font collection JSON file "%s" is invalid or does not exist.', 'gutenberg' ), $file_or_url );
+				// translators: %s: File path or url to font collection JSON file.
+				$message = __( 'Font collection JSON file is invalid or does not exist.', 'gutenberg' );


This error may be returned in an API response, so I removed the file path from the message to avoid leaking the server folder structure.

creativecoder · 2024-02-02T16:26:55Z

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

 		 */
-		private static $collection_json_cache = array();


The static caching is removed in favor of storing the collection data in the class instance after it's been loaded and verified.

creativecoder · 2024-02-02T16:28:16Z

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

-			if ( empty( $data['slug'] ) ) {
-				// translators: %s: Font collection JSON URL.
-				$message = sprintf( __( 'Font collection JSON file "%s" requires a slug.', 'gutenberg' ), $file );
-				_doing_it_wrong( __METHOD__, $message, '6.5.0' );
-				return new WP_Error( 'font_collection_invalid_json', $message );
-			}
-
-			static::$collection_json_cache[ $file ] = $data;
-


The slug is now a required arg for both php and json collection registration and validated by the constructor.

creativecoder · 2024-02-02T16:38:42Z

This has been rebased and updated to resolve the error pointed out by @matiasbenedetto, now ready for another review.

pbking · 2024-02-02T16:38:42Z

phpunit/tests/fonts/font-library/wpFontCollection/getData.php

+		}
+
+		return array(
+			'body'     => json_encode( self::$mock_collection_data ),


does this need to be wp_json_encode?

Good catch. Seems best to be consistent. Updated in 6f0b823.

Still using the Github url until s.w.org is updated.

This reverts commit 4cc23a5.

matiasbenedetto

The use of a hook to register the collection seems to be breaking the unregistered function. The sequence of actions is altered. .

If I try to unregister the google-fonts collection from a plugin, I get a notice, and the collection is not unregistered. Now the regression added in #58363 is fixed, the hook doesn't need to be necessary, so I think it can be removed.

Example:

wp_unregister_font_collection ( 'google-fonts' );

The response of the /font-collections/ endpoint

<b>Notice</b>:  Function WP_Font_Library: :unregister_font_collection was called incorrectly. Font collection "google-fonts" not found. Please see <a>Debugging in WordPress</a> for more information. (This message was added in version 6.5.0.) in <b>/var/www/html/wp1/wp-includes/functions.php</b> on line <b>6031</b><br />
[
    {
        "slug": "google-fonts",
        "name": "Google Fonts",
        "description": "Install from Google Fonts. Fonts are copied to and served from your site."
    }
]

lib/compat/wordpress-6.5/fonts/fonts.php

matiasbenedetto

👍 I don't like to rely on hook names to do that because that forces the extenders to understand the hooks and their order, which can be confusing. But due to this pattern being followed by other similar core functions, I think it's ok and should replicate that for consistency.
LGTM
Let's merge 🚀

creativecoder added [Type] Performance Related to performance efforts [Feature] Font Library labels Feb 1, 2024

creativecoder requested review from pbking, youknowriad and matiasbenedetto February 1, 2024 03:02

creativecoder self-assigned this Feb 1, 2024

creativecoder commented Feb 1, 2024

View reviewed changes

lib/experimental/fonts/font-library/font-library.php Outdated Show resolved Hide resolved

matiasbenedetto reviewed Feb 1, 2024

View reviewed changes

pbking force-pushed the try/font-collection-lazy-loading branch from 877c9d4 to fa2ea8c Compare February 2, 2024 02:16

creativecoder marked this pull request as ready for review February 2, 2024 03:17

creativecoder requested a review from spacedmonkey as a code owner February 2, 2024 03:17

youknowriad reviewed Feb 2, 2024

View reviewed changes

youknowriad approved these changes Feb 2, 2024

View reviewed changes

matiasbenedetto mentioned this pull request Feb 2, 2024

Font Library: WordPress 6.5 Required Tasks #55277

Closed

25 tasks

matiasbenedetto mentioned this pull request Feb 2, 2024

Add collection data for Gutenberg 17.7 WordPress/google-fonts-to-wordpress-collection#19

Merged

creativecoder and others added 9 commits February 2, 2024 09:54

Font Collections: lazy load json configuration for better performance

c037ddd

Use explicit get_data method for font collections and restore WP_Erro…

c351aff

…r returns

Do all loading and data validation in WP_Font_Collection::get_data

554d815

Refactored the Font Collection class unit tests

17c29d8

Fix test - suppress file loading error to test get_data() error response

ce1f4e6

Fix linting errors

b30de56

Move tests back to one file per method and align with standards

c6049f7

Remove outdated test

a177999

Remove test file not wanted from rebase

d4aed16

matiasbenedetto mentioned this pull request Feb 2, 2024

Font Library: Remove slug from collection schema to accommodate the changes on the wp_register_font_collection function signature #58623

Merged

matiasbenedetto mentioned this pull request Feb 2, 2024

Update Font Library non-REST API code to align with Core standards #58607

Merged

2 tasks

When getting collections from the REST API, only return valid collect…

20c88f9

…ions

creativecoder commented Feb 2, 2024

View reviewed changes

creativecoder force-pushed the try/font-collection-lazy-loading branch from c184ea3 to 20c88f9 Compare February 2, 2024 16:35

pbking reviewed Feb 2, 2024

View reviewed changes

creativecoder added 3 commits February 2, 2024 10:39

Update test to use wp_json_encode

6f0b823

Update google fonts collection url to release version

4cc23a5

Still using the Github url until s.w.org is updated.

Revert "Update google fonts collection url to release version"

a2f9238

This reverts commit 4cc23a5.

matiasbenedetto requested changes Feb 2, 2024

View reviewed changes

lib/compat/wordpress-6.5/fonts/fonts.php Outdated Show resolved Hide resolved

Fix collection controller test

4b11e02

matiasbenedetto approved these changes Feb 2, 2024

View reviewed changes

matiasbenedetto merged commit 0ec7554 into trunk Feb 2, 2024
61 of 63 checks passed

matiasbenedetto deleted the try/font-collection-lazy-loading branch February 2, 2024 18:08

github-actions bot added this to the Gutenberg 17.7 milestone Feb 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

creativecoder commented Feb 1, 2024 •

edited

github-actions bot commented Feb 1, 2024 •

edited

matiasbenedetto commented Feb 1, 2024

matiasbenedetto Feb 1, 2024 •

edited

creativecoder Feb 2, 2024

github-actions bot commented Feb 1, 2024

creativecoder commented Feb 2, 2024

youknowriad Feb 2, 2024 •

edited

matiasbenedetto Feb 2, 2024

matiasbenedetto Feb 2, 2024

youknowriad Feb 2, 2024

matiasbenedetto Feb 2, 2024

youknowriad Feb 2, 2024

matiasbenedetto Feb 2, 2024

matiasbenedetto Feb 2, 2024

creativecoder Feb 2, 2024

creativecoder Feb 2, 2024

youknowriad left a comment

matiasbenedetto commented Feb 2, 2024 •

edited

creativecoder Feb 2, 2024

creativecoder Feb 2, 2024

creativecoder Feb 2, 2024

creativecoder commented Feb 2, 2024

pbking Feb 2, 2024

creativecoder Feb 2, 2024

matiasbenedetto left a comment •

edited

matiasbenedetto left a comment

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

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

Conversation

creativecoder commented Feb 1, 2024 • edited

What?

Why?

Testing Instructions

github-actions bot commented Feb 1, 2024 • edited

matiasbenedetto commented Feb 1, 2024

matiasbenedetto Feb 1, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 1, 2024

Core SVN

GitHub Merge commits

creativecoder commented Feb 2, 2024

youknowriad Feb 2, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad left a comment

Choose a reason for hiding this comment

matiasbenedetto commented Feb 2, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

creativecoder commented Feb 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matiasbenedetto left a comment • edited

Choose a reason for hiding this comment

matiasbenedetto left a comment

Choose a reason for hiding this comment

creativecoder commented Feb 1, 2024 •

edited

github-actions bot commented Feb 1, 2024 •

edited

matiasbenedetto Feb 1, 2024 •

edited

youknowriad Feb 2, 2024 •

edited

matiasbenedetto commented Feb 2, 2024 •

edited

matiasbenedetto left a comment •

edited