Font Library: Update function signature of wp register font collection

[matiasbenedetto]

What?

• Updates function signature of wp_register_font_collection from wp_register_font_collection( $config ) to wp_register_font_collection( $slug, $config ).
• Updates the WP_Font_Collection constructor and settings validation function.
• Updates and improves unit tests.

Why?

To make the function more likely to other core functions.
Requested here: #57884 (comment)

How?

Change the parameters received by the functions.

Testing Instructions

• Use the font library and check that font collections continue working as expected.
• Run unit tests

[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

❔ 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/font-library.php
❔ phpunit/tests/fonts/font-library/wpFontCollection/__construct.php
❔ phpunit/tests/fonts/font-library/wpFontCollection/getConfig.php
❔ phpunit/tests/fonts/font-library/wpFontCollection/getContent.php
❔ phpunit/tests/fonts/font-library/wpFontCollection/isConfigValid.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

[mikachan]

This tests well for me, the font library and font collections are mostly working as expected 👍

I noticed one issue but I'm not sure if I've tested in a weird way. I'm using the Modern Fonts Stacks plugin to test, and I made the slug an empty string, in the wp_register_font_collection call, like this: wp_register_font_collection ( '', $modern_fonts_config );

This seems to break the Modern Fonts Stacks collection but also changes the default fonts collection to the default modal tab name, "Install Fonts":

I'm not sure what's expected here, but the empty slug leaking into the default collection config doesn't seem right.

[mikachan]

Should these three _doing_it_wrong calls use sprintf placeholders, similar to this change?

[matiasbenedetto]

👍 Added that here: b826cea

[matiasbenedetto]

This seems to break the Modern Fonts Stacks collection but also changes the default fonts collection to the default modal tab name, "Install Fonts".

Yep, that's expected. That was a request from some time ago about displaying a different name for the default font collection if there is more than one collection registered.

[creativecoder]

I've been looking over existing registration functions and related classes in Core and using an $args = array() as the last param is standard, but it seems to always be optional.

I'm thinking we should have all of the required font collection config be explicit arguments, and allow $args to be empty. For example:

// These would default to empty strings or array if not provided:
$args = array(
    'name'        => __( 'My Collection' ),
    'description' => __( 'My collection of fonts', 'text-domain' ),
    'categories   => array(
        array(
            'name' => __( 'Sans Serif', 'text-domain' ),
            'slug' => 'sans-serif'
         ),
    ),
);
$src = 'https://s.w.org/images/fonts/17.6/collections/google-fonts-with-preview.json';
wp_register_font_collection( 'my-collection', $src, $args );

Now $src can be a file or an array of font families, so this needs to be addressed. Perhaps two separate functions would be clearer

• wp_register_font_collection accepts a php array of font families
• wp_register_font_collection_from_file accepts a string that's a path to a local or remote file

This is similar to how register_block_type and register_block_type_from_metadata work for blocks, which is another entity that can be registered from php or a json file.

What do you think?

[creativecoder]

@matiasbenedetto @mikachan @pbking and I discussed this a bit on a hangout today. Using a signature like this seems to be most consistent with other registration functions

wp_register_font_collection( $slug, $file_or_array, $args );

• $slug is the font collection slug
• $file_or_array is either a path to a local/remote json file, or an array with font_families and categories
• $args is optional, and currently takes name and description strings

[creativecoder]

After working with font collections more, having the font collection properties split between a json file and PHP config seems quite strange.

I think any performance issues with fetching remote collections should be dealt with separately and not leak into the structure of WP_Font_Collection. (e.g. not using ::get_content() to access some properties that are remote and direct getters for other properties.)

I did an exploration here that addresses this, and adds caching to handle performance considerations: #58363

[matiasbenedetto]

closing in favor of #58363