-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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: Fix font installation failure #55893
Conversation
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/experimental/fonts/font-library/class-wp-rest-font-library-controller.php |
Flaky tests detected in d7d46a3. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6771852220
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! Good catch @t-hamano and thank you for fixing.
I made only one minor change so that the has_upload_directory
is private (and thus no need for unit testing) since it is not used externally.
🚢
Thanks for the review, @pbking!
Thank you for your follow-up. This certainly makes sense. |
Fixes #55879
What?
This PR fixes an issue where local fonts or Google fonts installation fails.
Why?
When I traced the cause, I found that it was caused by #54829. The error also occurs because
has_write_permission()
isfalse
at this location.gutenberg/lib/experimental/fonts/font-library/class-wp-rest-font-library-controller.php
Line 421 in 43c5e2a
The
has_write_permission()
method checks whether the font directory (/path-to/wp-content/fonts
) is writable.However, if you have never installed any fonts, the
fonts
directory does not exist and is considered unwritable.Before the change in #54829,
wp_upload_dir()['basedir']
(/path/to/wp-content/uploads
) was checked to see if it was writable, which is not the actual directory where the fonts would be written. In most cases theuploads
directory will be writable, so thehas_write_permission()
method will return true and the font will be installed unintentionally.How?
Originally, I think we should install the font after all checks below have passed.
fonts
directory exist?fonts
directory does not exist, try to create it and check whether the directory was created successfullyfonts
directory writable?I reflected all these conditions in the logic.
Testing Instructions
Pattern 1
/path/to/wp-content/fonts
directory exists, please delete it.fonts
directory will be created and the font installation should be successful.Pattern 2
/path/to/wp-content/fonts
directory./path-to/wp-content
directory non-writable with the following command:chmod a-w /path/to/wp-content
cannot_create_fonts_folder
).Pattern 3
/path/to/wp-content
directory writable again with the following command:chmod 755 /path/to/wp-content
path/to/wp-content/fonts
dorectory/path-to/wp-content/fonts
directory non-writable with the following command:chmod a-w /path/to/wp-content/fonts
cannot_write_fonts_folder
).