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 global configuration variables for font directory #57044

Closed
wants to merge 4 commits into from

Conversation

madhusudhand
Copy link
Member

What?

It adds following configuration variables for the font path.

  • WP_FONT_DIR: absolute path where fonts are uploaded to and loaded from. When not set, plugin will fall back to WP_CONTENT_DIR/fonts as default value.
  • WP_FONT_URL: full url to the font directory. It should be set when a custom path is set for WP_FONT_DIR. When not set, it defaults to content_url( 'fonts' )

Why?

It enables to set a custom directory as font directory.

Testing Instructions

Case 1:

  1. Upload / install a font from the font library modal in global styles.
  2. It should by default upload the fonts to /wp-content/fonts and the font url should be set to site_url/wp-content/fonts/your-font.ttf

Case 2:

  1. Define the following variables in wp-config.php
  2. define( 'WP_FONT_DIR', '/var/www/html/wp-content/new-font-dir');
  3. define( 'WP_FONT_URL', '//your.site/wp-content/new-font-dir');
  4. Now repeat the above steps to install a font.
  5. Fonts must be installed in the new path.

Also, validate the following use cases.

  1. with and without a trailing slash
  2. with and without protocol defined in the URL

Screenshots or screencast

NA

Related

Related discussion is here

fixes #55063

@madhusudhand madhusudhand added [Type] Enhancement A suggestion for improvement. [Feature] Typography Font and typography-related issues and PRs labels Dec 14, 2023
@madhusudhand madhusudhand self-assigned this Dec 14, 2023
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-library.php
❔ phpunit/tests/fonts/font-library/wpFontLibrary/setUploadDir.php

Copy link

Flaky tests detected in 8bfa658.
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/7206100374
📝 Reported issues:

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.

Nice one @madhusudhand!

This worked as expected under all the test conditions provided. We should be sure to document this one when the time comes.

Copy link
Contributor

@creativecoder creativecoder left a comment

Choose a reason for hiding this comment

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

I don't have the full history of feedback here, but my first instinct is to treat the fonts directory more like the uploads folder, rather than the theme or plugin folders.

That is, add a filter for it (like upload_dir) rather than add a constant for it.

Also, is there a need to define WP_FONT_DIR and WP_FONT_URL separately? Wouldn't the url need to be based on the dir path?

@jffng
Copy link
Contributor

jffng commented Dec 14, 2023

add a filter for it (like upload_dir) rather than add a constant for it.

I believe we are already filtering it:

add_filter( 'upload_dir', array( 'WP_Font_Library', 'set_upload_dir' ) );

Also, is there a need to define WP_FONT_DIR and WP_FONT_URL separately? Wouldn't the url need to be based on the dir path?

Could there be cases where the URL may differ from where it is stored in the filesystem?

If we do move forward with separate constants, it's worth noting that it's necessary to define both. Or a compromise could be to base the URL on the WP_FONT_DIR constant if it's defined but the WP_FONT_URL is not.

@creativecoder
Copy link
Contributor

I believe we are already filtering it:

I mean adding an apply_filters( 'fonts_dir', ... call directly within WP_Font_Library::get_fonts_dir, and probably creating a global function wp_fonts_dir that wraps that.

Comment on lines -129 to -173
$defaults['basedir'] = WP_CONTENT_DIR;
$defaults['baseurl'] = content_url();
$defaults['subdir'] = '/fonts';
Copy link
Contributor

@creativecoder creativecoder Dec 14, 2023

Choose a reason for hiding this comment

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

Is anything lost by not setting basedir, baseurl, and subdir here? It seems like unexpected things could happen if some of the properties for upload_dir are changed and some are not, but I haven't used this filter before.

Copy link
Member Author

Choose a reason for hiding this comment

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

Based on the definition of wp_upload_dir, when path is not set, basedir + subdir are used, and when url is not set, baseurl + subdir are used. I think we are not loosing anything here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Alternatively, we could switch to basedir + subdir where basedir becomes the path set by the variable WP_FONT_DIR and subdir will be the site_id (in case of multi site) and empty for simple sites.
Please refer this

@creativecoder
Copy link
Contributor

Also, is there a need to define WP_FONT_DIR and WP_FONT_URL separately?

Looking more closely, I see that this is what we do for WP_CONTENT_DIR, WP_PLUGIN_DIR, and WPMU_PLUGIN_DIR, so it makes sense to match that convention.


Thinking about this a little more, a filter seems quite useful here, for its flexibility.

As an example I could imagine wanting to sort your font files into subdirectories by font family or by file type (similar to how media uploads can be sorted into subfolders). A filter would support adding logic to do this.


Looking at how uploads works brings multisite to mind. Have we thought about this at all? Should each site have its own font folder, or should there be one shared folder? If the fonts are shared, how do we make sure one site doesn't delete another site's fonts?

Multisite seems like a piece of the puzzle for how we add font directory configuration. 🤔

@madhusudhand
Copy link
Member Author

madhusudhand commented Jan 8, 2024

@creativecoder

Also, is there a need to define WP_FONT_DIR and WP_FONT_URL separately? Wouldn't the url need to be based on the dir path?

Since the path for WP_FONT_DIR can be outside WordPress installation, a composed URL for directory outside wouldn't be available via http. In this case a separate http server may serve those assets via URLs and that needs to be configured via this variable.
This is the same case for WP_UPLOAD_DIR as well.

Multisite seems like a piece of the puzzle for how we add font directory configuration. 🤔

Similar to how uploads work, a sub directory /sites/site_id is created for the sub-site fonts in case of a multi site. Font files of the root site are still kept under the main font directory.

@creativecoder
Copy link
Contributor

I'll go ahead and close this one, since it's been incorporated into and replaced by #57697

@mikachan mikachan deleted the font-lib/add-config-variable branch January 11, 2024 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Typography Font and typography-related issues and PRs [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Font Library: Support alternate paths for fonts (WP_FONTS_DIR)
4 participants