Font Library: first time install fails if fonts upload directory isn't created.

[matiasbenedetto]

Reproduction Report

First attempt i got this error but second attempt installed successfully.

trac ticket: https://core.trac.wordpress.org/ticket/60528

Environment

• WordPress: 6.5-beta1-57631
• PHP: 8.3.2
• Server: nginx/1.25.3
• Database: mysqli (Server: 11.2.2-MariaDB / Client: mysqlnd 8.3.2)
• Browser: Chrome 121.0.0.0
• OS: macOS
• Theme: Twenty Twenty-Four 1.0
• MU Plugins: None activated
• Plugins:
  • Test Reports

Originally posted by @huzaifaalmesbah in #58997 (comment)

[t-hamano]

I remember that this issue was fixed in #55893, but maybe not all scenarios were covered, or maybe a regression occurred at some point.

[creativecoder]

I started looking into how other directories in wp-content are created by WordPress, to see if there's a convention that could be used here.

For the /uploads directory, the wp_upload_dir function has a $create_dir parameter that defaults to true, so the directory is created when getting the directory info. However, the documentation for $create_dir says "Default true for backward compatibility" and there's caching added to avoid multiple checks for the directory--so that may not be the best pattern to follow.

For /languages, it looks like get_filesystem_method is involved in creating the directory, which is related to WP_Filesystem and the different ways of interacting with the file system (direct, ssh, or ftp). It appears that the directory is created when installing the first language pack. Note that we'll probably want to add a wp_fonts_dir method to the WP_Filesystem_Base class.

Haven't found how the plugin and theme directories are created yet, but I suspect that happens during the initial WordPress install.

[creativecoder]

This is proving very tricky to replicate.

Generally, when a font is uploaded, the fonts directory should be created automatically, if needed

• WP_REST_Font_Faces_Controller::handle_font_file_upload calls wp_handle_upload (after filtering wp_upload_dir)
• wp_handle_upload calls _wp_handle_upload, which calls wp_upload_dir
• wp_upload_dir will create the fonts directory, if it doesn't exist

I was able to replicate this error a few times, where only the first font upload fails, by deleting both the fonts directory and the upload directory for the current month and year (e.g. uploads/2024/02). But I can't get it to fail consistently. I suspect there's some kind of race condition or similar that's related to filtering wp_upload_dir, but I'm not sure.

The error seems to be triggered here, within the wp_upload_dir function. If anyone who can replicate this can provide a backtrace and the values of the variables in the local scope of wp_upload_dir when it fails, that would be helpful.

cc @matiasbenedetto @huzaifaalmesbah

[swissspidy]

So far I am unable to reproduce this issue with the given pointers. wp_upload_dir() is always creating the fonts directory as expected.

Since this is an issue with creating directories, what are the file permissions for the wp-content directory in your case(s)?

---

The only thing I noticed is that I can re-install the same font over and over again even though it's already installed:

Screen.Recording.2024-02-23.at.13.44.48.mov

Is this on anyone's radar?

[kjnanda]

@swissspidy

While testing this ticket, I found this error. So here, I don't see the Install Fonts option though I can upload the font multiple times!

[mikachan]

FWIW, as I'm not sure this is the best way to reproduce this issue, I was seeing a similar issue consistently in Playground when testing out the Font Library. Here are the steps I was following:

1. Open Playground instance at https://playground.wordpress.net/
2. Enable both "Load extensions" and "Network access" in Playground settings, and switch the WordPress version to 6.5-beta2 (now beta3)
3. Open the Font Library modal
4. Go to the Upload tab
5. Try uploading a font file (I was testing with Commissioner regular)
6. The upload failed with this error: Invalid parameter(s): font_face_settings (which is different to the error reported in this issue)

As of beta 2 (and beta 3), these issues seem to be consistently resolved in Playground. Here is an example instance.

I'm wondering if these two errors are related, then maybe this issue has been fixed by something else between beta 1 and beta 2?

[creativecoder]

Noting that I ran into this issue again while testing #58839. One way of replicating this might be changing the fonts directory with a filter to a path that hasn't been used before.

[kjnanda]

I confirm that this is fixed with the latest beta 3 release.

[creativecoder]

Thanks for testing @kjnanda ! I don't think we can be sure this is fixed, as it's happening very intermittently. I encountered the error again just yesterday.

[peterwilsoncc]

I was working on peterwilsoncc/fonts-to-uploads over the weekend and hit this a couple of times too (using Chassis/Chassis on Vagrant/VirtualBox) but am having the same trouble as others: I'm unable to consistently reproduce it.

I agree it's likely a race condition on wp_mkdir_p() but it could also have something to do with filesystem caching.

[peterwilsoncc]

To avoid the risk of font files being created in multiple locations, I think this will need to be resolved as part of WordPress/wordpress-develop#6259

Is it possible it's hapopening due to parallel requests during uploads?

[getdave]

@matiasbenedetto @peterwilsoncc I'm in the process of clearing the Milestone for the WP 6.5 release and I came across this one.

Are you able to advise on the status?

From what I can see, the last comment references a Core PR which has subsequently been closed due to the decision around the Fonts upload location.

If it's resolved or no longer relevant we can remove from the Editor Board.

Thanks in advance.

[getdave]

Speaking with @creativecoder away from Github, my understanding is that this issue remains even if the directory is wp-content/uploads/fonts. It appears to occur when the directory isn't present on initial attempt to store content within it.

I updated the Issue title to reflect that.

[creativecoder]

Here's how I've been able to reproduce this today, (using wordpress-develop trunk)

Add a filter to specify a fonts directory that you've never used before for uploading files.

// Append an arbitrary value to the font directory name
function alter_wp_fonts_dir( $defaults ) {
	$append_dir = 'a';
	$defaults['path'] = $defaults['path'] . $append_dir;
	$defaults['url']  = $defaults['url'] . $append_dir;

	return $defaults;
}
add_filter( 'font_dir', 'alter_wp_fonts_dir' );

Check more than one font in the font library and install it. One of them will fail, but the rest should succeed. Checking only one seems to succeed without error.

To replicate again, change the $append_dir value in the filter to use a new directory.

One hint is that wp_mkdir_p outputs the following warning: PHP Warning: mkdir(): File exists in ... wordpress-develop/src/wp-includes/functions.php on line 2084. This is normally suppressed, but shows up in the error log if you remove the @ function prefix before mk_dir inside wp_mkdir_p.

[creativecoder]

Being able to replicate this issue most of the time locally, I can now confirm that it's caused by a race condition that happens when

• The fonts directory has not been created
• More that one request is dispatched to the font-faces endpoint at the same time

The requests are in a race to execute the wp_mkdir_p function to create the fonts directory. If both requests make it to the file_exists check within wp_mkdir_p around the same time, they will both try to create the directory using mkdir, but whoever looses the race will fail, triggering the error in wp_upload_dir.

[creativecoder]

I've opened a PR with minimal changes needed to work around the issue, for now, until we can fix the underlying problem in wp_upload_dir/wp_mkdir_p: #60180

[peterwilsoncc]

@creativecoder Nice work figuring this out, Grant. Thank you!

[swissspidy]

@creativecoder did you already open a trac ticket to explore improving wp_mkdir_p & co?

[creativecoder]

@swissspidy Done here: https://core.trac.wordpress.org/ticket/60855#ticket

[getdave]

Test coverage added and merged in #60221