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.

Sign up for GitHub

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

Jump to bottom

Add fallback for the default font folder path #6259

Closed
wants to merge 31 commits into from
Closed

Add fallback for the default font folder path #6259

Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
6e1e8e0
Add fallback for the default font folder path
matiasbenedetto Mar 11, 2024
0fb2cec
improve dir is_writable check
matiasbenedetto Mar 11, 2024
7a46783
format php
matiasbenedetto Mar 11, 2024
b85cdf3
remove unwanted early return
matiasbenedetto Mar 12, 2024
b8e35cc
re-order actual and expected
matiasbenedetto Mar 12, 2024
4f4e63c
remove not needed assignement
matiasbenedetto Mar 12, 2024
e655888
do not suppres errors
matiasbenedetto Mar 12, 2024
8e23ae4
format php
matiasbenedetto Mar 12, 2024
25d3c80
use the same format as in the wp-content/fonts default path
matiasbenedetto Mar 12, 2024
030b626
removing database persistence
matiasbenedetto Mar 12, 2024
48d7534
remove wp_default_font_dir function as not needed
matiasbenedetto Mar 12, 2024
171b621
save wp_mkdir_p result in a variable
matiasbenedetto Mar 13, 2024
596f452
check if the dir couldn't be created
matiasbenedetto Mar 13, 2024
9b05b6a
use array syntax
matiasbenedetto Mar 13, 2024
895984b
Update wp_get_font_dir function to use wp_get_upload_dir instead of w…
matiasbenedetto Mar 13, 2024
d2f6e5c
If the `wp-content/uploads/fonts` can not be created or written, an e…
matiasbenedetto Mar 13, 2024
928a564
be consistent in the use of $defaults['path'] to determine the path
matiasbenedetto Mar 13, 2024
9c51258
add a test using chmod (is failing)
matiasbenedetto Mar 13, 2024
e405ba8
format php
matiasbenedetto Mar 13, 2024
c3f21ea
undo unwanted changes
matiasbenedetto Mar 13, 2024
b2a34dd
fix test
matiasbenedetto Mar 13, 2024
e901a3f
Make test more robust
swissspidy Mar 13, 2024
ac77349
Delete files in directory too
swissspidy Mar 13, 2024
5f7b3d1
Fake a failure for creating wp-content/fonts directory.
hellofromtonya Mar 13, 2024
de55afb
Improve texts
matiasbenedetto Mar 14, 2024
e3e8289
improve test message
matiasbenedetto Mar 14, 2024
90a3982
If the font_dir was filtered it uses the filtered values and it doesn…
matiasbenedetto Mar 14, 2024
f7de278
remove error supression
matiasbenedetto Mar 14, 2024
45c8ece
format php
matiasbenedetto Mar 14, 2024
f0c3f3d
rename test case
matiasbenedetto Mar 14, 2024
206543a
Merge branch 'trunk' into add/font-dir-default-non-persisted
matiasbenedetto Mar 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions src/wp-includes/fonts.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,36 @@ function wp_get_font_dir() {
'error' => false,
);

$made_dir = wp_mkdir_p( $defaults['path'] );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()

Copy link
Contributor

@azaozz azaozz Mar 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, good points @peterwilsoncc. I agree 👍

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


