Make WordPress Core

Opened 7 weeks ago

Closed 3 weeks ago

Last modified 9 days ago

#60652 closed defect (bug) (fixed)

font_dir filter enters an infinite loop if wp_get_upload_dir() is used in the filter callback

Reported by: mmaattiiaass's profile mmaattiiaass Owned by: peterwilsoncc's profile peterwilsoncc
Milestone: 6.5.2 Priority: normal
Severity: normal Version: 6.5
Component: General Keywords: has-unit-tests has-patch fixed-major dev-reviewed
Focuses: Cc:

Description

Calling wp_get_upload_dir() inside a font_dir filter callback produces an infinite loop.

Use this snippet and try to install a font using the font library to reproduce the issue:

<?php
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' );

Attachments (2)

60652.diff (1.1 KB) - added by azaozz 3 weeks ago.
60652.1.diff (1.0 KB) - added by azaozz 3 weeks ago.

Download all attachments as: .zip

Change History (60)

This ticket was mentioned in PR #6198 on WordPress/wordpress-develop by @mmaattiiaass.


7 weeks ago
#1

  • Keywords has-patch added

## What?
wp_get_font_dir(): Bail if the font_dir filter is already running.

Props to @costdev for proposing this solution.

## Why?
This avoids an infinite loop that can occur if wp_upload_dir() is called inside a 'font_dir' callback.

## Testing instructions

  1. Use wp_get_upload_dir() inside a function used to filter upload_dir as in the following example:
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' );
  1. Upload fonts using the font library
  2. Check that the fonsts were successfully uploaded to the path set by the filter.

(wp-content/uploads/fonts in the example provided).

---

@swissspidy commented on PR #6198:


7 weeks ago
#2

The one thing I don't like about wp_get_upload_dir() is that it seems like a function that simply returns a directory name. But no, it is actually used for filtering the upload dir.

Things would be much easier if it would behave more like e.g. wp_privacy_exports_dir(), or if we split this up:

  • 1 function to filter the uploads dir
  • 1 function wp_get_upload_dir() that returns the desired location and itself has a filter that hosts/devs can use

@mmaattiiaass commented on PR #6198:


7 weeks ago
#3

Things would be much easier if it would behave more like e.g. wp_privacy_exports_dir(), or if we split this up:
1 function to filter the uploads dir
1 function wp_get_upload_dir() that returns the desired location and itself has a filter that hosts/devs can use

@swissspidy I' don't see how wp_privacy_exports_dir() is different to the trunk version of wp_get_font_dir(). They seem to be doing exactly the same: setting a default and applying a filter after that. Here are the 2 implementations to compare. I removed the comments to make them shorter:

function wp_privacy_exports_dir() {
        $upload_dir  = wp_upload_dir();
        $exports_dir = trailingslashit( $upload_dir['basedir'] ) . 'wp-personal-data-exports/';
        return apply_filters( 'wp_privacy_exports_dir', $exports_dir );
}
function wp_get_font_dir( $defaults = array() ) {
        $site_path = '';
        if ( is_multisite() && ! ( is_main_network() && is_main_site() ) ) {
                $site_path = '/sites/' . get_current_blog_id();
        }

        // Sets the defaults.
        $defaults['path']    = path_join( WP_CONTENT_DIR, 'fonts' ) . $site_path;
        $defaults['url']     = untrailingslashit( content_url( 'fonts' ) ) . $site_path;
        $defaults['subdir']  = '';
        $defaults['basedir'] = path_join( WP_CONTENT_DIR, 'fonts' ) . $site_path;
        $defaults['baseurl'] = untrailingslashit( content_url( 'fonts' ) ) . $site_path;
        $defaults['error']   = false;
        
        return apply_filters( 'font_dir', $defaults );
}

I think wp_privacy_exports_dir() is not entering an infinite loop because it's not used to filter upload_dir as the font library does. Otherwise, the same problem would arise.

https://github.com/WordPress/wordpress-develop/blob/a69052a75fa9bfdbf2e4e80a52b39b1c1233ab91/src/wp-includes/rest-api/endpoints/class-wp-rest-font-faces-controller.php#L859

What do you think about this alternative https://github.com/WordPress/gutenberg/pull/58839 to avoid the infinite loop this PR is trying to fix?

@swissspidy commented on PR #6198:


