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

Font Library: add wp_get_font_dir() function. #57730

Merged
merged 4 commits into from
Jan 10, 2024
Merged

Conversation

matiasbenedetto
Copy link
Contributor

@matiasbenedetto matiasbenedetto commented Jan 10, 2024

What?

Adds wp_get_font_dir() function.

Why?

Emulates wp_get_upload_dir() but for the fonts directory.

How?

Implementing wp_get_font_dir() function.

Testing Instructions

Ways to test:

  1. Use the font library to install fonts.

  2. Run PHP unit tests.

  3. Use the function in extension code (theme/plugin) as here:

$fonts = wp_get_font_dir();
echo ( print_r($fonts) );

should print:

Array
(
    [path] => /var/www/html/wp1/wp-content/fonts
    [url] => http://localhost/wp1/wp-content/fonts
    [subdir] => 
    [basedir] => /var/www/html/wp1/wp-content/fonts
    [baseurl] => http://localhost/wp1/wp-content/fonts
    [error] => 
)
  1. Modify the value using the filter:
function custom_font_dir( $defaults ) {
		$defaults['path']    = '/custom/path/';
		$defaults['url']     = 'https://example.com/custom/url';
		$defaults['basedir'] = '/custom/path/';
		$defaults['baseurl'] = 'https://example.com/custom/url';
	return $defaults;
}

add_filter( 'font_dir', 'custom_font_dir' );

$fonts = wp_get_font_dir();
echo ( print_r($fonts) );

should print:

Array
(
    [path] => /custom/path/
    [url] => https://example.com/custom/url
    [subdir] => 
    [basedir] => /custom/path/
    [baseurl] => https://example.com/custom/url
    [error] => 
)

@matiasbenedetto matiasbenedetto changed the title Font Library: add wp_font_dir() function. Font Library: add wp_get_font_dir() function. Jan 10, 2024
Copy link

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
❔ lib/experimental/fonts/font-library/class-wp-rest-font-families-controller.php
❔ lib/experimental/fonts/font-library/font-library.php
❔ phpunit/tests/fonts/font-library/wpFontFamily/base.php
❔ phpunit/tests/fonts/font-library/wpRestFontFamiliesController/base.php
❔ phpunit/tests/fonts/font-library/fontsDir.php

@matiasbenedetto matiasbenedetto added [Feature] Typography Font and typography-related issues and PRs [Type] Feature New feature to highlight in changelogs. labels Jan 10, 2024
@matiasbenedetto matiasbenedetto requested a review from jffng January 10, 2024 17:13
Copy link
Contributor

@jffng jffng left a comment

Choose a reason for hiding this comment

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

Still works as expected and tests pass, I left a couple minor comments.

*
* @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
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I should have asked in the previous PR, but are we including $subdir just to match the wp_upload_dir function? What is the use for it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think we should include the subdir property to match wp_get_upload_dir() because we are using the output of this function to update upload_dir filter before uploading fonts. Currently, we are not using it in the font library related code. It is used just in core's _wp_upload_dir to set the /year/month sub directory:

https://github.com/WordPress/wordpress-develop/blob/ddb67a0099c9912c46a22374bcf90e5aac2f25e3/src/wp-includes/functions.php#L2503-L2524

}

// Sets the defaults.
$defaults['path'] = path_join( WP_CONTENT_DIR, 'fonts' ) . $site_path;
Copy link
Contributor

Choose a reason for hiding this comment

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

The function and filter are named singularly (font_dir), but the default path is plural (fonts). Does it make sense that these are different?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so because we are following the pattern from core, the filter name is upload_dir and the folder is uploads.

@matiasbenedetto matiasbenedetto merged commit d8ef7d6 into trunk Jan 10, 2024
58 checks passed
@matiasbenedetto matiasbenedetto deleted the add/wp_font_dir branch January 10, 2024 19:05
@github-actions github-actions bot added this to the Gutenberg 17.6 milestone Jan 10, 2024
@getdave getdave added the Needs PHP backport Needs PHP backport to Core label Jan 15, 2024
@youknowriad youknowriad added Backported to WP Core Pull request that has been successfully merged into WP Core and removed Needs PHP backport Needs PHP backport to Core labels Feb 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backported to WP Core Pull request that has been successfully merged into WP Core [Feature] Typography Font and typography-related issues and PRs [Type] Feature New feature to highlight in changelogs.
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants