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

Architectural discussion allowing for fonts folder fallbacks. #59699

Closed
peterwilsoncc opened this issue Mar 7, 2024 · 12 comments
Closed

Architectural discussion allowing for fonts folder fallbacks. #59699

peterwilsoncc opened this issue Mar 7, 2024 · 12 comments
Labels
[Feature] Font Library [Type] Discussion For issues that are high-level and not yet ready to implement.

Comments

@peterwilsoncc
Copy link
Contributor

Following the discussion in #59417 and the related make post, it has been decided to prefer placing fonts in wp-content/fonts and fallback to wp-content/uploads/fonts if the directory is not writable.

In order to get the best possible outcome for this decision, the following will need to be decided upon:

Fallbacks

  • Once the fallback has occurred is the fallback used for all future uploads regardless of whether the file system settings change
  • What's the most performant way to determine the state of fallbacks
  • Is the fallback status for individual fonts stored in post meta to allow for the truncated file paths in the existing _wp_font_face_file post meta

Server relocations from one file system type to another

  • Are the post objects deleted if the file can not be deleted?
  • How to gracefully handle developer/site owner knowledge of fallbacks
    • site owners who know to move the font files
    • site owners who do not know to move the font files

File location

What is the file structure for multisite installs using the fallback location?

  • wp-content/uploads/site/X/fonts/*.*, or
  • wp-content/uploads/fonts/site/X/*.*

No doubt this discussion will raise further questions that can be edited in to this issue. (If you have edit rights on the repo, please sign off on the edit.)

@youknowriad
Copy link
Contributor

Once the fallback has occurred is the fallback used for all future uploads regardless of whether the file system settings change

This feels like the ideal for me, can we persist it using a site option or something? It's also important to consider the filter. I'm guessing the filter always takes over, even if previous fonts were uploaded to the default (or fallback) folders?

@noahtallen
Copy link
Member

Providing some context from the enterprise hosting side of things (read-only filesystem), we use upload_dir to change the upload directory to point at our external object store. So ultimately, this looks like vip://wp-content/uploads/fonts, rather than just the upload directory.

It's a small difference, but just to be specific, we'd want it to fallback to wp_upload_dir(). Our filter implementation for changing the font dir looks like this:

$upload_dir = wp_upload_dir();

$defaults['basedir'] = $upload_dir['basedir'] . '/fonts';
$defaults['baseurl'] = $upload_dir['baseurl'] . '/fonts';
$defaults['path']    = $defaults['basedir'];
$defaults['url']     = $defaults['baseurl'];

return $defaults;

@swissspidy
Copy link
Member

Thanks for opening the ticket and start the discussion! I wanted to chime in earlier, but WC Asia and travelling came in the way.

Once the fallback has occurred is the fallback used for all future uploads regardless of whether the file system settings change
This feels like the ideal for me, can we persist it using a site option or something? It's also important to consider the filter. I'm guessing the filter always takes over, even if previous fonts were uploaded to the default (or fallback) folders?

Good question. What do we gain by making this persistent (e.g. on database upgrade)? I don't see any benefits.

The regardless of whether the file system settings change part is key here. The idea behind the strategy described in the make/core post is for WordPress to figure the right location so that the user does not need to worry about it. If we persist the location and then the permissions change, font uploads would no longer work, which is the scenario we want to avoid.

So I'd suggest not making this persistent.

What's the most performant way to determine the state of fallbacks

Appreciate the callout to performance here 🙏

Given that the location is only really needed during creation & deletion operations, I don't see an issue with just doing a writable check on demand. Something like this (pseudo code):

function wp_get_font_dir() {
	$dir = 'wp-content/uploads/fonts';
	if ( wp_is_writable( 'wp-content/fonts' ) ) {
		$dir = 'wp-content/fonts';
	}

	return apply_filters( '...', $dir );
}

Is the fallback status for individual fonts stored in post meta to allow for the truncated file paths in the existing _wp_font_face_file post meta

The upload location can change, either because of filesystem permission changes or because of filters. So we should just store the full path IMO.

What is the file structure for multisite installs using the fallback location?

Good question. What is the current file structure for non-fallbacks on multisite? Is wp-content/fonts/ also divided per site right now?

For the fallback location, I think the main site should probably use wp-content/uploads/fonts and sub-sites would use wp-content/uploads/site/X/fonts/. That's in line with regular file uploads today and year/month based folders.

@aaronjorbin
Copy link
Member

Thinking about the use of filters and the fallback a bit, I think it would make sense to have a short circuit for when a site has chosen its own directory. So something along the lines of:

$upload_dir = apply_filters( 'pre_wp_get_font_dir', null);
if (! $upload_dir ){
	$dir = 'wp-content/uploads/fonts';
	if ( wp_is_writable( 'wp-content/fonts' ) ) {
		$dir = 'wp-content/fonts';
	}
}
return apply_filters( '...', $dir );

@matiasbenedetto
Copy link
Contributor

👋 I submitted a PR with a draft of the fallback default font dir: WordPress/wordpress-develop#6252

@peterwilsoncc
Copy link
Contributor Author

Good question. What is the current file structure for non-fallbacks on multisite? Is wp-content/fonts/ also divided per site right now?

The non-fallback directory uses the same sub-folder structure as the upload directory, ie wp-content/fonts/sites/x/*.*

For the fallback location, I think the main site should probably use wp-content/uploads/fonts and sub-sites would use wp-content/uploads/site/X/fonts/. That's in line with regular file uploads today and year/month based folders.

This makes sense to me. Given the fallback is to the uploads directory then let's use the default uploads directory. I think this will make falling back a little less complex too.

Is the fallback status for individual fonts stored in post meta to allow for the truncated file paths in the existing _wp_font_face_file post meta

The upload location can change, either because of filesystem permission changes or because of filters. So we should just store the full path IMO.

This is the most difficult one. The full file path /path/to/site/wp-content/(uploads/)?fonts may change in the event of server relocations, as may the content directory in the event of a server move.

I like the idea of storing a more complete path though, maybe it could be relative to wp-content? This idea is untested against offloading plugins so may not work and is something to test.

Good question. What do we gain by making this persistent (e.g. on database upgrade)? I don't see any benefits.

The regardless of whether the file system settings change part is key here. The idea behind the strategy described in the make/core post is for WordPress to figure the right location so that the user does not need to worry about it. If we persist the location and then the permissions change, font uploads would no longer work, which is the scenario we want to avoid.

I like @youknowriad's idea of making the fallback persistent with something like:

if ( '1' === get_option( 'wp_use_fonts_folder_fallback', '0' || ! wp_is_writable( /* default */ ) {
   update_option( 'wp_use_fonts_folder_fallback', '1', 'no' );
  return /* fallback */;
}