7 weeks ago
#4

What do you think about this alternative https://github.com/WordPress/gutenberg/pull/58839 to avoid the infinite loop this PR is trying to fix?

This is exactly what I suggested in my comment :)

I' don't see how wp_privacy_exports_dir() is different to the trunk version of wp_get_font_dir().
I think wp_privacy_exports_dir() is not entering an infinite loop because it's not used to filter upload_dir as the font library does.

That's exactly what I said is confusing :)

@costdev commented on PR #6198:


7 weeks ago
#5

Yeah, the alternative looks better to me, nice one both!

@mmaattiiaass commented on PR #6198:


7 weeks ago
#6

@swissspidy @costdev, if the alternative approach is looking better, could you please add a review in that PR?

This ticket was mentioned in PR #6200 on WordPress/wordpress-develop by @mmaattiiaass.


7 weeks ago
#7

## What?
Porting https://github.com/WordPress/gutenberg/pull/58839 from Gutenberg repo.
Fix infinite loop when calling wp_get_upload_dir in a function that's used to filter font_dir.
An alternative approach to: https://github.com/WordPress/wordpress-develop/pull/6198

## Why?
This avoids an infinite loop that can occur if wp_upload_dir() is called inside a 'font_dir' callback.
Fix: https://github.com/WordPress/gutenberg/issues/58696

## Testing instructions

  1. Use wp_get_upload_dir() inside a function used to filter upload_dir as in the following example:
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' );
  1. Upload fonts using the font library
  2. Check that the fonts were successfully uploaded to the path set by the filter.

(wp-content/uploads/fonts in the example provided).

---

@ironprogrammer commented on PR #6200:


7 weeks ago
#9

## Test Report

### Environment

  • System: MacBook Pro Apple M1 Pro, macOS 14.3.1
  • Browser: Safari 17.3.1
  • Server: nginx/1.25.4, PHP 8.2.16, MySQL 8.0.27
  • WordPress: 6.5-beta3-57723-src
  • Theme: twentytwentyfour v1.0
  • Active Plugins:
    • font-dir-mover (mu-plugin with sample font_dir filter code)

### Actual Results

  • ✅ Fonts saved successfully to relocated font storage directory, wp-content/uploads/fonts (+directory creation was successful).

### Supplemental Artifacts
_Figure 1: Before patch._
https://github.com/WordPress/wordpress-develop/assets/824344/0d79688a-d4c4-4763-87c7-bd123df916f0

_Figure 2: After patch._
https://github.com/WordPress/wordpress-develop/assets/824344/965fd421-942a-448c-b040-d54371b71eaf

_Figure 3: Source URL after patch._
https://github.com/WordPress/wordpress-develop/assets/824344/c77e3e71-8612-46fa-872b-924224eb473f

#10 @youknowriad
7 weeks ago

  • Owner set to youknowriad
  • Resolution set to fixed
  • Status changed from new to closed

In 57740:

Editor: Prevent infinite loops when filtering the font library folder.

Changing the font library is something we expect hosts to perform.
It's important that we make this filter as seemless as possible.
This commit prevents a potential infinite loop caused by calling wp_get_upload_dir() within the font_dir filter.

Props mmaattiiaass, ironprogrammer, costdev, swissspidy.
Fixes #60652.

#11 @youknowriad
7 weeks ago

  • Milestone changed from Awaiting Review to 6.5

This ticket was mentioned in PR #6211 on WordPress/wordpress-develop by @peterwilsoncc.


5 weeks ago
#13

  • Keywords has-unit-tests added

Prevent infinite loops getting the font directory.

Fast follow for r57740 / 9c415ed5fb9b4069208b9e43ff774acbfb3ef3a6

This allows for WordPress extenders to filter the uploads directory to the font directory in the typical WordPress fashion, ie add_filter( 'upload_dir', 'wp_get_font_dir' );.

On it's own this would work but can trigger infinite loops on hosts calling wp_upload_dir() or wp_get_upload_dir on the font_dir filter.

It looks like this will need to be committed to wordpress-develop prior to Gutenberg.

Fixes https://github.com/WordPress/gutenberg/issues/58696
Fixes https://core.trac.wordpress.org/ticket/60652

#14 @peterwilsoncc
5 weeks ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening per GB#58696 (comment) to account for naive filtering of the uploads directory.

The linked pull request:

  • modifies the font directory API to match the upload directory API
  • protects against infinite loops without need for closures
  • allows the filter to be remove from upload_dir
  • applies the default font directory as a WP default filter on the font_dir hook

While all of this is needed to prevent infinite loops, some of it will help manage the need to fallback fonts folder.

@azaozz commented on PR #6211:


4 weeks ago
#15

This allows for WordPress extenders to filter the uploads directory to the font directory in the typical WordPress fashion, ie add_filter( 'upload_dir', 'wp_get_font_dir' );.

I'm a bit unsure why would someone want to do this (emphasis added)? Uploads dir is the uploads dir, and the fonts dir is... the fonts dir? Same for the plugins dir, themes dir, etc.?

Yea, the code that implements uploading of fonts reuses wp_handle_upload(), and there is a lambda, non-removable filter to make that work (which doesn't seem needed). Seems it may be a good idea to just remove that add_filter( 'upload_dir', $set_upload_dir ) filter from class-wp-rest-font-faces-controller.php and pass the actual $fonts_dir data as another override to wp_handle_upload()? This should remove any possibilities for loops, etc.

@azaozz commented on PR #6211:


4 weeks ago
#16

Sorry, didn't mean to close :(

@peterwilsoncc commented on PR #6211:


4 weeks ago
#17

May be missing something but it seems it may be a good idea to just remove that add_filter( 'upload_dir', $set_upload_dir ) filter from class-wp-rest-font-faces-controller.php and pass the actual $fonts_dir data as another override to wp_handle_upload()? This should remove any possibilities for loops, etc.

@azaozz There is no way to pass a custom directory as an override. The upload handlers simply call wp_upload_dir()

https://github.com/WordPress/WordPress/blob/ecd4a277b7bc285f8483a9545fcad4cdc747ea9d/wp-admin/includes/file.php#L975-L983

A use case for an extender may be to make use of the feature outside of the site-editor. For example via the customizer for a classic theme or elsewhere for a hybrid theme. I'm not sure what else extenders will come up with but I'm often impressed by their ingenuity when extending WP.

My main concern with the lambda filter is that it works around the bug rather than fixes the bug. I've worked a lot on the feature over the last few weeks and don't trust myself to remember that I need a work-around if adding code elsewhere, let alone another contributor who has been less involved.

At a defensive coding level, if it's just as simple to fix a bug as to work around it then Core should always go with the former.

@azaozz commented on PR #6211:


4 weeks ago
#18

The upload handlers simply call wp_upload_dir()

Right. So the "override" could probably be something as simple as:

if ( isset( $overrides['font_upload'] ) ) {
    $uploads = wp_get_font_dir( /* make the dir */ );
} else {
    $uploads = wp_upload_dir( $time );
}

/*
 * A writable uploads dir will pass this test. Again, there's no point
 * overriding this one.
 */
if ( ! ( $uploads && false === $uploads['error'] ) ) {
    return call_user_func_array( $upload_error_handler, array( &$file, $uploads['error'] ) );
}

Internally wp_get_font_dir() uses wp_upload_dir() if /fonts has to be moved there. Both of these functions have filters, etc. so extenders will have no problems hooking/tweaking anything they need?

@peterwilsoncc commented on PR #6211:


4 weeks ago
#19

I think we'd need to reproduce wp_upload_dir() and some of _wp_upload_dir() rather than use the filter.

Mainly, though, it's as you say: it's extending an unintuitive API. It's also pretty easy to accidentally remove some of the security built in to the handlers using it so encouraging it's use further doesn't seem like a good idea. e3a520606dcfec7aec65b8fe94822183882f3055 is an example of where it can lead to problems.

@grantmkin commented on PR #6211:


4 weeks ago
#20

I've been working through this PR over the last couple of days, and here are my notes:

Given the constraints of

  • Not modifying wp_handle_upload
  • Wanting to directly apply wp_font_dir to the upload_dir filter
  • While also using wp_get_upload_dir() in a function applied to the font_dir filter

I can't find a better way of handling this than what we have in this PR.

---

That said, stepping back, the implementation does still seem a bit odd to me, and I wonder if that's a hint there's a better way. For example, the default font path is created add_filter( 'font_dir', 'wp_default_font_dir', 5 );. If that default filter is removed and no others are added, the font directory becomes to the (filtered) value wp_upload_dir, which doesn't seem right.

That leads me to wondering, what other filters are out there in the wild for upload_dir? What are the chances that those will override the font-faces endpoint filtering of upload_dir and put fonts in a location intended for uploads? Perhaps, at the least, the upload_dir filter in the font-faces endpoint should be running on a later priority. Or maybe we should modify wp_handle_upload to more directly account for uploading fonts vs. uploads, as @azaozz suggested.

---

Also, if the most common use case for changing the fonts directory is moving it to a different, singular location (e.g. /uploads/fonts), but not doing anything dynamic, perhaps the addition of a constant and/or a canonical plugin would handle a lot of the concern more simply. Those seem like they would be easier to implement for the majority of cases, and the complexity of using the font_dir filter as is seems like less of a problem.

@peterwilsoncc commented on PR #6211:


4 weeks ago
#21

That said, stepping back, the implementation does still seem a bit odd to me, and I wonder if that's a hint there's a better way. For example, the default font path is created add_filter( 'font_dir', 'wp_default_font_dir', 5 );. If that default filter is removed and no others are added, the font directory becomes to the (filtered) value wp_upload_dir, which doesn't seem right.

That's fair. I can rip that out if you wish and merge it in to a renamed wp_apply_font_dir_filters.

A part of my thinking was that it would make it easier for extenders and hosts to switch to /uploads/fonts but I see your point -- it makes it a little to easy to switch to uploads/2024/03/font.woff

@peterwilsoncc commented on PR #6211:


4 weeks ago
#22

That leads me to wondering, what other filters are out there in the wild for upload_dir? What are the chances that those will override the font-faces endpoint filtering of upload_dir and put fonts in a location intended for uploads? Perhaps, at the least, the upload_dir filter in the font-faces endpoint should be running on a later priority. Or maybe we should modify wp_handle_upload to more directly account for uploading fonts vs. uploads, as @azaozz suggested.

I did a search of the plugin repo for `["'upload_dir['"]`] and couldn't find much in the way of late filtering of the uploads directory that will concern us -- most late filtering appeared to be targeting a particular plugins uploads rather than all uploads.

