Fix: WP_Font_Face: Font names that contain single quotes are not wrapped in double quotes #8982

iamsandeepdahiya · 2025-06-15T11:12:05Z

I have created a patch for the following Trac ticket:
Trac ticket: https://core.trac.wordpress.org/ticket/63568

Details for the bug reproduction can be found here.

In this patch, the quotes are added by using the logic of maybe_add_quotes method in the build_font_face_css function.

The comparison of input and output before the patch and after the patch is:

Here is the before patch (bug reproduction video):-
https://github.com/user-attachments/assets/d93736c0-d410-46da-84fa-38900f843471

After patch (test report video):-
https://github.com/user-attachments/assets/4a610657-5c09-4de1-92a2-f161c9910812

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.

github-actions · 2025-06-15T11:12:14Z

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 Committers: Use this line as a base for the props when committing in SVN:

Props sandeepdahiya, wildworks, jonsurrell.

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

github-actions · 2025-06-15T11:24:38Z

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

The Plugin and Theme Directories cannot be accessed within Playground.
All changes will be lost when closing a tab with a Playground instance.
All changes will be lost when refreshing the page.
A fresh instance is created each time the link below is clicked.
Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

t-hamano · 2025-06-16T06:30:11Z

Thanks for the PR!

I'm thinking it might be better to expose the maybe_add_quotes method rather than duplicating it. Because the method belongs to a utility class.

That way, it should be available in the form of WP_Font_Utils::maybe_add_quotes().

iamsandeepdahiya · 2025-06-16T06:46:52Z

I'm thinking it might be better to expose the maybe_add_quotes method rather than duplicating it. Because the method belongs to a utility class.

To me, this seems better choice as well without duplication/repeatations.

t-hamano · 2025-06-16T07:30:01Z

Thanks for the update! I think we can commit this PR once the PHP lint errors are fixed.

sirreal · 2025-06-16T11:32:58Z

It seems good to always process the provided font family name, however I wonder whether we should improve on maybe_add_quotes to something like normalize_quoted_font_family_name.

I believe there are a number of edge cases that are unhandled here. Most of these are edge cases and not likely to appear in popular font names, however it seems ideal if the system behaves well under any conditions. For example, what happens if the string contains " instead of '?

This section of the CSS standard is interesting and relevant, in particular the notes:

means most punctuation characters and digits at the start of each token must be escaped in unquoted font family names.
if you really have a font whose name is the same as one of the names, or the system font names, or the CSS-wide keywords, it must be quoted.

I'd like to consider always using the quoted form of the font family name. This way they are handeled as CSS strings, and handling a few characters should ensure that font names are correctly represented in the resulting CSS.

A quoted form of the name seems ideal where some escaping inside the quoted CSS string would be necessary, namely:

Change \ to \\
Change the quote character to its escaped form, so if " is the quote used then " inside the string becomes \".
Newlines should become \A .

I'm not familiar with the fonts API so this may be problematic in some ways I'm unaware of, but from the CSS perspective this should be more robust.

sirreal · 2025-06-16T11:35:09Z

My comment is not intended to block this PR which seems to be an incremental improvement.

t-hamano · 2025-06-16T12:11:28Z

@sirreal Thanks for the feedback!

It seems good to always process the provided font family name, however I wonder whether we should improve on maybe_add_quotes to something like normalize_quoted_font_family_name.

I think this is a nice suggestion. This method was previously private, it should be safe to change the method name.

I believe there are a number of edge cases that are unhandled here.

This is true. There are also no unit tests and they will need to be added, but I think those can be addressed in a follow-up.

iamsandeepdahiya · 2025-06-17T03:51:39Z

I believe there are a number of edge cases that are unhandled here. Most of these are edge cases and not likely to appear in popular font names, however it seems ideal if the system behaves well under any conditions. For example, what happens if the string contains " instead of '?

Yes,  there’s zero downside to being more defensive here.

I think this is a nice suggestion. This method was previously private, it should be safe to change the method name.

I agree, this is a good suggestion. I've gone ahead and added some edge cases as mentioned above by @sirreal and renamed the method to normalize_quoted_font_family_name since it better reflects what the function is doing now.

