Font Library: "Fetch error: The response is not a valid JSON response" when filtering font_dir.

[jazzsequence]

Description

When testing a filter to change the directory of downloaded fonts, the fonts are not installed and return an error instead. Using the example in #57697 as a base.

Example code:

/**
 * Define a custom font directory for the WP Font Library.
 */
function alter_wp_fonts_dir( $defaults ) {
	$wp_upload_dir = wp_get_upload_dir();
	$uploads_basedir = $wp_upload_dir['basedir'];
	$uploads_baseurl = $wp_upload_dir['baseurl'];

	$fonts_dir = $uploads_basedir . '/fonts';
	// Generate the URL for the fonts directory from the font dir.
	$fonts_url = str_replace( $uploads_basedir, $uploads_baseurl, $fonts_dir );

	$defaults['path'] = $fonts_dir;
	$defaults['url']  = $fonts_url;

	return $defaults;
}
add_filter( 'font_dir', 'alter_wp_fonts_dir' );

Step-by-step reproduction instructions

1. Hook the code above to a function bootstrapped on init.
2. Navigate to the Font Library
3. Navigate to Install Fonts
4. Pick a font to install
5. Click the Install button

Screenshots, screen recording, code snippet

Loom recording

Console:

Environment info

• WordPress 6.5-alpha-57531
• PHP: tested on 7.4.33, 8.1.18, 8.2.5
• Gutenberg: tested on 17.6.1, 17.6.2, 17.6.4
• MacOS Sonoma 14.2.1
• Google Chrome 121.0.6167.85

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

[jazzsequence]

Additional info
Debug Log shows the following:

[06-Feb-2024 21:59:39 UTC] PHP Fatal error:  Allowed memory size of 1073741824 bytes exhausted (tried to allocate 20480 bytes) in /app/wp-includes/functions.php on line 2363

[youknowriad]

cc @matiasbenedetto @creativecoder Any feedback on this issue.

[matiasbenedetto]

The problem is caused by the use of wp_get_upload_dir() in this filter: add_filter( 'font_dir', 'alter_wp_fonts_dir' ).

Why? Because there's an infinite loop of calls to wp_get_upload_dir() when we set the upload di for the fonts:

gutenberg/lib/compat/wordpress-6.5/fonts/class-wp-rest-font-faces-controller.php

Line 861 in 77fec36

add_filter( 'upload_dir', 'wp_get_font_dir' );

I'm trying to figure out a fix for this.

[matiasbenedetto]

After some debugging, I'm not completely sure we should add a fix for this. The infinite loop is caused by the call to wp_get_upload_dir() in a function that is used to filter the upload_dir. This error happens also with code unrelated to the font library.

Let's see this example based on the code from the issue description but removing the parts related to fonts. In this case, I want to alter the upload dir for my media assets:

function alter_wp_upload_dir( $defaults ) {
    $wp_upload_dir = wp_get_upload_dir();
    $uploads_basedir = $wp_upload_dir['basedir'];
    $uploads_baseurl = $wp_upload_dir['baseurl'];

    $custom_dir = $uploads_basedir . '/custom-dir';
    // Generate the URL for the custom directory from the font dir.
    $custom_url = str_replace( $uploads_basedir, $uploads_baseurl, $custom_dir );

    $defaults['path'] = $custom_dir;
    $defaults['url']  = $custom_url;

    return $defaults;
}
add_filter( 'upload_dir', 'alter_wp_upload_dir' );

If I add that PHP snippet and try to upload an image to the media library, I get this error saying that the server ran out of memory. (that happen because of the infinite loop and not because a large image size like the error message affirms).

Given that this is the core behavior, I'm not sure we should add a fix for the issue reported. Maybe the fix needed is a better error message when installing the font face. However, I submitted a draft PR that, at first glance, seems to solve the problem; it would require more testing and I not 100% convinced that it is required, though. #58839

what do you think @jazzsequence @youknowriad @pbking @creativecoder ?

[matiasbenedetto]

@jazzsequence as an immediate fix for the use case you described in the issue, these two alternatives work without any change to Gutenberg, just moving $wp_upload_dir = wp_get_upload_dir() out of the function used to filter font_dir.

• Alternative 1: Use a global.

// Define $wp_upload_dir globally
$wp_upload_dir = wp_get_upload_dir();

function alter_wp_fonts_dir( $defaults ) {
    global $wp_upload_dir; // Make the global variable accessible inside the function

    $uploads_basedir = $wp_upload_dir['basedir'];
    $uploads_baseurl = $wp_upload_dir['baseurl'];

    $fonts_dir = $uploads_basedir . '/fonts';
    // Generate the URL for the fonts directory from the font dir.
    $fonts_url = str_replace( $uploads_basedir, $uploads_baseurl, $fonts_dir );

    $defaults['path'] = $fonts_dir;
    $defaults['url']  = $fonts_url;

    return $defaults;
}
add_filter( 'font_dir', 'alter_wp_fonts_dir' );

