Directory

Font Library: first time install fails if fonts upload directory isn't created. · Issue #59023 · WordPress/gutenberg · GitHub
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
matiasbenedetto opened this issue Feb 14, 2024 · 20 comments · Fixed by #60180
Closed
Labels
[Feature] Font Library [Type] Bug An existing feature does not function as intended

Comments

@matiasbenedetto
Copy link
Contributor

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
Screenshot 2024-02-14 at 12 46 32 PM

Originally posted by @huzaifaalmesbah in #58997 (comment)

@t-hamano
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Member

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
Copy link

kjnanda commented Feb 27, 2024

@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
Copy link
Member

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
Copy link
Contributor

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
Copy link

kjnanda commented Feb 29, 2024

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

image

@creativecoder
Copy link
Contributor

creativecoder commented Feb 29, 2024

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

getdave commented Mar 25, 2024

@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
Copy link
Contributor

getdave commented Mar 25, 2024

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.

@getdave getdave changed the title Font Library: first time install fails if /wp-content/fonts isn't created. Font Library: first time install fails if fonts upload directory isn't created. Mar 25, 2024
@creativecoder
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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

@swissspidy
Copy link
Member

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

@creativecoder
Copy link
Contributor

@getdave
Copy link
Contributor

getdave commented Mar 28, 2024

Test coverage added and merged in #60221

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Font Library [Type] Bug An existing feature does not function as intended
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

9 participants