t-hamano · 2025-06-17T08:16:48Z

@iamsandeepdahiya Thanks for the update. If we change the logic of the method itself, I feel like we need unit testing to prevent regressions. This method is very hard to test manually.

sirreal

Thanks for the additional changes @iamsandeepdahiya.

I'm concerned that making these additional changes is going to be much more involved. Ultimately, it seems good to do, but it may be a good idea to land a simple fix like you initially proposed in this PR. My recommendation would be to land the simpler fix and then create a new PR with the additional improvements.

Specifically, I agree with @t-hamano, this functionality absolutely needs testing and that's going to be a big addition to this PR. I've flagged several potential issues in this review that suggest some good test cases.

sirreal · 2025-06-17T08:15:05Z

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

@@ -29,12 +29,15 @@ class WP_Font_Utils {
 	 * @param string $item A font family name.
 	 * @return string The font family name with surrounding quotes, if necessary.
 	 */
-	private static function maybe_add_quotes( $item ) {
+	public static function normalize_quoted_font_family_name( $item ) {


I know I suggested this name, but considering again I'd prefer not to include "quoted" here, that seems like an implementation detail of normalization and this function could choose not to use quotes.

This seems preferable, indicating that it's a font family name for CSS which has different encoding rules than other contexts:

Suggested change

public static function normalize_quoted_font_family_name( $item ) {

public static function normalize_css_font_family_name( $item ) {

The description of this function is now incorrect and needs to be updated about its behavior.

sirreal · 2025-06-17T08:32:08Z

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

 		// Matches strings that are not exclusively alphabetic characters or hyphens, and do not exactly follow the pattern generic(alphabetic characters or hyphens).
 		$regex = '/^(?!generic\([a-zA-Z\-]+\)$)(?!^[a-zA-Z\-]+$).+/';
 		$item  = trim( $item );
 		if ( preg_match( $regex, $item ) ) {
 			$item = trim( $item, "\"'" );
+			$item = str_replace( [ "\r\n", "\r", "\n" ], '\\A', $item );


Good catch on normalizing the newlines and replacing them.

However the replacement may be problematic because it could be followed by hex digits which would then be interpreted as part of the escape sequence. The safest thing to do here is to include a trailing space character to terminate the escape sequence:

Suggested change

$item = str_replace( [ "\r\n", "\r", "\n" ], '\\A', $item );

$item = str_replace( [ "\r\n", "\r", "\n" ], '\\A ', $item );

sirreal · 2025-06-17T08:33:19Z

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

 		// Matches strings that are not exclusively alphabetic characters or hyphens, and do not exactly follow the pattern generic(alphabetic characters or hyphens).
 		$regex = '/^(?!generic\([a-zA-Z\-]+\)$)(?!^[a-zA-Z\-]+$).+/';
 		$item  = trim( $item );
 		if ( preg_match( $regex, $item ) ) {
 			$item = trim( $item, "\"'" );
+			$item = str_replace( [ "\r\n", "\r", "\n" ], '\\A', $item );
+			$item = str_replace( '\\', '\\\\', $item );


The order of these replacements is very important. The \ replacement must happen first or we risk replacing the \ characters that have been inserted. We don't want to replace a newline character with \A only to later replace that with \\A.

sirreal · 2025-06-17T08:35:06Z

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

 		// Matches strings that are not exclusively alphabetic characters or hyphens, and do not exactly follow the pattern generic(alphabetic characters or hyphens).
 		$regex = '/^(?!generic\([a-zA-Z\-]+\)$)(?!^[a-zA-Z\-]+$).+/';
 		$item  = trim( $item );
 		if ( preg_match( $regex, $item ) ) {
 			$item = trim( $item, "\"'" );
+			$item = str_replace( [ "\r\n", "\r", "\n" ], '\\A', $item );
+			$item = str_replace( '\\', '\\\\', $item );
+			$item = str_replace( '"', '\\"', $item );


We could decide to pick a quote character that avoids the need to escape by checking for ' and " characters inside the string. That adds some slight complication here but is potentially nicer in the result.

Stale review.

t-hamano · 2025-06-17T08:46:06Z

My recommendation would be to land the simpler fix and then create a new PR with the additional improvements.

I agree with this suggestion. I think it would be better to make only minimal changes here and address the logic changes and adding unit tests in a follow-up.

The feedback left by @sirreal will be useful when creating a follow-up PR. Looking at the feedback, it seems that there are many edge cases that need to be tested carefully 😅

This reverts commit 9f29537.

iamsandeepdahiya · 2025-06-17T09:02:05Z

I agree with this suggestion. I think it would be better to make only minimal changes here and address the logic changes and adding unit tests in a follow-up.

I have reverted the last commit as discussed and the recent changes have been undone. The branch is now back to the previous state. Please let me know if any further adjustments are needed. ✅

t-hamano

@iamsandeepdahiya Thanks for the update!

Can you delete this white space?
Personally, I think it's okay to include the method name change in this PR: from maybe_add_quotes to normalize_css_font_family_name

t-hamano

LGTM 👍

sirreal · 2025-06-18T09:24:46Z

Please don't wait for additional review from me, I'm not familiar with the font system and am satisfied with @t-hamano's approval.

fix:build-font-face-css single inverted comma in name issue

201d1cf

removed whitespace from the end

08ad65b

using WP_Font_Utils::maybe_add_quotes() after exposing it public

f09ea75

t-hamano self-requested a review June 16, 2025 07:30

t-hamano previously approved these changes Jun 16, 2025

View reviewed changes

renamed to normalize_quoted_font_family_name and added more edge cases

9f29537

iamsandeepdahiya requested a review from t-hamano June 17, 2025 03:51

sirreal reviewed Jun 17, 2025

View reviewed changes

"Reverting to previous minor fix"

b88e32c

This reverts commit 9f29537.

t-hamano reviewed Jun 17, 2025

View reviewed changes

removed whitespace and renamed to normalize_css_font_family_name

044e261

iamsandeepdahiya requested review from sirreal and t-hamano June 17, 2025 09:21

t-hamano approved these changes Jun 17, 2025

View reviewed changes

sirreal removed their request for review June 18, 2025 09:23

	public static function normalize_quoted_font_family_name( $item ) {
	public static function normalize_css_font_family_name( $item ) {

	$item = str_replace( [ "\r\n", "\r", "\n" ], '\\A', $item );
	$item = str_replace( [ "\r\n", "\r", "\n" ], '\\A ', $item );

Fix: WP_Font_Face: Font names that contain single quotes are not wrapped in double quotes #8982

Are you sure you want to change the base?

Fix: WP_Font_Face: Font names that contain single quotes are not wrapped in double quotes #8982

Conversation

iamsandeepdahiya commented Jun 15, 2025

Uh oh!

github-actions bot commented Jun 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jun 15, 2025

Test using WordPress Playground

Some things to be aware of

Uh oh!

t-hamano commented Jun 16, 2025

Uh oh!

iamsandeepdahiya commented Jun 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

t-hamano commented Jun 16, 2025

Uh oh!

sirreal commented Jun 16, 2025

Uh oh!

sirreal commented Jun 16, 2025

Uh oh!

t-hamano commented Jun 16, 2025

Uh oh!

iamsandeepdahiya commented Jun 17, 2025

Uh oh!

t-hamano commented Jun 17, 2025

Uh oh!

sirreal left a comment

Choose a reason for hiding this comment

Uh oh!

sirreal Jun 17, 2025

Choose a reason for hiding this comment

Uh oh!

sirreal Jun 17, 2025

Choose a reason for hiding this comment

Uh oh!

sirreal Jun 17, 2025

Choose a reason for hiding this comment

Uh oh!

sirreal Jun 17, 2025

Choose a reason for hiding this comment

Uh oh!

t-hamano commented Jun 17, 2025

Uh oh!

iamsandeepdahiya commented Jun 17, 2025

Uh oh!

t-hamano left a comment

Choose a reason for hiding this comment

Uh oh!

t-hamano left a comment

Choose a reason for hiding this comment

Uh oh!

sirreal commented Jun 18, 2025

Uh oh!

Uh oh!

github-actions bot commented Jun 15, 2025 •

edited

Loading

iamsandeepdahiya commented Jun 16, 2025 •

edited

Loading