Add additional sanitization for settings.typography.fontFamilies #53273

jffng · 2023-08-02T16:08:31Z

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 · 2023-08-02T16:10:04Z

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 · 2023-08-02T16:22:18Z

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 · 2023-08-03T08:32:15Z

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 · 2023-08-03T18:07:09Z

@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 · 2023-08-04T07:34:29Z

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 · 2023-08-04T19:04:20Z

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 · 2023-08-04T19:38:49Z

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 · 2023-08-08T21:06:01Z

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 · 2023-08-09T12:17:04Z

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 · 2023-08-11T13:28:31Z

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 · 2023-08-16T19:58:57Z

Currently looking into why the Font Face tests are failing.

hellofromtonya · 2023-08-16T21:02:36Z

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 · 2023-08-16T21:07:52Z

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 · 2023-08-16T21:32:56Z

Test Report

Env:

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

Testing Instructions

Go to Site Editor.
Select the Styles icon to open the Styles panel.
Select Typography.
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 · 2023-09-21T19:36:23Z

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 · 2023-09-22T00:14:58Z

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

UI: Check for the font picker under Styles > Typography > Headings > Typography.
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.

lib/class-wp-theme-json-gutenberg.php

mikachan · 2023-09-27T11:10:21Z

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 · 2023-10-02T17:49:23Z

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 · 2023-10-02T20:42:48Z

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 · 2023-10-03T00:03:15Z

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

jffng · 2023-11-29T15:05:32Z

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

