Add additional sanitization for settings.typography.fontFamilies

[jffng]

What?

This PR adds an additional level of sanitization for theme.json's settings.typography.fontFamilies.

Why?

Addresses #52798

How?

By adding additional properties to the valid settings of the theme.json class.

Testing Instructions

See #52798.

Testing Instructions for Keyboard

Screenshots or screencast

[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/class-wp-theme-json-gutenberg.php
❔ phpunit/class-wp-theme-json-test.php

[jffng]

I am not sure if this is a great approach to add all of these properties to the VALID_SETTINGS array.

There is another public method of WP_Theme_JSON::remove_insecure_properties, but when I tried using it on the example data with bad keys, it unset the entire object.

[matiasbenedetto]

Hi! Thanks for working on this @jffng. :)
I tested this and it is working as expected.
LGTM

[matiasbenedetto]

For those testing following the instructions on #52798 remember you need to create a new WP_Theme_JSON_Gutenberg object instead of WP_Theme_JSON because this change is not in core yet.

[oandregal]

Nice one. Would you be able to provide a test case to make sure both the constructor and remove_insecure_properties work well?

There are a couple of examples in the existing test suite

gutenberg/phpunit/class-wp-theme-json-test.php

Line 847 in 4d2a308

public function test_remove_invalid_element_pseudo_selectors() {

[jffng]

@oandregal When writing unit tests, I realized a bug about the current state of this PR — the sanitization removes any fontFamilies and fontFaces after the first one defined.

I think this is because the remove_keys_not_in_schema function compares the keys of the arrays and doesn't specifically account for arrays with numeric keys that represent list-like structures.

Any ideas for how to handle this?

[oandregal]

Ah, I see, it removes all keys that are not 0.

One approach would be to expand the logic in the sanitization method for the list of "valid keys" within font families to have all the indexes present in the input:

{
  "...": {
    "fontFamilies": {
      "0": { "name": "...", "slug": "...", "...": "..." },
      "1": { "name": "...", "slug": "...", "...": "..." },
      "2": { "name": "...", "slug": "...", "...": "..." }
    }
  }
}

We do something similar for blocks, when we create the schema based on the blocks registered. The schema for the font family under the PRESETS_METADATA, for example (and we could add the schemas for other presets later).

A second approach that comes to mind would be to use a special sign as key to signal that "any key is allowed as child". For example, make the VALID_SETTINGS something like:

{
  "...": {
    "fontFamilies": {
      "*": { "name": "...", "slug": "...", "...": "..." }
    }
  }
}

and update sanitization to allow any key when * is used in the VALID_SETTINGS.

Finally, another approach would be to leverage the info already present in this other schema.

Does this help?

I don't feel strongly about any approach, other than making sure the way we do it can be reused for any other preset.

[jffng]

Thank you for the ideas! I implemented your first suggestion here: ba8f4de — it seems to be working well, the unit tests are now passing.

I also added a test to ensure the typography settings are sanitized correctly: 260d7cf

EDIT
It looks like there are some issues with the Tests_Fonts_WPFontsResolver_RegisterFontsFromThemeJson, I'll look into this with @matiasbenedetto next.

[matiasbenedetto]

EDIT It looks like there are some issues with the Tests_Fonts_WPFontsResolver_RegisterFontsFromThemeJson, I'll look into this with @matiasbenedetto next.

I guess those tests were implemented by @anton-vlasenko or @hellofromtonya maybe they can help with the issues.

[jffng]

While tying to fix the tests, I realized the shape of settings.typography.fontFamilies is different depending on the context:

With the existing fonts API, settings.typography.fontFamilies the input looks like:

[theme] => Array 
(
    [0] => Array
        (
            [fontFamily] => Lato
            [name] => Lato
            [slug] => lato
            [fontFace] => Array
                (
                    [lato-400-normal] => Array
                        (
                            [origin] => gutenberg_wp_fonts_api
                            [provider] => local
                            [fontFamily] => Lato
                            [fontStyle] => normal
                            [fontWeight] => 400
                            [fontDisplay] => fallback
                            [src] => https://example.com/tests/assets/fonts/lato/Lato-Regular.woff2
                        )
              ...

or

[theme] => Array 
(
    [0] => Array
        (
            [fontFace] => Array
                (
                    [0] => Array
                        (
                            [fontFamily] => DM Sans
                            [fontStretch] => normal
                            [fontStyle] => normal
                            [fontWeight] => 400
                            [src] => Array
                                (
                                    [0] => file:./assets/fonts/dm-sans/DMSans-Regular.woff2
                                )
                        )
...

With the soon-to-be introduced Font Library, the input is expected to look like this:

Array
(
	'fontFamily' => 'Piazzolla',
	'name'       => 'Piazzolla',
	'slug'       => 'piazzolla',
	'fontFace'   => Array
	(
		Array
		(
			'fontFamily' => 'Piazzolla',
			'fontStyle'  => 'italic',
			'fontWeight' => '400',
			'src'        => 'https://example.com/font.ttf',
		),
	),
)

Is there much benefit to change the sanitization function to allow all of these cases? It feels overly specific and prone to error.

Alternatively we can add sanitization directly to the new Font Library class.

[oandregal]

That's a good discovery, glad you were looking into this.

Is there much benefit to change the sanitization function to allow all of these cases? It feels overly specific and prone to error.

I reckon I don't know every detail of font family management, though I understand we need to consolidate into only one shape.

[jffng]

I excluded the fontsapi tests per #52704 (comment) but there are still some failing tests in Tests_Fonts_WPPrintFontFaces and Tests_Fonts_WPFontFaceResolver_GetFontsFromThemeJson. Will these tests still be relevant when the Fonts API is removed?

[hellofromtonya]

Currently looking into why the Font Face tests are failing.

[hellofromtonya]

there are still some failing tests in Tests_Fonts_WPPrintFontFaces and Tests_Fonts_WPFontFaceResolver_GetFontsFromThemeJson. Will these tests still be relevant when the Fonts API is removed?

@jffng Yes, these tests are still relevant and will remain. They are for Font Face, which replaces the Fonts API. It works with Font Library.

[hellofromtonya]

Why are the Font Face tests failing?

When WP_Font_Face_Resolver::get_fonts_from_theme_json() is called, there is no 'fontFamilies' in the $settings['typography']:

public static function get_fonts_from_theme_json() {
	$settings = WP_Theme_JSON_Resolver_Gutenberg::get_merged_data()->get_settings();

	// Bail out early if there are no font settings.
	if ( empty( $settings['typography'] ) || empty( $settings['typography']['fontFamilies'] ) ) {
		return array();
	}

The tests fail because it expects to receive the fonts from theme.json, but no fonts are returned (an empty array is returned).

Why is WP_Theme_JSON_Resolver_Gutenberg::get_merged_data()->get_settings(); not returning the fontFamilies?

Here's the test theme's theme.json https://github.com/WordPress/gutenberg/blob/trunk/phpunit/data/themedir1/fonts-block-theme/theme.json#L22.

[hellofromtonya]

Test Report

Env:

• Localhost: wp-env
• Theme: TT3
• Plugins: Gutenberg
• OS: macOS
• Browser: Firefox

Testing Instructions

1. Go to Site Editor.
2. Select the Styles icon to open the Styles panel.
3. Select Typography.
4. Select Headings.

Expected behavior: The Fonts option should be there.

Test Results

❌ The Fonts selector is not present.

@jffng after applying the changes in this PR, the Fonts option and dropdown is no longer present in Site Editor > Styles > Typography > Headings UI.

[jffng]

I updated this PR so all the tests are now passing and the above bug reports should no longer be happening, it should be ready for another review.

[ironprogrammer]

This works as advertised, and local PHP unit tests pass 👍🏻

Test Report

Includes test from @hellofromtonya's report above, as well as the direct sanitation check from #52798.

Steps to Test

1. UI: Check for the font picker under Styles > Typography > Headings > Typography.
2. Plugin: Check sanitation directly by installing this mu-plugin. Reload the site to view result. Remove plugin after testing.

Environment

• OS: macOS 13.5.2
• Browser: Safari 16.6
• Server: nginx/1.25.2
• PHP: 7.4.33
• WordPress: 6.4-alpha-56267-src
• Theme: twentytwentythree v1.2
• Active Plugins:
  • gutenberg v16.6.0 (this PR)
  • test-gb-52798 (test mu-plugin)

Actual Results

• ✅ UI: The font dropdown is visible/functional.
• ✅ Plugin: The invalid values were removed from the final theme data using the patched WP_Theme_JSON_Gutenberg class.

[mikachan]

As this PR hasn't been merged yet and the latest version of Gutenberg is due to be released today, I'm going to remove the Backport to Gutenberg RC label.

I'm going to add the Backport to WP Beta/RC label as the Font Library work is a "blessed" task for WordPress 6.4.

[github-actions]

Flaky tests detected in 0eea111.
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/6383699639
📝 Reported issues:

• [Flaky Test] User Mention: should allow newlines after multiple mention completions #48537 in /test/e2e/specs/editor/various/autocomplete-and-mentions.spec.js

[hellofromtonya]

The Font Face tests are failing because they are using Core's Font Face files which do not use WP_Theme_JSON_Gutenberg.

Seems Font Face files in Gutenberg need to be modified to use _Gutenberg for running GB specific code. I'll follow-up with a PR shortly to fix the failing tests.

Note: Once this PR is merged into Core, the tests will pass again. But the problem will resurface anytime there's a change in one of the files here in this repo that already exists in Core.

[hellofromtonya]

PR #54990 should resolve the failing tests. @ironprogrammer can you test to verify please?

[jffng]

Thanks everyone for the input, it helped inform a cleaner solution at #56447.