I'm still not keen on adding another override to handling uploads. Even accepting that $overrides['font_upload'] works (and it likely would) I think it's premature to start adding context options to upload directories and upload handlers until further information is received about future first class objects.

I don't want to introduce a specific font_upload override in the final week of the release cycle, only to learn in a few releases that there's a need for another context and we've locked WordPress in to a pattern focused on a single feature.

@youknowriad commented on PR #6211:


4 weeks ago
#23

I've been looking at this PR a bit and I'm not entirely sure I follow everything just yet.

This allows for WordPress extenders to filter the uploads directory to the font directory in the typical WordPress fashion, ie add_filter( 'upload_dir', 'wp_get_font_dir' );.

Personally, I agree with @azaozz I don't know why anyone would do this and if I'm reading properly, it's because there's no way to indicate the target directory properly in the wp upload function. So say, I think I'd favor improving the upload primitive instead.

But improving the upload primitive, whether that means a newer function or modifications to the existing function or something else feels like a bigger undertaking at this point.

So I'm actually fine with this PR.

@swissspidy commented on PR #6211:


4 weeks ago
#24

So,I think I'd favor improving the upload primitive instead. But improving the upload primitive, whether that means a newer function or modifications to the existing function or something else feels like a bigger undertaking at this point.

That's also my key takeaway here. Given that there will be likely more folders like this in the future, we should definitely overhaul this whole thing in the future. That also means any modifications now should keep that in mind.

So the approach here to fix the infinite loop seems fine to me as a start.

Just a couple of suggestions/thoughts on the code itself:

  • function wp_get_font_dir() { return wp_font_dir( false ); } is a bit pointless. I don't see the need for this extra wrapping function. Can we just remove this, and rename wp_font_dir back to wp_get_font_dir like it was?
  • Maybe prefix wp_filter_font_directory with an underscore for consistency with other "private" functions.

@azaozz commented on PR #6211:


4 weeks ago
#25

Given the constraints of

Not modifying wp_handle_upload
...

I can't find a better way of handling this than what we have in this PR.

