Trac 59165: Add Font Face and its tests #5051

hellofromtonya · 2023-08-22T18:19:10Z

Adds the new Font Face, a server-side @font-face styles generator and printer. Code is copied from Gutenberg.

Trac ticket: https://core.trac.wordpress.org/ticket/59165

How to Test

Scenario 1 uses the Twenty Twenty-Three (TT3) theme. Testing instructions are provided here in the Trac ticket.

Scenario 2 is a classic site with a plugin that passes an array of fonts when invoking wp_print_font_faces(). Testing instructions are provided here in the Trac ticket.

For Code Reviewers

Font Face was developed in Gutenberg. The vast majority of its code comes from well-established font processing developed in multiple iterations of the Webfonts, Web Fonts, and Fonts API. You can find:

the source code here in Gutenberg's lib/compat/wordpress-6.4/fonts/ directory https://github.com/WordPress/gutenberg/tree/trunk/lib/compat/wordpress-6.4/fonts.
PHPUnit tests live here https://github.com/WordPress/gutenberg/tree/trunk/phpunit/tests/fonts/font-face.

Changes made in this PR will be copied / merged back into Gutenberg.

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

hellofromtonya · 2023-08-23T14:58:25Z

Need to merge WordPress/gutenberg#53805 to use wp_get_global_settings() instead of the private API. Commit will be pushed shortly.

Update: Done ✅ via e1da7ab.

Props @oandregal.

@oandregal

Changes the resolver to use wp_get_global_settings() instead of the chained objects to get the merged data layer. Reference: WordPress/gutenberg#53805 Props @oandregal.

hellofromtonya · 2023-08-23T16:13:11Z

src/wp-includes/fonts/fonts.php

+ null === $wp_font_face ||
+
+ // Ignore cache when automated test suites are running.
+ ( defined( 'WP_RUN_CORE_TESTS' ) && WP_RUN_CORE_TESTS )


The intent:

Use a new instance of WP_Font_Face for each PHPUnit test. Why? To avoid test-to-test interactions by ensuring a new object is used for each test.

Else, it acts as a singleton whether in WP_DEBUG, prod, or development modes.

Do the tests pass with a combination of @runInSeparateProcess and @preserveGlobalState disabled in the test docblocks?

If so, while it would slow down the tests, it removes the need for this test-related source code and separates implemention from the test suite. It may be worth considering if it hasn't been already.

All of the Font Face tests would require running in their own separate processes. What would the test performance impact be?

The code now: 00:05.447

Running in separate processes: 00:36.257

That's fairly significant for 20 tests. Ouch.

Whoopsie, I was wrong. Only the global function tests (i.e. for wp_print_font_faces()) need to run in a separate process, not all of the tests.

Re-running the fonts group tests with the wp_print_font_faces() tests in separate processes: 00:16.807

Adds about 11 seconds with the current tests, i.e. more unhappy tests are coming.

What do you think @costdev?

Colin and I chatted in Slack. Commit #5051 runs the tests in separate processes.

Results: adds ~11 seconds to the run time of the test suites 😢 , but removes the test constant from the source code 😄 .

hellofromtonya · 2023-08-23T16:19:19Z

src/wp-includes/fonts/font-face/class-wp-font-face-resolver.php

+ * @return array Returns the font-families, each with their font-face variations.
+ */
+ public static function get_fonts_from_theme_json() {
+ $settings = wp_get_global_settings();


@oandregal I merged in WordPress/gutenberg#53805 and updated to use Core's wp_get_global_settings().

felixarntz

@hellofromtonya Mostly the PR looks solid to me. I have a few points of feedback below.

One broader nit-pick, I'm not sure about the file organization of the new files, I think all the nested folders are unnecessary and don't fit well with the way core typically organizes files. I would instead advise the following structure:

wp-includes/fonts directory for the new classes (no additional subdirectories)
wp-includes/fonts.php file for the global function(s)

src/wp-includes/fonts/font-face/class-wp-font-face.php

felixarntz · 2023-08-24T14:55:44Z

src/wp-includes/fonts/font-face/class-wp-font-face.php

+
+ // Check the font-family.
+ if ( empty( $font_face['font-family'] ) || ! is_string( $font_face['font-family'] ) ) {
+ trigger_error( 'Font font-family must be a non-empty string.' );


Why is this class using trigger_error() (here and below)? Core should always use the appropriate wrapper functions, such as _doing_it_wrong(), so that e.g. WP_DEBUG is respected.

This change comes from @azaozz, who advise only using _doing_it_wrong() for the most severe cases. Since then, trigger_error() has been used for things like function/method parameter (input) validation. It exists in Core for things like PHP compat changes.

An architectural proposal for handling input validation (i.e. function/method parameter checks) is in the works which will also include new error handling.

Also noting the code being replaced also used trigger_error() for these same validation checks.

Note: Here's a Trac ticket that proposes introducing wp_trigger_error() to compliment _doing_it_wrong() https://core.trac.wordpress.org/ticket/57686. @azaozz shares reasoning of why not use _doing_it_wrong() for these purposes:

The _doing_it_wrong() function was introduced 12 years ago .. for "telling people the code they have written won't work"...

However that convenience has resulted in shifting the initial purpose, which was to tell developers they are doing something wrong. Lately _doing_it_wrong() seems to be used in any context, not specifically to "tell off" the developers and get them to fix their code.

@hellofromtonya Thank you for the additional context. I may be on board with a new function like wp_trigger_error(), but since that is not in place yet, I think the usage here should rather rely on _doing_it_wrong(), since in those cases it literally is the developer "doing it wrong" rather than something else leading to the problem.

As you mention, trigger_error() is currently mostly used in Core for things that deal with PHP compat, which is not the case here, and calling trigger_error() ignores WP_DEBUG etc constants which is the main reason I object to this change.

I think we should use _doing_it_wrong(), and then later we can update it to use wp_trigger_error() once that function has been added to Core (potentially immediately as part of https://core.trac.wordpress.org/ticket/57686).

calling trigger_error() ignores WP_DEBUG

Yea, lets just add wp_trigger_error(). I know there are ideas to expand/extend its use in the future, but seems is it needed now :)

Yea, lets just add wp_trigger_error(). I know there are ideas to expand/extend its use in the future, but seems is it needed now :)

Sorry for my delay in responding (been sick).

Thanks @felixarntz and @azaozz. I think you both make good points.

I moved Trac 57686 back into the 6.4 milestone to add wp_trigger_error(). I agree with you @azaozz that it can be modified in the future when it's time to implement input (function/method parameter) validation.

For this PR as a compromise, I'll switch trigger_error() to _doing_it_wrong() with a comment to replace it with wp_trigger_error().

Done in commit e695f6f ✅

src/wp-includes/fonts/fonts.php

src/wp-includes/fonts/font-face/class-wp-font-face.php

hellofromtonya · 2023-08-24T15:13:04Z

One broader nit-pick, I'm not sure about the file organization of the new files, I think all the nested folders are unnecessary and don't fit well with the way core typically organizes files. I would instead advise the following structure:

wp-includes/fonts directory for the new classes (no additional subdirectories)
wp-includes/fonts.php file for the global function(s)

Hello @felixarntz 👋 Thank you for your review!

The thinking around the file organization is to group the different fonts management components in sub-directories, which as of today is Font Face and Font Library. There's a mix of approaches in Core, though predominately there is as you note a tendency to group all like files in the "group" folder, rather than sub directories.

I'm okay with either approach. When Font Library lands, it would also go into the wp-includes/fonts/ directory too.

Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>

hellofromtonya · 2023-08-24T15:34:36Z

@felixarntz commit e2db981 relocates the wp-includes/fonts/font-face files into its root wp-includes/fonts/ per your suggestion ✅

felixarntz · 2023-08-24T18:07:42Z

@hellofromtonya Thank you for the updates, also for updating the file/folder structure. What about moving the functions file fonts.php directly into wp-includes, i.e. wp-includes/fonts.php? I think that ties in better with how Core organizes files, it is common to have general global function files in there and then specific classes for a feature in a directory. For example, that is similarly handled for the REST API, which uses wp-includes/rest-api.php for functions and the wp-includes/rest-api directory for classes.

costdev

Nothing major, just a few thoughts/queries in this review.

I'll get this tested today. 🙂

costdev · 2023-08-25T05:39:38Z

src/wp-includes/fonts/class-wp-font-face.php

+ // Wrap font-family in quotes if it contains spaces
+ // and is not already wrapped in quotes.


Suggested change

// Wrap font-family in quotes if it contains spaces

// and is not already wrapped in quotes.

/*

* Wrap font-family in quotes if it contains spaces

* and is not already wrapped in quotes.

*/

See the handbook entry for how Core formats multi-line comments.

Thank you @costdev! Done in 4a1d4c2 ✅

costdev · 2023-08-25T06:13:57Z

src/wp-includes/fonts/class-wp-font-face.php

+ }
+
+ // Validate the 'src' property.
+ if ( ! empty( $font_face['src'] ) ) {


The previous lines ensure that this can't be empty by now. Suggest removing this condition.

Thank you @costdev! Done in 4a1d4c2 ✅

costdev · 2023-08-25T06:19:01Z

src/wp-includes/fonts/class-wp-font-face.php

+
+ // Check the font-display.
+ if ( ! in_array( $font_face['font-display'], $this->valid_font_display, true ) ) {
+ $font_face['font-display'] = $this->font_face_property_defaults['font-display'];


It might be helpful to trigger an error or _doing_it_wrong() here to indicate an invalid font-display value.

Hmm, it's not a doing it wrong, as an invalid value will use the default instead and the processing continues (unlike the other validation above where validation fails and it bails out).

Looking at Core, other loosely similar checks do not inform the developer either. That makes me wonder .. Should Core alert that it is using a default value if the given value fails its validation? Hmm, interesting 🤔. It might be helpful to developers to help them debug.

If this were to throw a notice, likely should wrap that notice in empty( trim () ) to avoid the notice on an empty string value.

As this isn't a _doing_it_wrong() and wp_trigger_error() does not yet exist in Core, this helper message could be added later once wp_trigger_error() is introduced. It's definitely worth a good think and discussion.

costdev · 2023-08-25T06:19:47Z

src/wp-includes/fonts/class-wp-font-face.php

+ // Remove invalid properties.
+ foreach ( $font_face as $prop => $value ) {
+ if ( ! in_array( $prop, $this->valid_font_face_properties, true ) ) {
+ unset( $font_face[ $prop ] );


See above regarding indicating invalid properties/values.

Thank you @costdev! Done in 4a1d4c2 ✅

costdev · 2023-08-25T06:36:44Z

src/wp-includes/fonts/class-wp-font-face.php

+ printf(
+ $this->get_style_element(),
+ $this->get_css( $fonts )
+ );


As $fonts can come from anywhere, we should escape here to avoid evils slipping through from the ::get_css() callstack - happy to discuss details in DM.

Hmm, as the CSS string is within the style element, the nefarious code would need to inject </style> before its evil code. Alrighty, then tags need to be stripped from the CSS string to ensure that doesn't happen. wp_strip_all_tags() should be sufficient.

BTW: This is the approach used in TT0 and TT1 for their CSS auto-generator.

For example, here's TT1:

function twenty_twenty_one_generate_css( $selector, $style, $value, $prefix = '', $suffix = '', $display = true ) { // Bail early if there is no $selector elements or properties and $value. if ( ! $value || ! $selector ) { return ''; } $css = sprintf( '%s { %s: %s; }', $selector, $style, $prefix . $value . $suffix ); if ( $display ) { /* * Note to reviewers: $css contains auto-generated CSS. * It is included inside <style> tags and can only be interpreted as CSS on the browser. * Using wp_strip_all_tags() here is sufficient escaping to avoid * malicious attempts to close </style> and open a <script>. */ echo wp_strip_all_tags( $css ); // phpcs:ignore WordPress.Security.EscapeOutput } return $css; }

Added a test for it.

Using this data:

$fonts = array( 'Source Serif Pro' => array( array( 'src' => array( 'http://example.com/assets/source-serif-pro/SourceSerif4Variable-Roman.ttf.woff2' ), 'font-family' => 'Source Serif Pro', 'font-style' => 'normal', 'font-weight' => '200 900', 'font-stretch' => '</style><script>console.log("Hello")</script><style>', ), ), );

Before wp_strip_all_tags( $css ):

<style id='wp-fonts-local' type='text/css'> @font-face{font-family:"Source Serif Pro";font-style:normal;font-weight:200 900;font-display:fallback;src:url('http://example.com/assets/source-serif-pro/SourceSerif4Variable-Roman.ttf.woff2') format('woff2');font-stretch:</style><script>console.log("Hello")</script><style>;} </style>

After wp_strip_all_tags( $css ):

<style id='wp-fonts-local' type='text/css'> @font-face{font-family:"Source Serif Pro";font-style:normal;font-weight:200 900;font-display:fallback;src:url('http://example.com/assets/source-serif-pro/SourceSerif4Variable-Roman.ttf.woff2') format('woff2');font-stretch:;} </style>

Excellent catch @costdev 👏

Added wp_strip_all_tags( $css ) in the source code via commit e90b21d ✅

Added a test for it in commit b4bcd07 ✅

costdev · 2023-08-25T06:44:37Z

src/wp-includes/fonts/class-wp-font-face-resolver.php

+ $settings = wp_get_global_settings();
+
+ // Bail out early if there are no font settings.
+ if ( empty( $settings['typography'] ) || empty( $settings['typography']['fontFamilies'] ) ) {


Shouldn't the second condition cover the first?

Actually, the second condition is all that's needed as it'll catch both conditions https://3v4l.org/b6UYn.

Done in 4a1d4c2 ✅

costdev · 2023-08-25T06:51:56Z

src/wp-includes/fonts/class-wp-font-face.php

+ * @since 6.4.0
+ *
+ * @param array $font_face Font face properties to validate.
+ * @return false|array Validated font-face on success. Else, false.


Suggested change

* @return false|array Validated font-face on success. Else, false.

* @return array|false Validated font-face on success. Otherwise false.

Swapped return types. You pessimist! 😂

"Otherwise false" is Core's usual phrasing.

Also, since this validates both properties and values, and returns the validated font face on success rather than valid properties, does it make more sense to call this validate_font_face()?

Good catch on the swapped return types. Whoopsie.

"Otherwise false" is Core's usual phrasing.

Hmm, there's a mixture of phrasing in Core:

. Otherwise false

[the success], false on failure.

, otherwise false.'

, or false on failure.

I think I'll change it to the last one.

Done in 4a1d4c2 ✅

Also, since this validates both properties and values, and returns the validated font face on success rather than valid properties, does it make more sense to call this validate_font_face()?

I think that method name would be confusing due to:

The font-face array contains a collection of @font-face property / value pairings that relate directly to the CSS.

The method above it called validate_fonts().

properties here is a verbose attempt to infer property / value pairs.

Other more accurate names could be:

validate_font_face_property_value_pairs()

validate_font_face_declarations() <= the proper name for a property / value pair

@costdev Do you find properties to be confusing? Is either of the above more understandable?

Thanks for the clarification!

I think properties while verbose was also a bit vague as the code validates properties and values.

validate_font_face_declarations() makes more sense to me as it encompasses both, so there's no surprises when seeing the internal code.

Done in commit c8e4c03 ✅

Also updated the DocBlock to ensure "declaration" is well understood.

costdev · 2023-08-25T06:57:27Z

src/wp-includes/fonts/class-wp-font-face.php

+ }
+
+ // Remove invalid properties.
+ foreach ( $font_face as $prop => $value ) {


Nit: IMO, there's no need to abbreviate to "prop" here. "property" and "properties" are used in documentation, as well as property and method names throughout.

Note the coding standards on naming conventions:

Don’t abbreviate variable names unnecessarily; let the code be unambiguous and self-documenting.

Thank you @costdev! Done in 4a1d4c2 ✅

costdev · 2023-08-25T07:04:27Z

src/wp-includes/fonts/class-wp-font-face.php

+ }
+
+ /**
+ * Gets the `<style>` element for wrapping the `@font-face` CSS.


Suggested change

* Gets the `<style>` element for wrapping the `@font-face` CSS.

* Gets the style element for wrapping the font-face CSS.

The documentation standards state:

No HTML markup or Markdown of any kind should be used in the summary. If the text refers to an HTML element or tag, then it should be written as “image tag” or “img” element, not “< img >“. (spaces added by me)
Reference

🔢 Applies elsewhere.

Thank you @costdev! Done in 4a1d4c2 ✅

anton-vlasenko

Please find my notes below.
Thank you.

src/wp-includes/deprecated.php

anton-vlasenko · 2023-08-25T19:16:05Z

src/wp-includes/fonts/class-wp-font-face.php

+ * @param array[][] $fonts Optional. The font-families and their font variations.
+ * See {@see wp_print_font_faces()} for the supported fields.
+ * Default empty array.
+ * }


Nitpick: this curly brace doesn't seem to be necessary here.

Suggested change

* }

*

Thank you @anton-vlasenko! Done in 4a1d4c2 ✅

1. Uses _doing_it_wrong() until wp_trigger_error() is available. 2. Adds comment to each with `@todo` annotation as a reminder to replace these later.

felixarntz

@hellofromtonya This looks solid to me now. Other than the oustanding minor quirks outlined by other reviewers, I don't have any blockers on this one. Excited to see it land!

It would be great to do a performance comparison for how this does compared to the old private code, just to ensure there's no unexpected regression.

costdev

Thanks for the updates @hellofromtonya! This looks good to me!

hellofromtonya · 2023-08-31T21:49:03Z

Committed via https://core.trac.wordpress.org/changeset/53282. Thank you everyone for your contributions!

hellofromtonya added 2 commits August 22, 2023 14:18

Add Font Face and its tests

5fa6ac0

Deprecates stopgap code

f9b020d

hellofromtonya force-pushed the try/font-face branch from 61dc248 to f9b020d Compare August 22, 2023 19:27

hellofromtonya marked this pull request as ready for review August 22, 2023 22:15

hellofromtonya added 2 commits August 23, 2023 09:59

Use wp_get_global_settings() instead of private API

e1da7ab

Changes the resolver to use wp_get_global_settings() instead of the chained objects to get the merged data layer. Reference: WordPress/gutenberg#53805 Props @oandregal.

Details fonts array structure

eebeb2d

hellofromtonya requested review from costdev, oandregal and felixarntz August 23, 2023 15:56

hellofromtonya commented Aug 23, 2023

View reviewed changes

felixarntz requested changes Aug 24, 2023

View reviewed changes

hellofromtonya and others added 4 commits August 24, 2023 10:23

Reference @fonts param definition

f6c4266

Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>

Remove fonts param explanation line

31a3508

Remove "wp_font_face_property_defaults" filter

4db7d42

Relocate into fonts/

e2db981

hellofromtonya requested a review from felixarntz August 24, 2023 15:36

costdev requested changes Aug 25, 2023

View reviewed changes

anton-vlasenko reviewed Aug 25, 2023

View reviewed changes

hellofromtonya added 2 commits August 30, 2023 15:07

Move fonts.php to wp-includes/

1b2bc1c

Replace trigger_error()

e695f6f

1. Uses _doing_it_wrong() until wp_trigger_error() is available. 2. Adds comment to each with `@todo` annotation as a reminder to replace these later.

felixarntz approved these changes Aug 30, 2023

View reviewed changes

hellofromtonya added 3 commits August 30, 2023 16:18

Code reviews from Anton and Colin

4a1d4c2

Escape CSS for all tags

e90b21d

CSS: strip all tags before printing

b4bcd07

hellofromtonya requested a review from costdev August 30, 2023 22:17

Rename properties to declarations

c8e4c03

costdev approved these changes Aug 30, 2023

View reviewed changes

Run tests in separate processes

0fd304e

hellofromtonya closed this Aug 31, 2023

hellofromtonya deleted the try/font-face branch September 1, 2023 02:44

mikachan mentioned this pull request Sep 5, 2023

Plugin: Synchronize PHP changes for WordPress 6.4 release WordPress/gutenberg#54177

Closed

86 tasks

hellofromtonya mentioned this pull request Sep 25, 2023

Font Library #5285

Closed

		// Wrap font-family in quotes if it contains spaces
		// and is not already wrapped in quotes.

	* @return false\|array Validated font-face on success. Else, false.
	* @return array\|false Validated font-face on success. Otherwise false.

	* Gets the `<style>` element for wrapping the `@font-face` CSS.
	* Gets the style element for wrapping the font-face CSS.

Trac 59165: Add Font Face and its tests #5051

Trac 59165: Add Font Face and its tests #5051

Conversation

hellofromtonya commented Aug 22, 2023 • edited Loading

How to Test

For Code Reviewers

hellofromtonya commented Aug 23, 2023 • edited Loading

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

felixarntz left a 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

azaozz Aug 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hellofromtonya commented Aug 24, 2023

hellofromtonya commented Aug 24, 2023

felixarntz commented Aug 24, 2023

costdev left a comment

Choose a reason for hiding this comment

costdev Aug 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

costdev Aug 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hellofromtonya Aug 30, 2023 • edited Loading

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

Choose a reason for hiding this comment

hellofromtonya Aug 30, 2023 • edited Loading

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

costdev Aug 25, 2023 • edited Loading

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

anton-vlasenko left a 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

felixarntz left a comment

Choose a reason for hiding this comment

costdev left a comment

Choose a reason for hiding this comment

hellofromtonya commented Aug 31, 2023

hellofromtonya commented Aug 22, 2023 •

edited

Loading

hellofromtonya commented Aug 23, 2023 •

edited

Loading

azaozz Aug 25, 2023 •

edited

Loading

costdev Aug 25, 2023 •

edited

Loading

costdev Aug 25, 2023 •

edited

Loading

hellofromtonya Aug 30, 2023 •

edited

Loading

hellofromtonya Aug 30, 2023 •

edited

Loading

costdev Aug 25, 2023 •

edited

Loading