jffng added [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json labels Aug 2, 2023

jffng requested a review from matiasbenedetto August 2, 2023 16:08

jffng linked an issue Aug 2, 2023 that may be closed by this pull request

WP_Theme_JSON sanitization is not working below certain level of theme.json #52798

Closed

jffng marked this pull request as ready for review August 2, 2023 16:09

jffng requested a review from spacedmonkey as a code owner August 2, 2023 16:09

jffng added the [Type] Bug An existing feature does not function as intended label Aug 2, 2023

matiasbenedetto approved these changes Aug 2, 2023

View reviewed changes

matiasbenedetto reviewed Aug 2, 2023

View reviewed changes

matiasbenedetto requested a review from oandregal August 2, 2023 16:51

aristath approved these changes Aug 3, 2023

View reviewed changes

anton-vlasenko self-requested a review August 10, 2023 16:29

jffng force-pushed the fix/theme-json-fonts-validation branch from b0c6e6e to 8067f83 Compare September 18, 2023 21:41

jffng added the Needs PHP backport Needs PHP backport to Core label Sep 19, 2023

jffng added the Backport to Gutenberg RC Pull request that needs to be backported to a Gutenberg release candidate (RC) label Sep 21, 2023

jffng commented Sep 22, 2023

View reviewed changes

lib/class-wp-theme-json-gutenberg.php Outdated Show resolved Hide resolved

mikachan added Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta and removed Backport to Gutenberg RC Pull request that needs to be backported to a Gutenberg release candidate (RC) labels Sep 27, 2023

jffng force-pushed the fix/theme-json-fonts-validation branch from 87a6834 to 1169e19 Compare October 2, 2023 17:09

ironprogrammer assigned ironprogrammer and unassigned ironprogrammer Oct 2, 2023

hellofromtonya mentioned this pull request Oct 3, 2023

Font Face: Adds a GB specific resolver to capture changes not yet in Core #54990

Open

mikachan removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Oct 9, 2023

jffng and others added 8 commits November 14, 2023 09:50

Add additional keys to valid typography.fontFamily settings.

b98ac47

Expand sanitization logic for fontFamilies and fontFaces.

2e2589e

Add test for typography sanitization.

128566c

Format php.

01a284f

Fix case where a source is set.

108e6fa

Format php.

a6502ee

Fix bug in flattening function and expand test case.

222fa33

Run phpcbf on changes

2493483

jffng force-pushed the fix/theme-json-fonts-validation branch from 0eea111 to 2493483 Compare November 14, 2023 14:51

Handle case where fontFamilies have an origin e.g. theme or custom.

6d80919

matiasbenedetto mentioned this pull request Nov 15, 2023

Font Library: run wp_font_family post content sanitization using wp_insert_post_data filter #56172

Closed

Merge branch 'trunk' into fix/theme-json-fonts-validation

2ea6651

matiasbenedetto mentioned this pull request Nov 22, 2023

WP_Theme_JSON_Gutenberg: Add nested indexed array schema sanitization #56447

Merged

jffng closed this Nov 29, 2023

oandregal deleted the fix/theme-json-fonts-validation branch November 29, 2023 15:43

youknowriad removed the Needs PHP backport Needs PHP backport to Core label Feb 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add additional sanitization for settings.typography.fontFamilies #53273

Add additional sanitization for settings.typography.fontFamilies #53273

jffng commented Aug 2, 2023 •

edited

Loading

github-actions bot commented Aug 2, 2023 •

edited

Loading

jffng commented Aug 2, 2023 •

edited

Loading

matiasbenedetto left a comment

matiasbenedetto left a comment

oandregal commented Aug 3, 2023

jffng commented Aug 3, 2023

oandregal commented Aug 4, 2023

jffng commented Aug 4, 2023 •

edited

Loading

matiasbenedetto commented Aug 4, 2023

jffng commented Aug 8, 2023 •

edited

Loading

oandregal commented Aug 9, 2023

jffng commented Aug 11, 2023

hellofromtonya commented Aug 16, 2023

hellofromtonya commented Aug 16, 2023

hellofromtonya commented Aug 16, 2023

hellofromtonya commented Aug 16, 2023

jffng commented Sep 21, 2023

ironprogrammer commented Sep 22, 2023

mikachan commented Sep 27, 2023

github-actions bot commented Oct 2, 2023 •

edited

Loading

hellofromtonya commented Oct 2, 2023

hellofromtonya commented Oct 3, 2023

jffng commented Nov 29, 2023

Add additional sanitization for settings.typography.fontFamilies #53273

Add additional sanitization for settings.typography.fontFamilies #53273

Conversation

jffng commented Aug 2, 2023 • edited Loading

What?

Why?

How?

Testing Instructions

Testing Instructions for Keyboard

Screenshots or screencast

github-actions bot commented Aug 2, 2023 • edited Loading

jffng commented Aug 2, 2023 • edited Loading

matiasbenedetto left a comment

Choose a reason for hiding this comment

matiasbenedetto left a comment

Choose a reason for hiding this comment

oandregal commented Aug 3, 2023

jffng commented Aug 3, 2023

oandregal commented Aug 4, 2023

jffng commented Aug 4, 2023 • edited Loading

matiasbenedetto commented Aug 4, 2023

jffng commented Aug 8, 2023 • edited Loading

oandregal commented Aug 9, 2023

jffng commented Aug 11, 2023

hellofromtonya commented Aug 16, 2023

hellofromtonya commented Aug 16, 2023

hellofromtonya commented Aug 16, 2023

hellofromtonya commented Aug 16, 2023

Test Report

Testing Instructions

Test Results

jffng commented Sep 21, 2023

ironprogrammer commented Sep 22, 2023

Test Report

Steps to Test

Environment

Actual Results

mikachan commented Sep 27, 2023

github-actions bot commented Oct 2, 2023 • edited Loading

hellofromtonya commented Oct 2, 2023

hellofromtonya commented Oct 3, 2023

jffng commented Nov 29, 2023

jffng commented Aug 2, 2023 •

edited

Loading

github-actions bot commented Aug 2, 2023 •

edited

Loading

jffng commented Aug 2, 2023 •

edited

Loading

jffng commented Aug 4, 2023 •

edited

Loading

jffng commented Aug 8, 2023 •

edited

Loading

github-actions bot commented Oct 2, 2023 •

edited

Loading