Yep, I agree. The current code implements the needed functionality despite the constrains. My guess is that these constrains are left over from when that functionality was implemented in a plugin. (There is a better way to do that btw. Instead of trying to solve similar cases by imposing constrains, some temporary code can be introduced while WP is in development, and removed before release. All "feature plugins" can request such temp code, filters, settings, constants, etc.)

The question is: why not modify wp_handle_upload() in a simple, easy to follow, backwards compatible way? Imho the current patch here has higher chances to introduce errors/edge cases, and, as it uses nested filters, possible interference from existing code.

The fact is that wp_handle_upload() is a function that is re-used for new functionality. It is a standard practice to extend the functionality of the existing code to accommodate the new requirements.

Yes, it is usually possible to use filters to accomplish the same tasks. However this doesn't make sense unless the filters are designed to also be used by plugin and theme authors. I.e. the filters that are used to extend the existing WP code and implement the new functionality would need to "make sense" and be useful for plugins. I somehow don't think this is the case here.

@azaozz commented on PR #6211:


4 weeks ago
#26

But improving the upload primitive, whether that means a newer function or modifications to the existing function or something else feels like a bigger undertaking at this point.

Hmm, I tend to disagree. Adding support for another (simple) parameter to a function seems quite "safer" as there is no chance to break backwards compatibility or to introduce edge cases to existing code.

On the other hand using nested filters is a lot more complex, has some chance to introduce both back-compat problems and edge cases with existing plugins or themes.

So I'm actually fine with this PR.

Thinking that if this patch is to be added to WP 6.5 few days before release at least the new functions have to be private, hidden, and not accessible by plugins in any way. That would allow this code to be improved and these functions to be removed much easier, instead of having to support them "forever".

In that terms this code needs to be fixed and any new "helper" functions added to a locked-down class, etc.

@peterwilsoncc commented on PR #6211:


4 weeks ago
#27

Thinking that if this patch is to be added to WP 6.5 few days before release at least the new functions have to be private, hidden, and not accessible by plugins in any way. That would allow this code to be improved and these functions to be removed much easier, instead of having to support them "forever".

The new functions need to be accessible to WordPress so are required to be public.

I agree with @azaozz I don't know why anyone would do this and if I'm reading properly, it's because there's no way to indicate the target directory properly in the wp upload function. So,I think I'd favor improving the upload primitive instead.

I'm not sure what extenders will do either. A part of my concern is for future WordPress contributors unaware of the history. As the FL gets extended we need to avoid the infinite loop trap as a defensive coding technique.

  • function wp_get_font_dir() { return wp_font_dir( false ); } is a bit pointless. I don't see the need for this extra wrapping function. Can we just remove this, and rename wp_font_dir back to wp_get_font_dir like it was?

This mirrors the uploads API, wp_upload_dir() and wp_get_upload_dir, one creates the directory and the other does not. Having $create_dir default to true in one API and false in another seems quite unintuitive.

@peterwilsoncc commented on PR #6211:


4 weeks ago
#28

Maybe prefix wp_filter_font_directory with an underscore for consistency with other "private" functions.

Done in 22764e3650e38b56737f5f558d9a2125710b08dc

In cf211ae8117adbdc844ddd70bef04301c01c094f I added a code sample for using the filter and have confirmed the docs-parser correctly handles it.

#29 @swissspidy
4 weeks ago

  • Owner changed from youknowriad to peterwilsoncc
  • Status changed from reopened to assigned

#30 @peterwilsoncc
4 weeks ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 57868:

Editor: Prevent font folder naive filtering causing infinite loops.

This modifies the font directory API to more closely reflect the upload directory API to help account for naive filtering when uploading fonts.

This moves the protection of infinite loops to the new function _wp_filter_font_directory() to allow developers extending and maintaining the font library to apply the filter without the need for a closure.

These changes also ensure both the upload_dir and font_dir filter are applied consistently when both creating and deleting fonts faces. Prior to this commit the upload_dir filter was only fired when creating fonts faces via the REST API.

Applying the font directory filter to the upload_dir filter is now done by adding the _wp_filter_font_directory function rather than wp_get_font_dir(). Developers who have previously modified the font upload directory using the font_dir filter will NOT need to upload their code.

Extenders wishing to upload files to the font directory can do so via the code:

