Add fallback for the default font folder path #6259

matiasbenedetto · 2024-03-12T15:14:57Z

What?

Add a fallback for the default font directory.
Non persisted alternative of: #6252

Why?

For installations that don’t support modification of the wp-content directory, the Font Library will use wp-content/uploads/fonts as a fallback location, ensuring we stay true to our project philosophy of designing for the majority while still making the feature available to anyone out of the box without extra steps from the user.

Decision from https://make.wordpress.org/core/2024/03/07/unblocking-wp6-5-font-library-and-synced-pattern-overrides/
Discussion: WordPress/gutenberg#59699

How?

Update the wp_get_font_dir() function.

Default

If /wp-content/fonts is writable, wp_get_font_dir() should return:

array(
    'path' => '/var/www/src/wp-content/fonts',
    'url' => 'http://localhost:8889/wp-content/fonts',
    'subdir' => '',
    'basedir' => '/var/www/src/wp-content/fonts',
    'baseurl' => 'http://localhost:8889/wp-content/fonts',
    'error' => false
)

Fallback

If /wp-content/fonts is not writable, wp_get_font_dir() should return:

array(
    'path' => '/var/www/src/wp-content/uploads/fonts',
    'url' => 'http://localhost:8889/wp-content/uploads/fonts',
    'subdir' => '',
    'basedir' => '/var/www/src/wp-content/uploads/fonts',
    'baseurl' => 'http://localhost:8889/wp-content/uploads/fonts',
    'error' => false
)

If the wp-content/uploads/fonts can not be created or written, an error message is added in the 'error' key of the returned array following the wp_font_dir pattern: https://github.com/matiasbenedetto/wordpress-develop/blob/trunk/src/wp-includes/functions.php#L2393-L2406

array(
    'path' => '/var/www/src/wp-content/uploads/fonts',
    'url' => 'http://localhost:8889/wp-content/uploads/fonts',
    'subdir' => '',
    'basedir' => '/var/www/src/wp-content/uploads/fonts',
    'baseurl' => 'http://localhost:8889/wp-content/uploads/fonts',
    'error' => 'Unable to create directory wp-content/uploads/fonts. Is its parent directory writable by the server?'
)

Using `font_dir` filter

Example filtering:

// Define a callback function to pass to the filter.
function set_custom_dir( $defaults ) {
	$defaults['path']    = path_join( WP_CONTENT_DIR, 'custom_dir' );
	$defaults['url']     = 'http://example.com/custom-path/fonts/my-custom-dir';
	$defaults['basedir'] = path_join( WP_CONTENT_DIR, 'custom_dir' );
	$defaults['baseurl'] = 'http://example.com/custom-path/fonts/my-custom-dir';
	return $defaults;
}

// Add the filter.
add_filter( 'font_dir', 'set_custom_dir' );

If font_dir is filtered with a writable directory, wp_get_font_dir() should return:

array(
    'path' => '/var/www/src/wp-content/uploads/custom_dir',
    'url' => 'http://example.com/custom-path/fonts/my-custom-dir',
    'subdir' => '',
    'basedir' => '/var/www/src/wp-content/uploads/custom_dir',
    'baseurl' => 'http://example.com/custom-path/fonts/my-custom-dir',
    'error' => ''
)

If font_dir is filtered with a NON writable directory, wp_get_font_dir() should return:

array(
    'path' => '/var/www/src/wp-content/uploads/custom_dir',
    'url' => 'http://example.com/custom-path/fonts/my-custom-dir',
    'subdir' => '',
    'basedir' => '/var/www/src/wp-content/uploads/custom_dir',
    'baseurl' => 'http://example.com/custom-path/fonts/my-custom-dir',
    'error' => 'Unable to create directory wp-content/custom_dir. Is its parent directory writable by the server?'
)

Trac ticket: https://core.trac.wordpress.org/ticket/60751#ticket

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 · 2024-03-12T15:15:16Z

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 mmaattiiaass, swissspidy, hellofromtonya, peterwilsoncc, azaozz, ironprogrammer, costdev, grantmkin.

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

