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

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

Closed
56 changes: 44 additions & 12 deletions src/wp-includes/fonts.php
Original file line number Diff line number Diff line change
Expand Up @@ -92,22 +92,20 @@ function wp_unregister_font_collection( string $slug ) {
}

/**
* Returns an array containing the current fonts upload directory's path and URL.
* Returns the default font directory path and URL.
*
* @since 6.5.0
*
* @return array $defaults {
* Array of information about the upload directory.
*
* @type string $path Base directory and subdirectory or full path to the fonts upload directory.
* @type string $url Base URL and subdirectory or absolute URL to the fonts upload directory.
* @type string $subdir Subdirectory
* @type string $basedir Path without subdir.
* @type string $baseurl URL path without subdir.
* @type string|false $error False or error message.
* }
* @param bool $refresh_cache Optional. Whether to refresh the cache and generate a new default font directory path and URL. Default is false.
* @return array Default font directory path and URL.
*/
function wp_get_font_dir() {
function wp_default_font_dir( $refresh_cache = false ) {
$defaults = get_option( 'font_dir' );
Copy link
Author

Choose a reason for hiding this comment

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

I'm not 100% sure if there are significant benefits in making this persistent. Related: WordPress/gutenberg#59699 (comment)

Copy link
Author

Choose a reason for hiding this comment

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

An non-persisted alternative PR to this one: #6259

Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than store the entire font directory in the option, I'd suggest storing whether it falls back to the secondary location.

This will prevent future contributors from having to manage three scenarios:

  • default
  • fallback
  • option set

It's already complicated enough without allowing extenders to change the path via either an option or a filter.


if ( ! empty( $defaults ) && ! $refresh_cache ) {
return $defaults;
}

$site_path = '';
if ( is_multisite() && ! ( is_main_network() && is_main_site() ) ) {
$site_path = '/sites/' . get_current_blog_id();
Expand All @@ -122,6 +120,40 @@ function wp_get_font_dir() {
'error' => false,
);

wp_mkdir_p( $defaults['path'] );

if ( ! wp_is_writable( $defaults['path'] ) ) {
$defaults = wp_upload_dir();
$defaults['path'] = path_join( $defaults['basedir'], 'fonts' );
$defaults['url'] = $defaults['baseurl'] . '/fonts';
$defaults['subdir'] = '';
$defaults['basedir'] = path_join( $defaults['basedir'], 'fonts' );
$defaults['baseurl'] = $defaults['baseurl'] . '/fonts';
}

Comment on lines +123 to +133
Copy link
Contributor

Choose a reason for hiding this comment

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

Notes from #6259 (review) apply here too.

update_option( 'font_dir', $defaults );
return $defaults;
}

/**
* Returns an array containing the current fonts upload directory's path and URL.
*
* @since 6.5.0
*
* @return array $defaults {
* Array of information about the upload directory.
*
* @type string $path Base directory and subdirectory or full path to the fonts upload directory.
* @type string $url Base URL and subdirectory or absolute URL to the fonts upload directory.
* @type string $subdir Subdirectory
* @type string $basedir Path without subdir.
* @type string $baseurl URL path without subdir.
* @type string|false $error False or error message.
* }
*/
function wp_get_font_dir() {
$defaults = wp_default_font_dir( true );

/**
* Filters the fonts directory data.
*
Expand Down
51 changes: 51 additions & 0 deletions tests/phpunit/tests/fonts/wpDefaultFontDir.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
<?php
/**
* Test wp_default_font_dir().
*
* @package WordPress
* @subpackage Font Library
*
* @group fonts
* @group font-library
*
* @covers ::wp_default_font_dir
*/
class Tests_Fonts_WpDefaultFontDir extends WP_UnitTestCase {

public function test_default_font_dir() {
$font_dir = wp_default_font_dir();

$this->assertSame(
array(
'path' => WP_CONTENT_DIR . '/fonts',
'url' => content_url( 'fonts' ),
'subdir' => '',
'basedir' => WP_CONTENT_DIR . '/fonts',
'baseurl' => content_url( 'fonts' ),
'error' => false,
),
$font_dir,
'The font directory should be a dir inside wp-content'
);
}

public function test_uploads_font_dir() {
$font_dir = wp_default_font_dir( true );
chmod( $font_dir['path'], 0000 );
Comment on lines +33 to +34
Copy link
Author

Choose a reason for hiding this comment

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

I haven't found a way to restrict the writing permissions in the context of unit tests. This chmod line isn't working. It is there just to show how the functionality could be tested.

Copy link
Contributor

Choose a reason for hiding this comment

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

No idea how to test this either. If the option is changed to record the fallback rather than the directory structure then you migh be able to filter it using the pre_option_{$option} hook.


$upload_dir = wp_upload_dir();

$this->assertSame(
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,
),
$font_dir,
'The font directory should be a subdir in the uploads directory.'
);
}
}
Loading