Fonts Library: change upload directory to wp-content/fonts

[jffng]

What
This PR closes #53965.

How
We can't use wp_handle_upload because this function only works to move files into the uploads directory (or subdirectory). This PR reworks the implementation to rename the temporary file to its new location in the wp-content/fonts directory.

To test

• Ensure the related wpFontFamily tests pass
• Check out Font Library: Frontend [Stage 1] #53884 and verify you can install / uninstall a font successfully.

[github-actions]

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-font-family.php
❔ lib/experimental/fonts/font-library/class-wp-font-library.php
❔ phpunit/tests/fonts/font-library/wpFontFamily/base.php
❔ phpunit/tests/fonts/font-library/wpFontFamily/install.php
❔ phpunit/tests/fonts/font-library/wpFontLibrary/getFontsDir.php
❔ phpunit/tests/fonts/font-library/wpFontLibrary/setUploadDir.php
❔ phpunit/tests/fonts/font-library/wpRestFontLibraryController/installFonts.php

[matiasbenedetto]

@matiasbenedetto what is the best way to test this, or is the tests passing sufficient?

Yep, it should be enough because we are testing if the files exist in the right location.

gutenberg/phpunit/tests/fonts/font-library/wpFontFamily/install.php

Lines 96 to 99 in 22d24f3

	foreach ( $expected as $font_file ) {
	$font_file = static::$fonts_dir . $font_file;
	$this->assertFileExists( $font_file, "Font file [{$font_file}] should exists in the uploads/fonts/ directory after installing" );
	}

Please update the tests to look for the files in the right place. I think we would need to update here, for example:

gutenberg/phpunit/tests/fonts/font-library/wpFontFamily/base.php

Lines 28 to 34 in 22d24f3

	public static function set_up_before_class() {
	parent::set_up_before_class();
	$uploads_dir = wp_upload_dir();
	static::$fonts_dir = $uploads_dir['basedir'] . '/fonts/';
	}

There could be an enhancement opportunity there ☝️ by simplifying and avoiding the need to define the $upload_dir in the set_up method.

[github-actions]

Flaky tests detected in 119f15b.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6029896336
📝 Reported issues:

• [Flaky Test] should not allow blocks to be inserted in inner blocks #44341 in specs/editor/plugins/cpt-locking.test.js

[matiasbenedetto]

After the tests work as expected, making a manual test round would be nice, too. For that, you can checkout the frontend branch: #53884 build, and after that, checkout this branch and use the UI to upload a few fonts.

[jffng]

There are two related tests erroring or failing, which are passing for me locally. Any idea why these would fail in the CI but not locally?

[matiasbenedetto]

There are two related tests erroring or failing, which are passing for me locally. Any idea why these would fail in the CI but not locally?

I suspect that the destination folder is not being created on the CI machine. I'll take a look at that.

Apart from that, @jffng, could you please add your findings about why we could not use wp_handle_upload() function to move the uploaded files?

[jffng]

could you please add your findings about why we could not use wp_handle_upload() function to move the uploaded files?

The current implementation uses the core function wp_handle_upload to move the font files to the correct folder.

If we want to store the fonts in wp-content/fonts, we can't use this function because it is designed to move files within wp-content/uploads only: https://github.com/WordPress/wordpress-develop/blob/6.3/src/wp-admin/includes/file.php#L979-L987.

As far as I can tell, there is no option to override this: https://github.com/WordPress/wordpress-develop/blob/6.3/src/wp-admin/includes/file.php#L776-L789

[matiasbenedetto]

As far as I can tell, there is no option to override this: https://github.com/WordPress/wordpress-develop/blob/6.3/src/wp-admin/includes/file.php#L776-L789

This function is for that:

gutenberg/lib/experimental/fonts/font-library/class-wp-font-library.php

Lines 112 to 115 in 2c0aebd

	public static function set_upload_dir( $defaults ) {
	$defaults['subdir'] = '/fonts';
	$defaults['path'] = $defaults['basedir'] . $defaults['subdir'];
	$defaults['url'] = $defaults['baseurl'] . $defaults['subdir'];

It is called by the install() method before writing the files and it's removed just after to avoid changing the normal work of the rest of the codebase:

gutenberg/lib/experimental/fonts/font-library/class-wp-font-family.php

Lines 544 to 547 in 2c0aebd

	add_filter( 'upload_dir', array( 'WP_Font_Library', 'set_upload_dir' ) );
	$were_assets_written = $this->download_or_move_font_faces( $files );
	remove_filter( 'upload_dir', array( 'WP_Font_Library', 'set_upload_dir' ) );

I just tried something like this, changing these WP_Font_Library methods and it seems to work as expected :

public static function get_fonts_dir() {
    return path_join( WP_CONTENT_DIR, 'fonts' );
}

public static function set_upload_dir( $defaults ) {
	$defaults['basedir'] = WP_CONTENT_DIR;
	$defaults['baseurl'] = content_url();
	$defaults['path']    = self::get_fonts_dir();
	$defaults['url']     = $defaults['baseurl'].'/fonts';
	$defaults['subdir']  = 'fonts';

	return $defaults;
}

I think it would be nice to continue using wp_handle_upload() due to their fine tunning around security and server edge cases.

[matiasbenedetto]

Another way to test manually is running this in your console. It's a POST request to install one font family.
You need to update these parameters to your local dev env:

• location (your test URL)
• header Cookie
• header X-WP-Noce

curl --location 'http://localhost/wp1/wp-json/wp/v2/fonts?' \
--header 'X-WP-Nonce: f8a6ddb873' \
--header 'Cookie: wordpress_logged_in_2deaa0cca6489f796dc77dfbf8093554=admin%7C1694439469%7C1y868xyGGOHTwsT0m6fWjX4ZHmWDURInUlyUtylHvDg%7Cd76eab503a23d6965189b349984e7ef47fd0e24d2047550e7f642ef7596b0ede' \
--header 'Content-Type: application/x-www-form-urlencoded' \
--data-urlencode 'fontFamilies=[{"name":"Aguafina Script","fontFamily":"Aguafina Script, cursive","slug":"wp-font-lib-aguafina-script","category":"handwriting","fontFace":[{"downloadFromUrl":"http://fonts.gstatic.com/s/aguafinascript/v17/If2QXTv_ZzSxGIO30LemWEOmt1bHqs4pgicOrg.ttf","fontWeight":"400","fontStyle":"normal","fontFamily":"Aguafina Script"}]}]'

[jffng]

@matiasbenedetto good suggestion, thank you!

Closing this in favor of #54122.