Fix naive filtering of uploads directory.

[peterwilsoncc]

Prevent infinite loops getting the font directory.

Fast follow for r57740 / 9c415ed

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 WordPress/gutenberg#58696
Fixes https://core.trac.wordpress.org/ticket/60652

---

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

[github-actions]

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

• The Plugin and Theme Directories cannot be accessed within Playground.
• All changes will be lost when closing a tab with a Playground instance.
• All changes will be lost when refreshing the page.
• A fresh instance is created each time the link below is clicked.
• Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
  it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props peterwilsoncc, swissspidy, mukesh27, mikachan, costdev, mmaattiiaass, azaozz, grantmkin, youknowriad, dd32, jazzs3quence.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[peterwilsoncc]

@matiasbenedetto Ping. Unable to do so by adding you as a reviewer, sorry. I really need to fix that.

[swissspidy]

What I did not like about this prior wp_get_font_dir design is that it‘s actually quite confusing. The name suggests you can just use this function to get the font upload dir. But that‘s not true at all. It‘s only used to temporarily filter the upload location.

So if we want to keep the filter like this, we should really rename it to something like _wp_filter_fonts_upload_dir() or something

[peterwilsoncc]

So if we want to keep the filter like this, we should really rename it to something like _wp_filter_fonts_upload_dir() or something

I agree. I had something to convert wp_get_font_dir() to a getter but decided to take the KISS approach. I'll work on renaming it and figuring out if it needs to go in the GB or wp-dev repo first.

[peterwilsoncc]

@swissspidy The function is called directly and used as a getter during the deletion of posts. I'll look at splitting it out so that WordPress runs a filter on font_dir or something similar.

wordpress-develop/src/wp-includes/fonts.php

Line 191 in b64bfae

$font_dir = wp_get_font_dir()['path'];

[costdev]

I'm not entirely sure that this is needed when this PR was merged in Gutenberg. @peterwilsoncc Can you explain why this is needed aswell?

[matiasbenedetto]

@costdev I think the rationale is here: WordPress/gutenberg#58696 (comment)

[matiasbenedetto]

This is testing fine on my end. I tried these cases:

1. without adding any additional filter.
2. Removing the filter with:

 add_filter( 'upload_dir', function ( $ul ) {
	remove_filter( 'upload_dir', 'wp_get_font_dir' );
	return $ul;
}, 5 );

3. Altering the filter with:

/**
 * Define a custom font directory for the WP Font Library.
 */
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' );

In the 3 cases, the fonts were successfully written in the expected folder:

1. /wp-content/fonts
2. /wp-content/uploads/<year>/<month>/
3. /wp-content/uploads/fonts

---

Apart from that, I agree about that receiving a parameter in wp_get_font_dir( $font_dir = array() ), seems contradictory with its name. It would be nice to split it into two functions as proposed here.

[peterwilsoncc]

I've pushed 19cc1c7 which is nothing more than a thought bubble at this stage.

wp_get_font_dir() and wp_font_dir() are to mirror the names and APIs of the *_upload_dir() functions without the time component.

wp_font_dir() includes a call to wp_upload_dir() to allow removing the filter on upload_dir, 5 when using wp_get_font_dir() as a getter in _wp_before_delete_font_face(). IE this is for consistent behavior with:

 add_filter( 'upload_dir', function ( $ul ) {
	remove_filter( 'upload_dir', 'wp_get_font_dir' );
	return $ul;
}, 5 );

I don't really like the need for extending the number of functions but figured I'd push up the thought bubble so I can get feedback on it and figure out how to take a cleaner approach that allows for consistency using the new functions as a getter.

[peterwilsoncc]

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

wordpress-develop/src/wp-admin/includes/file.php

Lines 975 to 982 in 14d55b8

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

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]

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( 'create' ); // Passing "create" to indicate to make the dir if needed
} 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 know, the "design" with overrides here is super old and non-intuitive (imho). It's been there since WP 1.5 I think.)

[peterwilsoncc]

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. e3a5206 is an example of where it can lead to problems.

[creativecoder]

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]

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]

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]

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,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]

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]

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]

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".

Given that there will be likely more folders like this in the future, we should definitely overhaul this whole thing in the future.

Big +1!

To account for this thinking that this code needs to be "fixed" and any helper functions made private and added to a locked-down class, etc.

[peterwilsoncc]

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]

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

Done in 22764e3

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

[peterwilsoncc]

Committed r57868 / ca8d78e

[azaozz]

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 make a tl;dr list of 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 an infinite loop. This type of "double callbacks" errors are not that easy to see and figure out.
   • 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 not that hard to make new WP private functions not accessible by plugins. It is also considered a "bad behavior" for plugins to use or tamper with such functions, but still some do. When that happens it 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.

[peterwilsoncc]

@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.

[azaozz]

@peterwilsoncc Thanks for you replay.

I'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 #6211 (comment).

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 the 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.