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

[mmaattiiaass]

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

## 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' );

2. Upload fonts using the font library
3. Check that the fonsts were successfully uploaded to the path set by the filter.

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

---

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

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?

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 :)

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

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

## 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' );

2. Upload fonts using the font library
3. Check that the fonts were successfully uploaded to the path set by the filter.

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

---

Closing this PR in favour of: ​https://github.com/WordPress/wordpress-develop/pull/6200

## 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._

_Figure 2: After patch._

_Figure 3: Source URL after patch._

[youknowriad]

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.

Commit https://core.trac.wordpress.org/changeset/57740

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

[peterwilsoncc]

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.

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.

Sorry, didn't mean to close :(

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.

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?

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.

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.

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

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.

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.

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.

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.

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.

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.

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.

[peterwilsoncc]

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.

Committed r57868 / ​https://github.com/WordPress/wordpress-develop/commit/ca8d78e5f2f50c59749398b5932e246ada0c0a37

[peterwilsoncc]

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

[azaozz]

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.

[azaozz]

Follow up: #60835.

[swissspidy]

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.

[azaozz]

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.

[swissspidy]

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.

[swissspidy]

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.

[azaozz]

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]

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.

[azaozz]

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 :)

[peterwilsoncc]

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.

[swissspidy]

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.

[azaozz]

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.

[azaozz]

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 :)

[azaozz]

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

[azaozz]

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.

[azaozz]

Reopen for 6.5.

[audrasjb]

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

[audrasjb]

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

[davidbaumwald]

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

[audrasjb]

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.

[peterwilsoncc]

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.

[azaozz]

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.

[peterwilsoncc]

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.

[davidbaumwald]

Milestone renamed

This is a comment on a closed PR that was committed to WP 6.5 but for posterity's sake I think it would be good to list the points from the above reviews.

1. This code does not actually fix the infinite loop problem as specified in the corresponding Trac ticket, it only "hides" it. The (possible) error that developers may make here is to call a function from its callback. This will always result in an infinite loop, and hiding it is quite unhelpful to the developers. Then they cannot find their error easily and may even miss it and release their code with it.

Imagine if PHP suddenly decided to not throw a syntax error when somebody made a typo in their code. How bad/annoying/unhelpful would this be? :) Unfortunately the code in this PR is something like the WordPress equivalent of this.

2. This PR introduces an anti-pattern in WP: running a filter in another filter's callback. This is the main cause for the infinite loop and is a pretty bad idea for several reasons:
   • Developers may cause an infinite loop by calling the function where the first filter runs in their callback on the second filter. In this case calling wp_upload_dir() from the callback on the font_dir filter causes the infinite loop.
   • Plugins may remove the callback that contains the font_dir filter breaking the core API. This is a significant defect as it makes that WP functionality unreliable.
   • Finally this code introduces yet another "private" function in the global scope in WP. This is not a huge problem but such code should be avoided in new functionality. It is considered a "bad behavior" for plugins to tamper with such functions, but still some do. This needlessly makes WP core and plugin development harder.

I'd also want to make is clear that I'm not trying to blame anybody whit this review. This PR was reviewed and committed during the "late" RC stage of WP 6.5, only few days before the (planned) release. Everybody was under a significant pressure at that time, and mistakes can happen at any point. The follow-up Trac ticket that fixes these issues is https://core.trac.wordpress.org/ticket/60835.

@azaozz I'm not going to debate the merit of defensive coding so future maintainers of the code can perform upkeep without the risk of breaking a filter been used in the way it is intended to be used. It's just pointless.

This PR was approved by a tech lead after multiple reviews in the two weeks leading up to its commit.

@peterwilsoncc Thanks for you replay.

'm not going to debate the merit...

Sorry that I didn't make myself clear enough. I'm not discussing or debating anything here. The above comment is an attempt for a tl;dr of my review of this PR, nothing more.

This PR was approved by a tech lead...

Right. It was approved on the condition that this code will be fixed in WP 6.6. See ​https://github.com/WordPress/wordpress-develop/pull/6211#issuecomment-2011793974.

without the risk of breaking a filter been used...

Sorry but not sure what you mean by that. Are you saying that it is okay for newly introduced font_dir filter to be inconsistent? or perhaps that it is optional? How would plugins and eventually core use that filter then?
In any case, if you want to talk about how to fix this code, please lets do that on the Trac ticket for it: https://core.trac.wordpress.org/ticket/60835.