if ( ! $made_dir || ! wp_is_writable( $defaults['path'] ) ) {
$upload_dir = wp_get_upload_dir();
$defaults = array(
'path' => path_join( $upload_dir['basedir'], 'fonts' ),
'url' => $upload_dir['baseurl'] . '/fonts',
'subdir' => '',
'basedir' => path_join( $upload_dir['basedir'], 'fonts' ),
'baseurl' => $upload_dir['baseurl'] . '/fonts',
'error' => false,
);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@azaozz azaozz Mar 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

if ( ! $made_dir || ! wp_is_writable( $defaults['path'] ) ) {
if ( str_starts_with( $defaults['path'], ABSPATH ) ) {
$error_path = str_replace( ABSPATH, '', $defaults['path'] );
} else {
$error_path = wp_basename( $defaults['path'] );
}

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

/**
* Filters the fonts directory data.
*
Expand Down
75 changes: 70 additions & 5 deletions tests/phpunit/tests/fonts/font-library/wpFontsDir.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,18 +12,30 @@
*/
class Tests_Fonts_WpFontDir extends WP_UnitTestCase {
private static $dir_defaults;
private static $fake_fonts_file;

public static function set_up_before_class() {
parent::set_up_before_class();

static::$dir_defaults = array(
'path' => path_join( WP_CONTENT_DIR, 'fonts' ),
'url' => content_url( 'fonts' ),
$path = path_join( WP_CONTENT_DIR, 'fonts' );
$url = content_url( 'fonts' );
self::$dir_defaults = array(
'path' => $path,
'url' => $url,
'subdir' => '',
'basedir' => path_join( WP_CONTENT_DIR, 'fonts' ),
'baseurl' => content_url( 'fonts' ),
'basedir' => $path,
'baseurl' => $url,
'error' => false,
);

static::$fake_fonts_file = $path;
}

public function tear_down() {
$this->remove_fonts_directory();
$this->remove_no_new_directories_in_wp_content_fake();

parent::tear_down();
}

public function test_fonts_dir() {
Expand Down Expand Up @@ -69,4 +81,57 @@ function set_new_values( $defaults ) {

$this->assertSame( static::$dir_defaults, $font_dir, 'The wp_get_font_dir() method should return the default values.' );
}

public function test_should_created_fonts_dir_in_uploads_when_fails_in_wp_content() {
matiasbenedetto marked this conversation as resolved.
Show resolved Hide resolved
// Set the expected results.
$upload_dir = wp_upload_dir();
$path = path_join( $upload_dir['basedir'], 'fonts' );
$expected = array(
'path' => $path,
'url' => $upload_dir['baseurl'] . '/fonts',
'subdir' => '',
'basedir' => $path,
'baseurl' => $upload_dir['baseurl'] . '/fonts',
'error' => false,
);

$this->fake_no_new_directories_in_wp_content();
$this->assertFileExists( self::$fake_fonts_file );

$font_dir = wp_get_font_dir();

$this->assertDirectoryDoesNotExist( path_join( WP_CONTENT_DIR, 'fonts' ), 'The `wp-content/fonts` directory should not exist' );
$this->assertDirectoryExists( $font_dir['path'], 'The `uploads/fonts` directory should exist' );
matiasbenedetto marked this conversation as resolved.
Show resolved Hide resolved
$this->assertSame( $expected, $font_dir, 'The font directory should be a subdir in the uploads directory.' );
matiasbenedetto marked this conversation as resolved.
Show resolved Hide resolved
}

private function remove_fonts_directory() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

$directories = array(
path_join( WP_CONTENT_DIR, 'fonts' ),
path_join( WP_CONTENT_DIR, 'uploads/fonts' ),
'/custom-path/fonts/my-custom-subdir',
);

foreach ( $directories as $dir ) {
if ( ! is_dir( $dir ) ) {
continue;
}

$this->rmdir( $dir );
@rmdir( $dir );
}
}

private function fake_no_new_directories_in_wp_content() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created it in a method for 2 reasons:

  1. In the test, makes it more readable and less code.
  2. Reusability. This logic might be useful elsewhere for other Font Library tests. If yes, it can be moved to a trait or base TestCase.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the name is fine

Copy link
Author

@matiasbenedetto matiasbenedetto Mar 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

matiasbenedetto marked this conversation as resolved.
Show resolved Hide resolved
file_put_contents(
self::$fake_fonts_file,
'fake file'
);
}

private function remove_no_new_directories_in_wp_content_fake() {
if ( file_exists( self::$fake_fonts_file ) ) {
@unlink( self::$fake_fonts_file );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed here: f7de278

}
}
}
Loading