return /*default*/;

But I don't have super strong feelings on it.

@matiasbenedetto
Copy link
Contributor

The regardless of whether the file system settings change part is key here. The idea behind the strategy described in the make/core post is for WordPress to figure the right location so that the user does not need to worry about it. If we persist the location and then the permissions change, font uploads would no longer work, which is the scenario we want to avoid.

So I'd suggest not making this persistent.

I agree. I do not find too much value in persisting the path. I imagine that for extenders, it could be a little unexpected to have that path stored in the database (no other base path is persisted like that) and unclear how to refresh it.

I submitted another PR without the database persistence: WordPress/wordpress-develop#6259

@peterwilsoncc
Copy link
Contributor Author

The reason I like the idea of the database option is that it allows for:

  • some sort of flakiness with the file system or relocation from one file system type to another
  • ensuring that WordPress attempts to delete the file in the correct directory
  • helps ensure that the reported directory in the site health feature is correct

@aaronjorbin
Copy link
Member

I think that storing the location of where things have been uploaded is valuable, but in my eyes the decision of where to upload should be made at runtime since systems can change especially when it deals with people moving between environments.

@peterwilsoncc
Copy link
Contributor Author

As the likely PR is using non-persistence then it will need to include some indicator of where the file is stored in each posts meta data (simply whether it's the default or the fallback, not the full path). Without it, there's no way of knowing where to attempt deletion of the file in _wp_before_delete_font_face(). This is in order to account for site moves/server re-configurations.

@jazzsequence
Copy link

Noting here that the current behavior (check if wp-content is writeable, default to wp-content/fonts if so, write to wp-content/uploads/fonts by default if not) creates a discrepancy between local development and the server environment if the font_dir filter is not used. Locally, almost always wp-content will be writeable. On read-only filesystems, it may not be.

That means that installing a font on your server would install to /uploads/fonts but when you are testing locally, your fonts would be in /fonts. When you sync your code, you might accidentally commit changes to wp-content/fonts/ if you had installed fonts locally. If you pull your database and files from your server to your local environment, anything you installed locally will disappear (which might be fine) assuming there are database records for the path to the actual file location in uploads/fonts.

I don't necessarily think this should block or change the behavior of having a fallback. However, it does not change the need for hosts to use the font_dir filter to define a single, canonical path for fonts so that local development matches the server environment.

@peterwilsoncc
Copy link
Contributor Author

After learning a little more about including a fallback within Core by default, its' been decided not to include the feature in 6.5. An announcement post has posted on the Make Core blog.

The original feature announcement dev note has been updated to reflect this change.


For now I will close this ticket as unplanned as there isn't a maybe later option on GitHub. The issue can be reopened if it is decided to include the fallback later.

@peterwilsoncc peterwilsoncc closed this as not planned Won't fix, can't repro, duplicate, stale Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Font Library [Type] Discussion For issues that are high-level and not yet ready to implement.
Projects
Status: Done
Development

No branches or pull requests

7 participants