<?php
add_filter( 'upload_dir', '_wp_filter_font_directory' );
// Your code to upload or sideload a font file.
remove_filter( 'upload_dir', '_wp_filter_font_directory' );

Introduces:

  • wp_font_dir(): Attempt to create and retrieve the font upload directory. The equivalent to wp_upload_dir().
  • _wp_filter_font_directory(): To run on the upload_dir filter, this sets the default destination of the fonts directory and fires the font_dir filter.

wp_get_font_dir() has been modified to be a lightweight getter for the font directory. It returns the location without attempting to create it. The equivalent to wp_get_upload_dir().

Follow up to [57740].

Props peterwilsoncc, mukesh27, mikachan, costdev, mmaattiiaass, swissspidy, youknowriad, dd32, grantmkin.
Fixes #60652.

#32 @peterwilsoncc
4 weeks ago

  • Keywords dev-feedback added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for committer sign-off and backport consideration to the 6.5 branch

#33 @azaozz
4 weeks ago

  • Keywords needs-patch added; has-patch removed

Think the inline docs/docblocks need fixing after [57868]. For example: it introduces a private function _wp_filter_font_directory() but the docs have "recommended usage" section? As far as I'm aware it is not recommended for plugins and themes to use any of the WP functions marked as private.

In addition thinking there should be a warning that the above function will be removed in WP 6.6 (as discussed in the PR) as the implementation there will be revised. It is pretty unfortunate that [57868] exposes this function (it was a lambda function "hidden" in handle_font_file_upload() before), but thinking that at least the extenders can be warned not to use it as it will go away in 6.6.

Last edited 4 weeks ago by azaozz (previous) (diff)

#34 @azaozz
4 weeks ago

Follow up: #60835.

#35 follow-up: @swissspidy
4 weeks ago

Given the latest updates this might even need to be reverted, so let‘s hold off on going into details and opening follow-up tickets just yet.

#36 in reply to: ↑ 35 @azaozz
4 weeks ago

Replying to swissspidy:

this might even need to be reverted...

Yea, that's possible. However the follow-up is about removing the restrictions/constrains when implementing handling of font uploads, mostly the first one. See https://github.com/WordPress/wordpress-develop/pull/6211#issuecomment-2011172935. Seems that the code around there would need some larger changes in the way it works.

#37 @swissspidy
3 weeks ago

  • Keywords dev-reviewed fixed-major added; dev-feedback removed

#38 follow-up: @swissspidy
3 weeks ago

I see, that wasn't really clear from the ticket, it sounded very generic.

Given the latest update, I'm proceeding with merging [57868] and [57868] into the 6.5 branch so we can unblock the release.

#39 @swissspidy
3 weeks ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 57879:

Editor: Prevent font folder naive filtering causing infinite loops.

This modifies the font directory API to more closely reflect the upload directory API to help account for naive filtering when uploading fonts.

This moves the protection of infinite loops to the new function _wp_filter_font_directory() to allow developers extending and maintaining the font library to apply the filter without the need for a closure.

These changes also ensure both the upload_dir and font_dir filter are applied consistently when both creating and deleting fonts faces. Prior to this commit the upload_dir filter was only fired when creating fonts faces via the REST API.

Applying the font directory filter to the upload_dir filter is now done by adding the _wp_filter_font_directory function rather than wp_get_font_dir(). Developers who have previously modified the font upload directory using the font_dir filter will NOT need to upload their code.

Extenders wishing to upload files to the font directory can do so via the code:

<?php
add_filter( 'upload_dir', '_wp_filter_font_directory' );
// Your code to upload or sideload a font file.
remove_filter( 'upload_dir', '_wp_filter_font_directory' );

Introduces:

  • wp_font_dir(): Attempt to create and retrieve the font upload directory. The equivalent to wp_upload_dir().
  • _wp_filter_font_directory(): To run on the upload_dir filter, this sets the default destination of the fonts directory and fires the font_dir filter.

wp_get_font_dir() has been modified to be a lightweight getter for the font directory. It returns the location without attempting to create it. The equivalent to wp_get_upload_dir().

Follow up to [57740].

Reviewed by swissspidy.
Merges [57868] to the 6.5 branch.

Props peterwilsoncc, mukesh27, mikachan, costdev, mmaattiiaass, swissspidy, youknowriad, dd32, grantmkin.
Fixes #60652.