github-actions · 2024-03-12T15:45:19Z

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.

peterwilsoncc

I've added a few notes inline.

There will need to be some tests to ensure against:

infinite loops (as noted)
fallback applies when expected
fallback does not apply when it isn't expected

If neither directory can be created then error should be defined and return a similar result to wp_upload_dir().

src/wp-includes/fonts.php

Co-authored-by: Peter Wilson <519727+peterwilsoncc@users.noreply.github.com>

…p_upload_dir Co-authored-by: Peter Wilson <519727+peterwilsoncc@users.noreply.github.com>

matiasbenedetto · 2024-03-13T15:52:51Z

There will need to be some tests to ensure against:
* infinite loops (as noted)
* fallback applies when expected
* fallback does not apply when it isn't expected
If neither directory can be created then error should be defined and return a similar result to wp_upload_dir().

I thought about these 2 alternatives for testing this functionality but they didn't work on my end:

use chmod() in the test body to change the permissions of the path (it seems to be ignored, and wp_is_writable always returns true in the unit test env).
try to mock the wp_is_writable( $path ) function in the context of the unit test. (I couldn't mock this global function. I found examples in the core's codebase about mocking class methods but not functions in the global scope).

Any help bout how to test this functionality is welcome.

cc. @peterwilsoncc @swissspidy @youknowriad

…rror message is added in the 'error' key of the returned array following the `wp_font_dir` pattern: https://github.com/matiasbenedetto/wordpress-develop/blob/trunk/src/wp-includes/functions.php#L2393-L2406 Co-authored-by: Peter Wilson <519727+peterwilsoncc@users.noreply.github.com>

matiasbenedetto · 2024-03-13T16:13:06Z

If neither directory can be created then error should be defined and return a similar result to wp_upload_dir().

Added here: d2f6e5c

swissspidy · 2024-03-13T16:17:22Z

@matiasbenedetto Which test is this in context of? This PR does not have any tests, but I do see tests at #6252. The chmod() there actually works just fine for me.

Some of my recent l10n tests also use chmod without issues, so I don't see why that would be an issue here.

If you can point me at the code this is in reference to, I am happy to take a look.

matiasbenedetto · 2024-03-13T16:27:13Z

@matiasbenedetto Which test is this in context of? This PR does not have any tests, but I do see tests at #6252. The chmod() there actually works just fine for me.

@swissspidy thanks for taking a look. The test using chmod is failing in #6252 CI: https://github.com/WordPress/wordpress-develop/actions/runs/8251059911/job/22567291729?pr=6252

Some of my recent l10n tests also use chmod without issues, so I don't see why that would be an issue here.

If you can point me at the code this is in reference to, I am happy to take a look.

I added the same kind of test using chmod in this PR to test here too but is failing on my local env and in the CI: 9c51258

peterwilsoncc · 2024-03-13T21:18:39Z

src/wp-includes/fonts.php

@@ -122,6 +122,36 @@ function wp_get_font_dir() {
 		'error'   => false,
 	);

+	$made_dir = wp_mkdir_p( $defaults['path'] );


I've come to realize that attempting to create the directory here is problematic.

As the code runs before the font_dir filter, it's unknown at this point in the execution whether the directory path will be overridden by a plugin. If it is overridden then this code may create a junk directory in the sites filesystem that will never be used.

What I think the code will need to do is:

check if the folder exists and is writable

if the folder doesn't exist, check the parent exists and is writable

rely on wp_upload_dir() to create the directory after the upload_dir filter runs (which will include firing the font_dir filter)

check the error status produced by wp_upload_dir()

create the directory after the upload_dir filter runs (which will include firing the font_dir filter)

+1.

Seems the first two checks:

check if the folder exists and is writable

if not, check the parent exists and is writable

are also done in wp_mkdir_p(). Somewhat unsure if they need to be done (repeated) before running wp_mkdir_p()?

Yes, good points @peterwilsoncc. I agree 👍