• Alternative 2: Use an anonymous function.

$wp_upload_dir = wp_get_upload_dir();

add_filter( 'font_dir', function ( $defaults ) use ( $wp_upload_dir ) {
    $uploads_basedir = $wp_upload_dir['basedir'];
    $uploads_baseurl = $wp_upload_dir['baseurl'];

    $fonts_dir = $uploads_basedir . '/fonts';
    // Generate the URL for the fonts directory from the font dir.
    $fonts_url = str_replace( $uploads_basedir, $uploads_baseurl, $fonts_dir );

    $defaults['path'] = $fonts_dir;
    $defaults['url']  = $fonts_url;

    return $defaults;
} );

[jazzsequence]

@matiasbenedetto Makes sense. I will experiment and report back!

[jazzsequence]

🎉 Using a global to pull the upload dir seems to work. Probably worth noting in any forthcoming documentation around the filter because I see wanting to use the uploads directory (rather than wp-content) as being a pretty common use-case.

Closing this ticket. 👍

[dd32]

The other option to avoid the infinite loop is to remove the filter, as done here:
https://github.com/WordPress/wordcamp.org/pull/1245/files#diff-e441f1053cefcd468bd20fed91d1aac5e902871d7c564be909fc35590f9c3082R635-R637

Also not ideal, but potentially better than a global.

[peterwilsoncc]

@youknowriad @matiasbenedetto I'm reopening this as the solution in #58839 works around the problem rather than fixing the problem.

The existing changes are inadequate as they fail to consider either extenders naively adding the filter in the fasion of other filters, ie add_filter( 'upload_dir', 'wp_get_font_dir' );.

It also relies on future core contributors knowing that they need to use very unintuitive code in order to avoid causing infinite loops. I don't trust myself to remember this, let along a contributor who has not been working on the font library.

It also prevents the removal of the filter via an earlier hook on the upload_dir hook.

A unit test to prove the issue.

public function test_core_should_allow_for_extenders_naively_adding_the_filter() {
	// Naively add the core filter.
	add_filter( 'upload_dir', 'wp_get_font_dir' );

	// Create a font-family post prior to a font being uploaded.
	$font_family_id = wp_insert_post(
		array(
			'post_title'     => 'Open Sans',
			'comment_status' => 'closed',
			'ping_status'    => 'closed',
			'post_status'    => 'publish',
			'post_type'      => 'wp_font_family',
			'post_content'   => wp_json_encode( array( 'fontFamily' => '\\"Open Sans\\"' ) ),
		)
	);

	wp_set_current_user( self::$admin_id );

	// Upload a font via a REST request.
	$font_file = __DIR__ . '/data/OpenSans-Regular.ttf';
	$font_path = wp_tempnam( 'OpenSans-Regular.ttf' );
	copy( $font_file, $font_path );

	$files = array();
	$files[ 'file-' . count( $files ) ] = array(
		'name'      => 'OpenSans-Regular.ttf',
		'full_path' => 'OpenSans-Regular.ttf',
		'type'      => 'font/ttf',
		'tmp_name'  => $font_path,
		'error'     => 0,
		'size'      => filesize( $font_path ),
	);
	$path    = "/wp/v2/font-families/{$font_family_id}/font-faces";
	$request = new WP_REST_Request( 'POST', $path );
	$request->set_param(
		'font_face_settings',
		wp_json_encode(
			array(
				'fontFamily' => '"Open Sans"',
				'fontWeight' => '200',
				'fontStyle'  => 'normal',
				'src'        => array_keys( $files )[0],
			)
		)
	);

	$request->set_file_params( $files );
	$response = rest_get_server()->dispatch( $request );
	$data = $response->get_data();

	// Delete font face post in order to clean up the file used in the tests.
	wp_delete_post( $data['id'], true );

	// Get the uploaded directory. The REST request should have removed the filter.
	$uploads = wp_get_upload_dir();
	$fonts   = wp_get_font_dir();

	// Order the arrays to make the comparison easier.
	ksort( $uploads );
	ksort( $fonts );

	$this->assertNotSame( $uploads, $fonts, 'REST API should remove naively added upload filter' );
}

[peterwilsoncc]

This was fixed in wordpress-develop in r57868 / WordPress/wordpress-develop@ca8d78e

I'll close this off and open an issue for backporting the commit to the gutenberg plugin's 6.5 compat folder.