#40 @azaozz
3 weeks ago

  • Keywords dev-reviewed fixed-major removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

As mentioned above the docblock for the new _wp_filter_font_directory() function seems wrong. This is a private function not intended for use by plugins and themes, and as far as I see it is pretty likely it will be deprecated and will stop working in WP 6.6.

Patch coming up.

@azaozz
3 weeks ago

#41 @azaozz
3 weeks ago

In 60652.diff : Fix the docblock for _wp_filter_font_directory().

As this is a documentation only change, it can be committed at any time before the release. However it is still preferable to commit it asap as the release is in few days. @mmaattiiaass, @peterwilsoncc, @swissspidy, please review if you get a chance.

Last edited 3 weeks ago by azaozz (previous) (diff)

#42 in reply to: ↑ 38 @azaozz
3 weeks ago

Replying to swissspidy:

that wasn't really clear from the ticket, it sounded very generic.

Sorry about that. Please feel free to modify the ticket description as you see fit :)

#43 follow-up: @peterwilsoncc
3 weeks ago

I'm happy with the docblock as it stands.

The design of the fonts PHP API is that such that the only way extenders can upload or sideload to the font directory is to apply the filter to the uploads directory. Until a discussion is had about to deal with different upload contexts it's premature to declare that the filter will be removed.

That said, it's up to @swissspidy and @davidbaumwald as core tech leads. As the easter long weekend has started here, I will leave it up to them.

Last edited 3 weeks ago by peterwilsoncc (previous) (diff)

#44 follow-up: @swissspidy
3 weeks ago

I'm -1 on prematurely adding docblocks saying "this function will be deprecated" because that definitely is not certain. This is a private function that won't be listed in the function reference anyway, so this is only relevant for people looking at the source code directly.

#45 in reply to: ↑ 43 ; follow-up: @azaozz
3 weeks ago

Replying to peterwilsoncc:

I'm happy with the docblock as it stands.

Sorry but seems I'm missing something.

The current docblock encourages plugin authors to use a WordPress private function. This is wrong in principle and should never happen imho.

In addition it seems that this private function is part of an implementation that is not the best. It was added to WP with the excuse that there is not enough time to improve the code, and an improvement would most likely be made in WP 6.6.

the only way extenders can upload or sideload to the font directory

You mean the existing wp_font_dir() and the 'font_dir' filter are inadequate and the extenders shouldn't or couldn't use them? Then why was this committed in the first place?

That said, it's up to @swissspidy and @davidbaumwald as core tech leads.

I'll be happy if @swissspidy and/or @davidbaumwald have a look here. However not sure if this docs change has anything to do with any particular role like "core tech lead", etc. This is a documentation change, not a change to production code.

Last edited 3 weeks ago by azaozz (previous) (diff)

#46 in reply to: ↑ 44 @azaozz
3 weeks ago

Replying to swissspidy:

I'm -1 on prematurely adding docblocks saying "this function will be deprecated"

Sure, will remove that. New patch coming up.

this is only relevant for people looking at the source code directly

Yep, was thinking some more background there may be helpful, but agree that docblocks probably shouldn't make any promises :)

Last edited 3 weeks ago by azaozz (previous) (diff)

@azaozz
3 weeks ago

#47 @azaozz
3 weeks ago

In 60652.1.diff: Improve the patch by removing the part about deprecating the private function.

#48 @azaozz
3 weeks ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 57902:

Docs: Improve the docblock for _wp_filter_font_directory(). Remove the recommended use section as this is a private use only function.

Props peterwilsoncc, swissspidy, azaozz.
Fixes #60652.

#49 @azaozz
3 weeks ago

  • Keywords has-patch fixed-major added; needs-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopen for 6.5.

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


3 weeks ago

#51 @audrasjb
3 weeks ago

  • Milestone changed from 6.5 to 6.5.1

Moving this to 6.5.1 as per today's dry run.

#52 @audrasjb
3 weeks ago

  • Milestone changed from 6.5.1 to 6.5

Moving back to 6.5 as it's just a Docs change.
Backporting it right now.

#53 @davidbaumwald
3 weeks ago

  • Keywords dev-reviewed added
  • Milestone changed from 6.5 to 6.5.1

Looks good to merge to the 6.5 branch. @audrasjb will handle the commit.

#54 @audrasjb
3 weeks ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 57907:

Docs: Improve the docblock for _wp_filter_font_directory().

Removes the recommended use section as this is a private use only function.

Reviewed by davidbaumwald.
Merges [57902] to the 6.5 branch.
Props peterwilsoncc, swissspidy, azaozz.
Fixes #60652.

#55 in reply to: ↑ 45 ; follow-up: @peterwilsoncc
2 weeks ago

Replying to azaozz:

Sorry but seems I'm missing something.

The current docblock encourages plugin authors to use a WordPress private function. This is wrong in principle and should never happen imho.

In addition it seems that this private function is part of an implementation that is not the best. It was added to WP with the excuse that there is not enough time to improve the code, and an improvement would most likely be made in WP 6.6.

I'm surprised I need to but I will explain how the Fonts PHP API works:

Nothing substantial has changed since r57619 was committed on the day of Beta 1.

In order to upload or sideload files, it's required to add a filter to the upload_dir filter. Without applying the filter files will upload to the default uploads/yyyy/mm directory.

I agree that it would probably be better if an upload context was added to the upload handlers and related functions but a ticket for the enhancement was not opened during the development of the Fonts API so by the time it was committed to beta 1 it was too late to do so.

the only way extenders can upload or sideload to the font directory

You mean the existing wp_font_dir() and the 'font_dir' filter are inadequate and the extenders shouldn't or couldn't use them? Then why was this committed in the first place?

The Fonts API can be extended by Core developers, theme or plugin developers. As the API will be primarily maintained by contributors to wordpress-develop, it's important to note how it works.

wp_font_dir() is for getting the upload directory. It can be for deleting files while removing font-face post types.

The API committed on the day of beta 1 attempted to use it during filtering of uploads too but that introduced an infinite loop bug. A suggestion was made in Slack on how to fix this but the suggestion was ignored and r57740 used a workaround introducing a maintenance burden for future contributors.

The only way to avoid the infinite loop was to use a separate function for filtering the directory during uploads.

The pull request was open for several weeks and reviewed by multiple core contributors. Once I had followed up their suggestions I pinged them again to remind them the ticket needed a follow up review so it could be committed.


Documentation is both for Core contributors and extenders. I think the documentation had been made inferior in the patch that has been committed and it would have been better practice to wait for further comment before committing it.

#56 in reply to: ↑ 55 ; follow-up: @azaozz
2 weeks ago

Replying to peterwilsoncc:

I'm surprised I need to but I will explain how the Fonts PHP API works:

Thanks for the explanation.

In order to upload or sideload files, it's required to add a filter to the upload_dir filter. Without applying the filter files will upload to the default uploads/yyyy/mm directory.

Think I see where the misunderstanding comes from. It is not required to use the upload_dir filter in order to upload a font. It is only required to pass the correct information/context about where the /fonts directory is located, as stated below.

I agree that it would probably be better if an upload context was added to the upload handlers and related functions but a ticket for the enhancement was not opened during the development of the Fonts API so by the time it was committed to beta 1 it was too late to do so.

Yes, this seems to be the other part of the misunderstanding. Imho not using a filter when one is not needed (and causes infinite loops) is an essential part of this code, not an enhancement. Fixing the infinite loop possibility by removing the filter is the correct patch here, regardless of beta or RC.

Anyway, planning to fix that in #60835 and commit early to 6.6.

#57 in reply to: ↑ 56 @peterwilsoncc
2 weeks ago

Replying to azaozz:

Think I see where the misunderstanding comes from. It is not required to use the upload_dir filter in order to upload a font. It is only required to pass the correct information/context about where the /fonts directory is located, as stated below.

I agree that it would probably be better if an upload context was added to the upload handlers and related functions but a ticket for the enhancement was not opened during the development of the Fonts API so by the time it was committed to beta 1 it was too late to do so.

What I am saying is that for WordPress 6.5 it is required.

Opening an enhancement for WordPress 6.6 to introduce an upload context is fine but it still needs to be documented how uploading works for WordPress 6.5 so maintainers are aware of it.

If you can provide a unit test that shows the filter is not required for uploading to 6.5 then we can document that approach instead.

#58 @davidbaumwald
9 days ago

  • Milestone changed from 6.5.1 to 6.5.2

Milestone renamed

Note: See TracTickets for help on using tickets.