Good call; in this commit I implemented the change to create the folder only after the font_dir filter runs.

@costdev

To trigger the fallback logic, need to create a fake where `wp_mkdir_p()` bails out early and returns `false`. Looking at wp_mkdir_p(), notice this code: ``` if ( file_exists( $target ) ) { return @is_dir( $target ); } ``` When the `$target` exists but is a file (not a directory), then `false` gets returned. Aha! Props to @costdev. This commit: * Adds a fake file, reworks the test, and then removes the file in the tear_down(). * Refactors the removing of the fonts directory by putting it into a separate method. These new methods can be put into a trait or base abstraction that all of the Font Library test classes use to ensure the filesystem is restored between tests (that's out-of-scope for this PR).

hellofromtonya · 2024-03-13T22:38:18Z

Whomever commits this, please also prop @costdev.

hellofromtonya · 2024-03-13T22:49:37Z

tests/phpunit/tests/fonts/font-library/wpFontsDir.php

+		}
+	}
+
+	private function fake_no_new_directories_in_wp_content() {


This is a weird fake, but it works. The way it works is this:

A file named fonts that exists in the wp-content will trigger wp_mkdir_p() to fail into the file_exists() bail out and return false when is_dir() runs.

That will then cause the fallback branch to execute.

I created it in a method for 2 reasons:

In the test, makes it more readable and less code.

Reusability. This logic might be useful elsewhere for other Font Library tests. If yes, it can be moved to a trait or base TestCase.

Wondering if the method name should be focused on what it does -> makes wp_mkdir_p() return false ??

I think the name is fine

I renamed this function create_fake_file_to_avoid_dir_creation because I think it's a little clearer, making it path-agnostic. It was needed because it is used to create a path received as a parameter in the additional test cases added here: 90a3982

hellofromtonya · 2024-03-13T22:52:33Z

tests/phpunit/tests/fonts/font-library/wpFontsDir.php

+		$this->assertSame( $expected, $font_dir, 'The font directory should be a subdir in the uploads directory.' );
+	}
+
+	private function remove_fonts_directory() {


I moved the cleanup code to a separate method to make it reusable.

All of the Font Library tests should be removing the fonts directory between tests. Moving it here is the step. Then a trait or base TestCase can be add that is shared amongst all of the test classes (though out-of-scope for this PR).

ironprogrammer

Thanks for this PR, and for everyone's input in fleshing it out!

In addition to the suggestions in this review, I also recommend adding the following annotations to the file header:

@since 6.5.0
@ticket 60751

And while not added in this PR, the function test_fonts_dir_with_filter() might be better named test_font_dir_filter(), as the filter name is font_dir.

tests/phpunit/tests/fonts/font-library/wpFontsDir.php

Co-authored-by: Brian Alexander <824344+ironprogrammer@users.noreply.github.com>

swissspidy · 2024-03-14T13:47:32Z

tests/phpunit/tests/fonts/font-library/wpFontsDir.php

+
+	private function remove_no_new_directories_in_wp_content_fake() {
+		if ( file_exists( self::$fake_fonts_file ) ) {
+			@unlink( self::$fake_fonts_file );


I don't think we need or should add error suppression.

removed here: f7de278

…'t fall back to the /wp-content/uploads/fonts folder.

matiasbenedetto · 2024-03-14T18:16:43Z

the function test_fonts_dir_with_filter() might be better named test_font_dir_filter(), as the filter name is font_dir.

Done here: f0c3f3d

matiasbenedetto · 2024-03-14T18:20:54Z

Thanks everyone for the contributions.

In the last commits, I tried to implement the remaining feedback.
I updated the PR description with all the cases being tested and their expected returned values.
I think it is ready for another review.

peterwilsoncc · 2024-03-14T21:16:04Z

src/wp-includes/fonts.php

@@ -131,7 +131,44 @@ function wp_get_font_dir() {
 	 *
 	 * @param array $defaults The original fonts directory data.
 	 */
-	return apply_filters( 'font_dir', $defaults );
+	$font_dir = apply_filters( 'font_dir', $default_dir );


This needs to be applied with the final default directory known.

While the use-case we've seen is to change the default directory, there are other use cases that need to be considered. Once such example is wishing to store different font families in sub-directories, eg fonts/open-sans, fonts/source-code-pro.

It also presents the problem that WordPress may override the changes introduced by a plugin. If the directory defined by the plugin is not writable then it's the plugin's problem to handle.

+1 I agree.

The processing order then:

First, determine where the fonts directory should be created.

Then pass that "final" location to the filter.

Then use that filtered location to make the directory.

As @peterwilsoncc noted, the first step of determining the "where" needs to include more conditionals, such as checking if the parent is writable.

peterwilsoncc · 2024-03-14T21:19:41Z

src/wp-includes/fonts.php

-	return apply_filters( 'font_dir', $defaults );
+	$font_dir = apply_filters( 'font_dir', $default_dir );
+
+	wp_mkdir_p( $font_dir['path'] );


This function can not attempt to create the directory. It's unaware if wp_upload_dir() has been called to get the directory without creating it.

If the directory doesn't exist, we'll need to check if the parent directory is writable.

As far as I see wp_mkdir_p() expects full paths and makes directories recursively (calls PHP's mkdir() with true as third param). That should make any missing parent and sub-parent directories? I.e. trying to make /wp-content/dir1/dir2/fonts would work, and dir1 and dir2 will be created if they don't exist.

If the directory doesn't exist, we'll need to check if the parent directory is writable.

May be missing something but seems wp_mkdir_p() would return false if the parent (or grandparent, great-grandparent, etc.) directory is not writable. Seems that's how it is used elsewhere in core? Then checking beforehand whether the parent directory is writable sounds like a tiny bit of optimization for the cases where that directory is not writable, but a tiny bit of overhead for cases where it is writable (more common)?

I.e. something like this code (example copied from wp_mkdir_p()) will have to run before every wp_mkdir_p() call:

// Do not allow path traversals. if ( str_contains( $target, '../' ) || str_contains( $target, '..' . DIRECTORY_SEPARATOR ) ) { return false; // Bail out somehow... } // We need to find the permissions of the parent folder that exists and inherit that. $target_parent = dirname( $target ); while ( '.' !== $target_parent && ! is_dir( $target_parent ) && dirname( $target_parent ) !== $target_parent ) { $target_parent = dirname( $target_parent ); } // Get the permission bits. $stat = @stat( $target_parent ); // Or maybe use is_writable( $target_parent ); (seems this might be a bit problematic on some filesystems if $target_parent was just created)

And then almost the same code will run inside wp_mkdir_p() if the permissions indicate that the directory is writable.

Frankly I'm not sure that the extra code to check the parent directory's permissions is warranted? Seems wp_mkdir_p() can handle the case where parent is not writable?

Frankly I'm not sure that the extra code to check the parent directory's permissions is warranted? Seems wp_mkdir_p() can handle the case where parent is not writable?

wp_upload_dir() (which this code filters) runs in two context: one in which it creates the directory, one in which it doesn't.

As this function doesn't know the context re file creation, it's not able to create a directory as it risks adding junk directories to the server. Junk meaning that they may never be used.

The font directory functions are also used as a getter during post deletion so we can't create directories to delete files that do not exist.

Sorry but I think I'm really missing something here.

wp_upload_dir() (which this code filters) runs in two context: one in which it creates the directory, one in which it doesn't.

Right.

As this function doesn't know the context re file creation

I.e. we don't know whether wp-content or wp-content/uploads (or their replacements if somebody moved/renamed them) exist, correct? But how could that be after wp_upload_dir() has already returned the path and URL to the uploads dir? Do you mean the data from wp_upload_dir() may be wrong?

it's not able to create a directory as it risks adding junk directories to the server

Meaning wp_mkdir_p() (actually PHP's mkdir()) will not be able to create a directory but may add "junk" directories? Sorry but I'm lost, how can it add directories without being able to create them...

Perhaps some practical example would help? Also, what would a fix for this look like? Maybe that would help to understand and recreate the problem?

It has nothing to do with whether wp_mkdir_p() can create the directory.

What I want to avoid is this function attempting to create the directories when it should not. This can happen in two circumstances:

when hooked on to upload_dir: wp_upload_dir() is called with the $create_dir parameter set to false, either directly or via wp_get_upload_dir().

when called as a getter in _wp_before_delete_font_face(): this is used to delete font files if the _wp_font_face_file post meta is defined. As font face post types may not contain a font file (see the discussion in wp_font_face post type is a near duplication of the attachment post type. gutenberg#59387) then WordPress can't start creating directories in order to delete files that don't exist.

Ah I see what you mean. So basically this function has to be split in two: a getter and then a function that would only be used to create the dirs if needed.

@azaozz Yes, that's correct.

It's what I did in the PR to fix the infinite loop issue rather than work aroudn it #6211 (reviews welcome, btw), partly to prepare for this.

peterwilsoncc · 2024-03-14T21:21:49Z

src/wp-includes/fonts.php

+			'error'   => false,
+		);
+
+		wp_mkdir_p( $font_dir['path'] );


azaozz · 2024-03-14T22:28:06Z

src/wp-includes/fonts.php

+			'baseurl' => $upload_dir['baseurl'] . '/fonts',
+			'error'   => false,
+		);
+


As @peterwilsoncc mentions above thinking it would probably be good to run the font_dir filter here again as the values were reset (hardcoded).

I'd really prefer to just run it once after WordPress figures out what it will present as the default.

If I'm understanding correctly you prefer this function to do something like this?

if ( is_dir( WP_CONTENT_DIR ) && is_writable( WP_CONTENT_DIR ) ) { $default_font_dir = WP_CONTENT_DIR . '/fonts'; elseif ( is_dir( WP_CONTENT_DIR . '/uploads' ) && is_writable( WP_CONTENT_DIR . '/uploads' ) ) { // This will only work if /uploads already exists. The assumption is that /uploads cannot be created here as WP_CONTENT_DIR either doesn't exist or is not writable. $default_font_dir = WP_CONTENT_DIR . '/uploads/fonts'; } else { // Throw error, etc. } $font_dir = apply_filters( 'font_dir', $default_font_dir ); // Try to crate the /fonts dir

peterwilsoncc · 2024-03-15T01:11:09Z

tests/phpunit/tests/fonts/font-library/wpFontsDir.php

+	private function remove_font_paths() {
+		$paths = array(
+			path_join( WP_CONTENT_DIR, 'fonts' ),
+			path_join( WP_CONTENT_DIR, 'custom_dir' ),
+			path_join( WP_CONTENT_DIR, 'uploads/fonts' ),
+		);
+
+		foreach ( $paths as $path ) {
+			if ( ! is_dir( $path ) ) {
+				if ( file_exists( $path ) ) {
+					unlink( $path );
+				}
+			} else {
+				$this->rmdir( $path );
+				rmdir( $path );
+			}
+		}
+	}
+
+	/**
+	 * A placeholder "fake" file at $path triggers `wp_mkdir_p()` to fail into the `file_exists()` bail out, causing `is_dir()` to  return `false`.
+	 * This effectively makes $path unwritable.
+	 */
+	private function create_fake_file_to_avoid_dir_creation( $path ) {
+		file_put_contents(
+			$path,
+			'fake file'
+		);
+	}


These two methods will cause problems for most local development environments, including the docker container provided in this repo.

Typically the test suite and the development environment share content directories so developers running these environments may have existing font folders as a result of their manual testing.

During my tests the fonts from manual testing were deleted during these test runs.

To reproduce using the repos docker environment:

Run commands to bootstrap the environment
npm install npm run build:dev npm run env:start npm run env:install

Visit http://localhost:8889/wp-admin/site-editor.php?canvas=edit

Opens Styles > Typography > Manager > Install Fonts & Install some fonts via google

Apply the font to the main header in 2024

Save and exit the site editor

Observe the folder src/wp-content/fonts has been created

Run this test suite npm run test:php -- --filter Tests_Fonts_WpFontDir

Observe the folder src/wp-content/fonts has been deleted

Visit the front end of the site

Observe the font download fails with a 404 error

Typically the test suite and the development environment share content directories so developers running these environments may have existing font folders as a result of their manual testing.

I think this is unfortunate and unexpected, right? I get your point but I think this is more a problem of the core dev env than a problem of the tests.

The unit tests are supposed to run in a reproducible environment and not depend on or produce 'side effects.' If other parts of the workflow unrelated to unit tests are affected by the changes made by running the unit tests, the workflow is deficient, and it should be improved instead of degrading the unit tests.

You're right @peterwilsoncc. I think what is needed is to:

Check if the fonts folder already exists (do that within the set_up_before_class() fixture).

If no, do nothing.

If yes,

Store it in a static property to remember.

If that property is set, then disable deleting the existing folder (i.e. in the tear down) and skip the associated tests that will fail due to it already existing.

In Core's CI, the tests will run as the folder will not exists. But in local development environments, this technique will guard against the scenario you described.

Check if the fonts folder already exists

+1. If the /fonts dir already exists (at any tested location) it can be used for testing but shouldn't be deleted. As far as I see keeping a static var whether the fonts dir pre-existed would be a good way to do this.

The approach seems sound, though I'd do the same for wp-content/uploads/fonts. This may also already exist for manual testing purposes, and the test suite shouldn't delete it.

Ideally, we would be using a tests-only fonts directory to avoid affecting the actual site's filesystem in the tests and also preventing possible filename clashes, etc. Alternatively, if we continue to use the actual site's filesystem, maybe store the existence and pre-test contents of the directories. This means we could accurately compare the contents before/after the function under test has run, and therefore detect unexpected creation/deletion of files in those directories, plus delete only the files created by the tests.

peterwilsoncc · 2024-03-17T21:38:56Z

tests/phpunit/tests/fonts/font-library/wpFontsDir.php

 	public function test_fonts_dir() {
 		$font_dir = wp_get_font_dir();

 		$this->assertSame( $font_dir, static::$dir_defaults );
 	}

-	public function test_fonts_dir_with_filter() {
+	public function test_font_dir_filter() {


I've realised this will trigger test failures on some of the hosting providers running the test suite if the content folder isn't writable on their file system.

creativecoder · 2024-03-17T22:06:13Z

src/wp-includes/fonts.php

+			$font_dir['error'] = sprintf(
+				/* translators: %s: Directory path. */
+				__( 'Unable to create directory %s. Is its parent directory writable by the server?' ),
+				esc_html( str_replace( ABSPATH, '', $font_dir['path'] ) )
+			);


Note that there are a few places where wp_get_font_dir is used that we'll need to check to see if the error is populated and handle it accordingly:

wordpress-develop/src/wp-includes/fonts.php

Line 178 in 17415a7

$font_dir = wp_get_font_dir()['path'];

wordpress-develop/src/wp-includes/rest-api/endpoints/class-wp-rest-font-faces-controller.php

Line 869 in 17415a7

$font_dir = wp_get_font_dir();

wordpress-develop/src/wp-includes/rest-api/endpoints/class-wp-rest-font-faces-controller.php

Line 869 in 17415a7

$font_dir = wp_get_font_dir();

azaozz · 2024-03-19T01:16:24Z

Trying an alt solution in matiasbenedetto#1. It simplifies the code a bit and only depends on wp_is_writable( WP_CONTENT_DIR ) to determine whether to move the /fonts dir or not.

Also has an optional param to tell wp_get_font_dir() whether to actually make the directory or just get the path/data for it (works similarly to wp_upload_dir() $create_dir param).

Also see #6211 (comment) re: an idea how to "fix" the lambda filter/possible loop for wp_upload_dir() in _wp_handle_upload() by adding an "override".

peterwilsoncc · 2024-03-19T22:46:27Z

@azaozz The alternative PR in to this branch doesn't work as it can place font files in uploads/fonts on the first install and wp-content/fonts on the second install. If the content directory doesn't exist initially for an upload it's created.

See the gif attached (in my local setup wp-content is simply content)

It also causes further lock in to having to work around the infinite loop bug rather than fix it.

azaozz · 2024-03-20T01:19:38Z

it can place font files in uploads/fonts on the first install and wp-content/fonts on the second install

You mean WP_CONTENT_DIR was non-writable on the first try to install a font and suddenly became writable when trying to install a font for the second time? Otherwise how can WP ever make directories or add files to a non-writable directory?

Would it be possible to try couple of var_dump( is_writable( WP_CONTENT_DIR ) );?
(Btw, having quite a bit of problems with PHP's chmod() when trying to test this. Doesn't seem to work at all. Could that be related?)

It also causes further lock in to having to work around the infinite loop bug rather than fix it.

But the fix for that infinite loop is to not use a filter there at all, just do a simple "override" like the rest of _wp_upload_dir(). Imho there is no good reason for that lambda filter anyway. Discussion: #6211 (comment).

azaozz · 2024-03-20T01:30:11Z

Made the alt patch into a stand-alone PR (as requested): #6292

peterwilsoncc · 2024-03-21T23:02:31Z

Closing this PR off per https://make.wordpress.org/core/2024/03/21/font-library-update-storage-of-font-files/. Thanks everyone for your work on this, I appreciate it.

matiasbenedetto added 10 commits March 11, 2024 17:14

Add fallback for the default font folder path

6e1e8e0

improve dir is_writable check

0fb2cec

format php

7a46783

remove unwanted early return

b85cdf3

re-order actual and expected

b8e35cc

remove not needed assignement

4f4e63c

do not suppres errors

e655888

format php

8e23ae4

use the same format as in the wp-content/fonts default path

25d3c80

removing database persistence

030b626

This was referenced Mar 12, 2024

Add a databse persisted fallback for the default font folder path #6252

Closed

Architectural discussion allowing for fonts folder fallbacks. WordPress/gutenberg#59699

Closed

remove wp_default_font_dir function as not needed

48d7534

peterwilsoncc reviewed Mar 12, 2024

View reviewed changes

src/wp-includes/fonts.php Outdated Show resolved Hide resolved

src/wp-includes/fonts.php Outdated Show resolved Hide resolved

src/wp-includes/fonts.php Outdated Show resolved Hide resolved

src/wp-includes/fonts.php Outdated Show resolved Hide resolved

matiasbenedetto and others added 4 commits March 13, 2024 11:02

save wp_mkdir_p result in a variable

171b621

Co-authored-by: Peter Wilson <519727+peterwilsoncc@users.noreply.github.com>

check if the dir couldn't be created

596f452

Co-authored-by: Peter Wilson <519727+peterwilsoncc@users.noreply.github.com>

use array syntax

9b05b6a

Update wp_get_font_dir function to use wp_get_upload_dir instead of w…

895984b

…p_upload_dir Co-authored-by: Peter Wilson <519727+peterwilsoncc@users.noreply.github.com>

be consistent in the use of $defaults['path'] to determine the path

928a564

matiasbenedetto added 2 commits March 13, 2024 13:22

add a test using chmod (is failing)

9c51258

format php

e405ba8

matiasbenedetto added 2 commits March 13, 2024 13:30

undo unwanted changes

c3f21ea

fix test

b2a34dd

Delete files in directory too

ac77349

peterwilsoncc reviewed Mar 13, 2024

View reviewed changes

hellofromtonya reviewed Mar 13, 2024

View reviewed changes

ironprogrammer reviewed Mar 14, 2024

View reviewed changes

matiasbenedetto and others added 2 commits March 14, 2024 10:33

Improve texts

de55afb

Co-authored-by: Brian Alexander <824344+ironprogrammer@users.noreply.github.com>

improve test message

e3e8289

Co-authored-by: Brian Alexander <824344+ironprogrammer@users.noreply.github.com>

swissspidy reviewed Mar 14, 2024

View reviewed changes

matiasbenedetto added 4 commits March 14, 2024 14:55

If the font_dir was filtered it uses the filtered values and it doesn…

90a3982

…'t fall back to the /wp-content/uploads/fonts folder.

remove error supression

f7de278

format php

45c8ece

rename test case

f0c3f3d

peterwilsoncc reviewed Mar 14, 2024

View reviewed changes

peterwilsoncc mentioned this pull request Mar 14, 2024

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

Closed

azaozz reviewed Mar 14, 2024

View reviewed changes

peterwilsoncc reviewed Mar 15, 2024

View reviewed changes

peterwilsoncc reviewed Mar 17, 2024

View reviewed changes

creativecoder reviewed Mar 17, 2024

View reviewed changes

Merge branch 'trunk' into add/font-dir-default-non-persisted

206543a

azaozz mentioned this pull request Mar 20, 2024

Alternative fallback for the default /fonts path #6292

Closed

peterwilsoncc closed this Mar 21, 2024

Add fallback for the default font folder path #6259

Add fallback for the default font folder path #6259

Conversation

matiasbenedetto commented Mar 12, 2024 • edited Loading

What?

Why?

How?

Default

Fallback

Using font_dir filter

github-actions bot commented Mar 12, 2024 • edited Loading

github-actions bot commented Mar 12, 2024

Test using WordPress Playground

Some things to be aware of

peterwilsoncc left a comment

Choose a reason for hiding this comment

matiasbenedetto commented Mar 13, 2024 • edited Loading

matiasbenedetto commented Mar 13, 2024

swissspidy commented Mar 13, 2024

matiasbenedetto commented Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

azaozz Mar 13, 2024 • 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 Mar 13, 2024

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

matiasbenedetto Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ironprogrammer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matiasbenedetto commented Mar 14, 2024

matiasbenedetto commented Mar 14, 2024

Choose a reason for hiding this comment

hellofromtonya Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

hellofromtonya Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azaozz Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azaozz Mar 15, 2024 • 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

azaozz Mar 15, 2024 • edited Loading

Choose a reason for hiding this comment

peterwilsoncc Mar 15, 2024 • edited Loading

Choose a reason for hiding this comment

matiasbenedetto Mar 15, 2024 • edited Loading

Choose a reason for hiding this comment

hellofromtonya Mar 15, 2024 • edited Loading

Choose a reason for hiding this comment

azaozz Mar 15, 2024 • 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

azaozz commented Mar 19, 2024 • edited Loading

peterwilsoncc commented Mar 19, 2024

azaozz commented Mar 20, 2024 • edited Loading

azaozz commented Mar 20, 2024

peterwilsoncc commented Mar 21, 2024

matiasbenedetto commented Mar 12, 2024 •

edited

Loading

Using `font_dir` filter

github-actions bot commented Mar 12, 2024 •

edited

Loading

matiasbenedetto commented Mar 13, 2024 •

edited

Loading

matiasbenedetto commented Mar 13, 2024 •

edited

Loading

azaozz Mar 13, 2024 •

edited

Loading

matiasbenedetto Mar 14, 2024 •

edited

Loading

hellofromtonya Mar 14, 2024 •

edited

Loading

hellofromtonya Mar 14, 2024 •

edited

Loading

azaozz Mar 14, 2024 •

edited

Loading

azaozz Mar 15, 2024 •

edited

Loading

azaozz Mar 15, 2024 •

edited

Loading

peterwilsoncc Mar 15, 2024 •

edited

Loading

matiasbenedetto Mar 15, 2024 •

edited

Loading

hellofromtonya Mar 15, 2024 •

edited

Loading

azaozz Mar 15, 2024 •

edited

Loading

azaozz commented Mar 19, 2024 •

edited

Loading

azaozz commented Mar 20, 2024 •

edited

Loading