Fix and improve handling of uploading of font files

[azaozz]

Follow up to #60652 and [57868].

As discussed at ​https://github.com/WordPress/wordpress-develop/pull/6211 the code that enables uploading of font files by replacing the output of wp_handle_upload() by using the upload_dir filter is not the best solution. In addition it implements nested filters that can be pretty confusing so understand and/or use for plugin and theme authors.

A more straightforward solution is needed for this and similar cases in the future.

[azaozz]

Given the constraint of "Do not modifying wp_handle_upload()", the current code in WP 6.5 makes sense. However there is no such constrain when implementing new features in core, hence the current code can be (and imho should be) improved to implement that functionality in a better way. This will also serve as an example of now to resolve similar cases in the future (without imposing such constrains).

[peterwilsoncc]

[57868] may need a follow up but any changes must retain protections against an infinite loop without the need for closures to avoid the risks of future maintainers and extenders introducing bugs. Tests_Fonts_WpFontDir::test_fonts_dir_filters_do_not_trigger_infinite_loop() will need to be retained in some form to ensure against this.

[azaozz]

Replying to peterwilsoncc:

any changes must retain protections against an infinite loop without the need for closures to avoid the risks of future maintainers and extenders introducing bugs

Yes, definitely. Seems that the nested filters can be avoided if wp_handle_upload() and/or wp_get_upload_dir() are updated/modified. That would completely remove the possibility for an infinite loop caused by using these filters.

Improve fonts upload dir handling logic.

Trac ticket: #60835

This reintroduces the infinite loop bug fixed in #60652 when filtering the font directory. It looks to be a wider issue than the original as the unit tests are timing out due to entering a loop so the relevant test isn't running.

$ ./vendor/bin/phpunit --filter test_fonts_dir_filters_do_not_trigger_infinite_loop
Installing...
Running as single site... To run multisite, use -c tests/phpunit/multisite.xml
Not running ajax tests. To execute these, use --group ajax.
Not running ms-files tests. To execute these, use --group ms-files.
Not running external-http tests. To execute these, use --group external-http.
PHPUnit 9.6.19 by Sebastian Bergmann and contributors.

Warning:       Your XML configuration validates against a deprecated schema.
Suggestion:    Migrate your XML configuration using "--migrate-configuration"!

F                                                                   1 / 1 (100%)

Time: 00:05.709, Memory: 214.27 MB

There was 1 failure:

1) Tests_Fonts_WpFontDir::test_fonts_dir_filters_do_not_trigger_infinite_loop
Filtering the uploads directory triggered an infinite loop.

It looks to be a wider issue than the

[khokansardar]

Improve fonts upload handling logic

This reintroduces the infinite loop bug fixed in #60652 when filtering the font directory. It looks to be a wider issue than the original as the unit tests are timing out due to entering a loop so the relevant test isn't running.

$ ./vendor/bin/phpunit --filter test_fonts_dir_filters_do_not_trigger_infinite_loop
Installing...
Running as single site... To run multisite, use -c tests/phpunit/multisite.xml
Not running ajax tests. To execute these, use --group ajax.
Not running ms-files tests. To execute these, use --group ms-files.
Not running external-http tests. To execute these, use --group external-http.
PHPUnit 9.6.19 by Sebastian Bergmann and contributors.

Warning:       Your XML configuration validates against a deprecated schema.
Suggestion:    Migrate your XML configuration using "--migrate-configuration"!

F                                                                   1 / 1 (100%)

Time: 00:05.709, Memory: 214.27 MB

There was 1 failure:

1) Tests_Fonts_WpFontDir::test_fonts_dir_filters_do_not_trigger_infinite_loop
Filtering the uploads directory triggered an infinite loop.

It looks to be a wider issue than the

@peterwilsoncc I have updated the logic of function handle_font_file_upload in WP_REST_Font_Faces_Controller

[azaozz]

Replying to khokansardar:

Thanks for the patch. I've been working on a patch too, will probably be ready tomorrow. I'll ping you for a review when I add it. It takes a different approach, removes the nested filters completely, which also removes any possibility of infinite loop.

As I said above using nested filters there is not a particularly good idea. It is how a plugin may do this. However this is core, there is no need to "filter a filter" :)

BTW, been having some "second thoughts" on that infinite loop caused by improper use of a filter. This is a "developer error". Of course there will be an infinite loop when you call the parent function from a callback (well, it is a little more complex here, but still the same thing: a callback should not call any of its parent, grandparent, great-grandparent, etc. functions).

Seems that hiding this kind of developer error doesn't actually help anybody. The developer is left wondering why their code doesn't work as there is no clue that they did something wrong.

But it seems too late now. Seems we will have to go along with preventing the infinite loop and not letting the developers know that they made a mistake.

In any case, not having a possibility to call the parent function from a filter and cause an infinite loop seems best.

Remove the use of nested filters. Add params to wp_handle_upload() and wp_upload_bits() instead.

Trac ticket: https://core.trac.wordpress.org/ticket/60835

[azaozz]

@khokansardar Could you test ​https://github.com/WordPress/wordpress-develop/pull/6407.

@matiasbenedetto Sorry for the ping but do you remember why the (temp) filter
add_filter( 'upload_mimes', array( 'WP_Font_Utils', 'get_allowed_font_mime_types' ) );
in handle_font_file_upload() was needed? The same array of allowed mime types is already passed to wp_handle_upload() in the $overrides, seems that should be enough?

[peterwilsoncc]

My review requesting changes on ​PR#6707 didn't get cross posted to this ticket so, I'll do so manually...

I think it's too late do do this for the font directory, the best time do have done something like this would have been to log an enhancement mid last year when the feature was been worked on. No such enhancement was logged.

This breaks backward compatibility for developers removing the font directory filter in the simplest way to avoid an infinite loop:

<?php
add_filter( 'upload_dir', function ( $ul ) {
        remove_filter( 'upload_dir', '_wp_filter_font_directory' );
        return $ul;
}, 5 );

As there has been talk of moving the font directory WordPress needs to maintain backward compatibility for site owners removing the filter, either because:

• file system issues
• deployment scripts using rsync (this being the most likely scenario)

I think it's too late...

You mean it is too late to fix some poor code in WP? How come?

This breaks backward compatibility for developers removing the font directory filter in the simplest way to avoid an infinite loop

Sorry but what backwards compatibility is broken by completely removing the filter that was causing the infinite loop? The code above still works as expected as there is no filter to remove and no infinite loop.

If you can still trigger an infinite loop after the changes in this PR, please provide an example of how to do it.

Also, as I mentioned earlier at https://core.trac.wordpress.org/ticket/60835#comment:9, hiding an infinite loop caused by improper use of a filter (a developer error) is not a good thing to do. Frankly I think that not telling developers that they are doing something wrong is a bad thing. Would have been much better to warn them about that possibility here instead of quietly hiding it and leaving them in the dark.

Sorry but what backwards compatibility is broken by completely removing the filter that was causing the infinite loop? The code above still works as expected as there is no filter to remove and no infinite loop.

The backward compatibility break is this:

• prior to this change the files are uploaded to the default uploads directory
• with this change the developers wishes are ignored and the file is uploaded to the default font directory (which you have advocated to change in the future)

hiding an infinite loop caused by improper use of a filter (a developer error) is not a good thing to do.

Prior to the changes I committed during the RC cycle, an infinite loop was triggered by using the filter in the exact manner for which is was designed: See ​the dev-note.

Furthermore WordPress itself was using an anti-pattern/coding standards violation to work around rather than fix it (a closure preventing the removal of a filter when another solution was available).

You mean it is too late to fix some poor code in WP? How come?

I don't understand why you where happy with a filter for the six or so months you worked on this feature but have suddenly decided that the approach is no longer appropriate. Why didn't you open an enhancement at the time?

Re The backward compatibility break: that's in reference to the code sample I provided earlier.

The backward compatibility break is this: prior to this change the files are uploaded to the default uploads directory

I see. This is actually an interesting question. Plugins should not use "private" functions (private in quotes as the functions are just marked as private in the docs, but still accessible). If a plugin uses a private function it implies it is "doing it wrong".

But what if a plugin "disables" a private function, or bypasses it in some way (when this is not intended)? Should that also be considered "doing it wrong"?

I'm not sure if removing the upload_dir filter in order to prevent setting of the /fonts directory is a supported behaviour. There is a font_dir filter for exactly this purpose, right?

In general it seems this is a "pretty bad behaviour" as that will also remove the font_dir filter (it will never run) which would break all/any plugins that are using it. Even thinking that it may be worth it to try to add a call to _doing_it_wrong() in such cases. It seems pretty bad for a plugin to hinder the operation of other plugins.

Luckily this is a "brand new" functionality and there are no plugins that are doing anything like this: ​https://wpdirectory.net/search/01HVWE9SGPTSR5TATTWGXTCCRZ. Thinking that preventing such code from running would actually be an enhancement and should not be blocked. This seems to be not only unexpected use, but also harmful to other plugins and the overall integrity of the WP API there.

I don't understand why you where happy with a filter for the six or so months you worked on this feature but have suddenly decided...

Yes would have been a lot better/easier if that change was done in time. I wasn't working on this, was just helping from time to time when needed. Unfortunately I didn't review the code thoroughly (like most other committers) and saw this at approximately the same time you saw it :(

Thinking more about the above example: it seems it would be a lot better to consider this use case, and make it easier for plugins to implement it. Seems a very straightforward way would be to add the $upload_dir as another param to the font_dir filter, something like this:

$font_dir = apply_filters( 'font_dir', $font_dir, $upload_dir );

Then it will be very easy for a plugin to change anything, and will not need to do another wp_upload_dir() call. Seems faster, simpler, and easier to use.

When I documented the function as private, I was trying to indicate that it shouldn't be called directly, ie _wp_filter_font_directory(), but rather only used by filtering the uploads directory (a core filter using the core functions add|remove_filter().

These comments were subsequently removed despite this been mentioned prior to the commit.

was trying to indicate that it shouldn't be called directly, ie _wp_filter_font_directory(), but rather only used by filtering...

Don't think a private function should be used by plugins, ever. This is just "bad behaviour". What's the point of having private functions then? :)
Also don't think plugins should interfere with default WP filters that use private functions, unless this is an elaborate design decision.

Adding docs about how to use private functions seems to defeat the purpose. I'm actually thinking if/how WP can warn developers that they are using something they shouldn't be, even maybe throw and error and exit when WP is in developer mode. I'll open a trac ticket with more details, If you disagree with this please lets take the conversation there.

In this case, it's probably best to remove the private delegation.

WordPress is really stuck with the filtering approach now that it's been introduced. The WordPress importer is going to need to use the filter to support the importing of the font face post type.

With the talk of defining what constitutes first and second class objects and uploads, and subsequently the addition of new first class objects it's premature to lock the overrides in to an architecture that may or may not be suitable long term. To do so risks adding further complications to uploads/sideloads and the various associated functions.

WordPress is really stuck with the filtering approach now that it's been introduced

Stuck? Why? Nothing is using this "bad behaviour" at the moment, and if a plugins tries to use it I think it should be considered as breaking the requirement for not interfering with other plugins. Seems such plugins should not be hosted by WP and should be removed from the plugins repo when detected. This is similar to a plugin that is disabling/deactivating other plugins...

The WordPress importer is going to need to use the filter...

Which filter are you talking about? The font_dir filter (that is being removed by the example code) or the upload_dir, and why? Seems the importer(s) should be using the font_dir filter when determining where fonts are stored, anything else would be bad code.

To do so risks adding further complications to uploads/sideloads and the various associated functions.

Not sure I understand exactly what you mean here, but there is a filter that is designed for WP and plugins to use when determining where the /fonts directory is located. That filter is font_dir. Any plugin that would prevent this filter from even running is breaking this part of the WP API and should be considered "harmful".

Yea, I agree it is not a good design to add a filter that is in the callback to another filter. Unfortunately this was made quite worse after https://core.trac.wordpress.org/ticket/60652 and https://core.trac.wordpress.org/changeset/57868. Seems that ticket doesn't seem to fix anything but only makes things worse. It not only hides the developer error, but also makes it possible for plugins to "behave badly" and stop a WP filter from running completely (i.e. mangle the WP API).

Frankly I think only this would be enough of a reason to fix that code.

Seems such plugins should not be hosted by WP and should be removed from the plugins repo when detected.

Not all plugins are in the plugin repo. The plugins are using the filter in the intended fashions, as seen on ​WordCamp (since reverted following updates in Core), ​Pantheon and ​WP VIP.

As I've explained before, this approach of calling wp_get_upload_dir() was ​advised in the dev-note.

Importantly, the defensive coding is for future maintainers of WordPress rather than for plugin authors.

The WordPress importer is going to need to use the filter...

Which filter are you talking about? The font_dir filter (that is being removed by the example code) or the upload_dir, and why? Seems the importer(s) should be using the font_dir filter when determining where fonts are stored, anything else would be bad code.

The importer will need to use the filter in order to support WordPress 6.5 and to store uploads in the correct location for future versions of WordPress.

---

You seem intent on making the font library code more difficult to maintain rather than proceeding with the discussions that came out of the entire frustrating experience.

Before making any changes to how the font library works the following discussions need to occur:

1. What constitutes a first and second class object in WordPress
2. First class object that may be introduced in the future

Both of these discussions affect how the introduction of an upload context can be managed. Should the context be added up the upload/sideload functions alone or should it also be added to to wp_get_upload_dir() too.

The plugins are using the filter in the intended fashions, as seen on ​WordCamp (since reverted following updates in Core), ​https://github.com/pantheon-systems/pantheon-mu-plugin/pull/29 and ​https://github.com/Automattic/vip-go-mu-plugins/pull/5265.

Hm, thinking we may be misunderstanding one another. I'm not talking about plugins that use the font_dir filter. I'm talking about plugins that may be using the example code you posted in ​https://github.com/WordPress/wordpress-develop/pull/6407#pullrequestreview-2010091916, i.e. plugins that do remove_filter( 'upload_dir', '_wp_filter_font_directory' );.

This code will break all of the above mentioned plugins as it removes the font_dir filter completely. Imho this is a serious problem that has to be fixed immediately.

As I've explained before, this approach of calling wp_get_upload_dir() was ​advised in the dev-note.

So instead of fixing the error in the docs, and explaining what not to do as a developer, the actual code has to be changed to hide that error? I mean, this particular infinite loop may not happen after the patch from ​https://github.com/WordPress/wordpress-develop/pull/6211, however if a developer makes the error that causes it, chances are that their code will not work properly and now it is quite harder to see and fix it. Don't think that's good.

You seem intent on making the font library code more difficult to maintain...

Exactly the opposite. The current code is very hart to maintain and also makes future development there much harder.

What would happen when a plugin disable the font_dir filter, and another plugin wants to use it? I.e. when a plugin uses the example code you posted above:

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

and another plugin or eventually core wants to use the font_dir filter, it will need to ensure that filter runs. So it will have to add something like this:

add_filter( 'upload_dir', function ( $ul ) {
        if ( ! has_filter( 'upload_dir', '_wp_filter_font_directory' ) ) {
            add_filter( 'upload_dir', '_wp_filter_font_directory' );
        }
        return $ul;
}, 9 );

Of course, this will break the first plugin that disabled the font_dir filter.

Don't think this is a good and proper way for WP core to work. Requiring plugins to "step on one another's toes" and pretty much break one another. So thinking that the possibility for this misuse of the current functionality should be fixed asap, even maybe in a dot release.

...do you remember why the (temp) filter

add_filter( 'upload_mimes', array( 'WP_Font_Utils', 'get_allowed_font_mime_types' ) );
in handle_font_file_upload() was needed? The same array of allowed mime types is already passed to wp_handle_upload() in the $overrides, seems that should be enough? If not enough, there might be another place the current WP code may need fixing/updating.

@azaozz I recalling going back and forth on whether the filter, the override, or both were needed while ​developing the endpoint. At one point we had only the filter, but it seemed that the override was also needed to restrict the file types. I don't know if we checked removing the filter and only using the override, but probably not.

I agree now that it seems only the override is needed, as I can see the override value takes precedence a over the filter value when wp_check_filetype is called during the upload.

it seems only the override is needed, as I can see the override value takes precedence...

@creativecoder Yep, same here. I'll try to look a bit more if it makes sens to keep add_filter( 'upload_mimes', array( 'WP_Font_Utils', 'get_allowed_font_mime_types' ) );.

the font upload will fail if the directory doesn't already exist...
but I can see the change causing breakage

Thanks for testing! Yea, I can't see why using the font_dir filter may change whether the directory is created or not (assuming PHP can write to the filesystem location). This (should) depend on the $create_dir param in wp_font_dir( $create_dir = true ) which is true by default.

Quickly tried it here (again) and seems to be working as expected, but will try to "break it", with and without this patch.

@creativecoder Found the cause. It was creating the /fonts dir from $font_dir['basedir'] but that should have been $font_dir['path']. These two values are generally the same (I was setting both too when testing), but a plugin would probably change only the path, not basedir. That's also how the unpatched code works atm. Thanks for spotting!

@peterwilsoncc It's been more than a month since you "requested changes" for this patch, but there is still no code showing how this code can be improved. Are you going to suggest some changes or maybe you've reconsidered?

The plugins are using the filter in the intended fashions, as seen on ​WordCamp (since reverted following updates in Core), ​https://github.com/pantheon-systems/pantheon-mu-plugin/pull/29 and ​https://github.com/Automattic/vip-go-mu-plugins/pull/5265.

Looking at these examples, they all will break when a plugin does:

remove_filter( 'upload_dir', '_wp_filter_font_directory' );

As there are still plugins in the WP repository that do this, and the examples of usage outside of the WP repository clearly show how damaging that may get, I think this "opportunity" for plugins to break the WP plugins API should be fixed asap.

It's really unfortunate that such code was added to WP 6.5 just few days before the release date. But I understand, mistakes can happen at any time.

It's been more than a month since you "requested changes" for this patch, but there is still no code showing how this can be improved and what changes you're actually requesting

@azaozz My last comment was two weeks ago but you ​asked for alternative reviews in the dev chat shortly after so I figured it was going to go the way of b094b53a26600ca4cd6329919f5b3c3f55613c76 and feedback was being ignored.

My requests remain unchanged:

1. Avoid breaking backward compatibility for plugins using the upload_dir filter. Removing the filter is a legitimate way for developers who wish to use the standard upload destination for font files, eg 2024/05. This PR breaks backward compatibility for plugin authors doing that (yes, font_dir will no longer fire but that's fine if that's the developer's intent)
2. Before making any changes to how the font library works the following discussions need to occur:
   1. What constitutes a first and second class object in WordPress
   2. First class object that may be introduced in the future

I shall provide a review of the existing code in this PR later today but that does not change the fact that the above needs to happen.

you ​asked for alternative reviews in the dev chat

@peterwilsoncc Right. Still expecting more reviews here. Also expecting some code to show what changes are "requested" :)

Avoid breaking backward compatibility...

There is no backwards compatibility breakage as there are no plugins using this function and filter. Please see for yourself if you don't believe me: ​https://wpdirectory.net/.

font_dir will no longer fire but that's fine if that's the developer's intent

No. This is not fine as it breaks the WP plugins API by completely disabling the font_dir filter. This was never intended and is a bug that needs to be fixed. Please consider that this would have broken the code as written by WordCamp.org: ​https://github.com/WordPress/wordcamp.org,
Pantheon: ​https://github.com/pantheon-systems/pantheon-mu-plugin, and VIP Go: ​https://github.com/Automattic/vip-go-mu-plugins/pull/5265 that was used as examples ​above.

Before making any changes to how the font library works

This PR does not make any changes to how the font library works. Please have a look at the code. This is a bugfix that fixes few bugs that were mostly introduced in https://core.trac.wordpress.org/changeset/57868. That's all.

the following discussions need to occur

If you want to discuss how the Font Library works please lets take this to a more appropriate place. Seems a post on ​https://make.wordpress.org/core/ would be such place.

There is no backwards compatibility breakage as there are no plugins using this function and filter. Please see for yourself if you don't believe me: ​https://wpdirectory.net/.

As has been pointed out earlier, not all plugins are in the plugin repository.

font_dir will no longer fire but that's fine if that's the developer's intent

No. This is not fine as it breaks the WP plugins API by completely disabling the font_dir filter. This was never intended and is a bug that needs to be fixed. Please consider that this would have broken the code as written by WordCamp.org: ​https://github.com/WordPress/wordcamp.org, Pantheon: ​https://github.com/pantheon-systems/pantheon-mu-plugin, and VIP Go: ​Automattic/vip-go-mu-plugins#5265 that was used as examples ​above.

Backward compatibility is about HOW code may be used.

Removing the filter is a different approach. The plugins you identify save fonts to upload/fonts, removing the filter saves them to uploads/yyyy/mm. Uploading still works, nothing is broken.

Running two plugins: one using font_dir and one removing the filter is unlikely as each take a different approach to managing the font directory.

Running two plugins using font_dir to change the directory would also result in one plugin winning out. That's how filters & the extensible nature of WordPress works.

the following discussions need to occur

If you want to discuss how the Font Library works it would be better to do this at a more appropriate place. Seems a post on ​https://make.wordpress.org/core/ would be such place. The fixes in this PR have nothing to do with such discussions.

This is what I am asking for.

When concerns were raised about this entire feature (by myself and many others) the response was that fonts were different. No links to such discussions were provided.

Please have this discussion before making changes.

Here's a review for bugs in the PR.

Thanks for the review. Replies are inline.

As has been pointed out earlier, not all plugins are in the plugin repository.

Perhaps, but that doesn't mean the plugins that are not hosted on WordPress.org should behave badly. I think it is much better to prevent any possibilities of "bad behavior" as soon as possible, before anybody starts doing it :)

The plugins you identify save fonts to upload/fonts, removing the filter saves them to uploads/yyyy/mm. Uploading still works, nothing is broken.

Not exactly. The "broken" part is that other plugins (and eventually core) cannot run properly as the font_dir filter is disabled. This is an unexpected, and very undesirable change to how the WP plugins API works that should be fixed.

Another reason for not storing fonts in the standard uploads sub-directories (year/month) is that it will probably interfere with future functionality. For example there were several requests for fonts to be "dropped in" the /fonts directory and automatically recognized and set up by WP, similarly to how plugins and themes work. This will be pretty much impossible to implement is fonts are stored in multiple sub-directories mixed up with all other kinds of files.

Running two plugins using font_dir to change the directory would also result in one plugin winning out.

Right, but both plugins will be able to run. If the font_dir filter is disabled one of the plugins will not be able to run at all. For example there may be more code that runs at the same time that filter is used, etc. Also the possibility for plugins to disable that filter means that core will never be able to rely on it. That would limit plans for future enhancements and development. That's not acceptable imho.

Oops didn't mean to close :(

Regarding the refactor and deprecation of _wp_filter_font_directory in this PR, which prevents plugins from bypassing the font_dir filter completely: I think this is a change we should move forward with. To me, the ability for one plugin to remove a filter (in a way that apply_filters is no longer called), while other plugins may be using that filter, is a problem we should address.

I can think of ways that multiple font_dir filters might be used together, maybe one sorts font files into subfolders by file format or font family, and one moves the overall folder location. I believe that decoupling the font_dir filter from upload_dir will give a better chance for these to work together as well as avoid collisions with other filters intended to only apply to upload_dir.

It's unfortunate that the development of the fonts API endpoints within Gutenberg led to placing an artificial limitation on how to set the font_dir folder (by using the upload_dir filter) and that we didn't look at other possibilities sooner--it's a lesson I will remember for future feature development and Gutenberg to wordpress-develop code backports. Still, I think the benefits of changing this now outweigh the risk of breakage.

Currently, a developer can choose to use date based upload directories in one of two ways:

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

/* OR */

add_filter( 'font_dir', 'wp_upload_dir' );

Either is legitimate and breaking backward compatibility will not prevent the latter from working.

Whether to decision to allow this was intentional or an unfortunate mistake is of no consequence to the effect: the feature exists and has been released.

Another reason for not storing fonts in the standard uploads sub-directories (year/month) is that it will probably interfere with future functionality. For example there were several requests for fonts to be "dropped in" the /fonts directory and automatically recognized and set up by WP, similarly to how plugins and themes work. This will be pretty much impossible to implement is fonts are stored in multiple sub-directories mixed up with all other kinds of files.

Sub-directories will still need to be accounted for, as Grant identifies, developers may filter the directory to store files in fonts/font-family/*.woff. With Google Fonts providing file names that are apparently quite random strings, this is quite helpful.

However, such a change would be a major architectural change as it would render the post types wp_font_family and wp_font_face redundant and require significant migration work. It's well outside the scope of this issue.

Currently, a developer can choose to use date based upload directories in one of two ways:

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

/* OR */

add_filter( 'font_dir', 'wp_upload_dir' );

Either is legitimate and breaking backward compatibility will not prevent the latter from working.

Hmm, no. The top one (remove_filter( 'upload_dir', ...) exploits a bug in WordPress that is caused by using an anti-pattern, namely doing apply_filters() in the callback to another filter. This is not a feature, and as such there is no backwards compatibility break for it. It is a bug that needs to be fixed.

...will not prevent the latter from working

Right. The intent of this fix is to ensure the integrity of the WP plugins API.

I can think of ways that multiple font_dir filters might be used together

Right. The last plugin that uses the filter will "win". This is how filters work in the WP plugins API and it is expected. It is not expected for a filter to be "missing" though. This makes the plugins API inconsistent and unreliable. This PR fixes that.

It is not expected for a filter to be "missing" though.

That's incorrect. Pretty much all the preflight filters, for example pre_wp_setup_nav_menu_item, cause filters to be bypassed. ​https://github.com/WordPress/wordpress-develop/blob/45a3d53f6641325a50c70783ca1e4636c7bcaf4a/src/wp-includes/nav-menu.php#L850

In this case removing the core filter bypasses the need for potentially expensive file operations that may not be needed. In some case, literally expensive as the use of a bucket offloader can result in charges to site owners.

That's incorrect. Pretty much all the preflight filters, for example pre_wp_setup_nav_menu_item, cause filters to be bypassed.

Right but these are designed to work that way and are (mostly) in functions that output HTML. So the purpose/design there is for a plugin to replace the core function and output the HTML.

The same is true for all functions in pluggable.php. Why do you think there are no new functions added to that file? Because it became apparent (very long tome ago) that this doesn't work well.

However this is a bad idea for functions that are part of an API as they would make that API inconsistent. There is nothing worse than an inconsistent API, right? The design for the font_dir filter is to always be available, just like the upload_dir filter. Can you imagine what would happen if a plugin disables the upload_dir filter? The sky will fall, right? :)

In this case removing the core filter bypasses the need for...

Again, to short-circuit a function is a specific code design decision. This is unsuitable when building an API. It was never intended for plugins to be able to remove the font_dir filter. The https://core.trac.wordpress.org/changeset/57868 that changed the behavior/introduced the bug actually says this:

These changes also ensure both the upload_dir and font_dir filter are applied consistently when both creating and deleting fonts faces.

How can a filter be applied consistently if a plugin removes it?

The design for the font_dir filter is to always be available, just like the upload_dir filter. Can you imagine what would happen if a plugin disables the upload_dir filter? The sky will fall, right? :)

This may have been the intention but it's not, in fact, how it was designed.

The sky doesn't fall if the font_dir filter is bypassed: the font files are uploaded, the URLs are referenced correctly and the directory structure matches the plugin users intent.

r57868 was about avoiding infinite loops and reduced the need to remove the filter. It's unrelated to this discussion.

How can a filter be applied consistently if a plugin removes it?

Prior to r57868 the upload_dir filter was only fired upon upload, it was not fired when getting the directory during post deletion.

If a plugin removes the filter, the filters still fire consistently between different operations. It just happens to be the case that the plugin author and site owner's intent is to upload files to a different location.

---

While architectural disagreements are one thing, I am finding the apparent disregard for the issue of creating unrelated yyyy/mm directories raised in ​https://github.com/WordPress/wordpress-develop/pull/6407/files#r1609047174 most frustrating.

Even if this or a similar change is made, the issue of unnecessary file system writes needs to be addressed.

This may have been the intention but it's not, in fact, how it was designed.

Sorry but not sure what are you trying to say here? You mean the intend was not implemented when designing this code? I don't think so. As we both mentioned several times above this was modeled after wp_upload_dir(). This fully implements the intent of how it should and would work.

The sky doesn't fall if the font_dir filter is bypassed

This is not what I meant. Let me explain again. I was giving an example of what would happen if other filters were removed by plugins, and how damaging that would be. Please try to imagine imagine what would happen if a plugin removes the upload_dir filter, not the font_dir filter.

r57868 was about avoiding infinite loops and reduced the need to remove the filter. It's unrelated to this discussion.

This is the commit that introduced this bug. And this PR fixes it. I don't see how it is unrelated?

If a plugin removes the filter...

If a plugin removes this filter it will break the normal operation of the WP plugins API. Do you really think this is a good way for plugins to work? I.e. hinder parts of an API and potentially break other plugins and maybe prevent future enhancements?

I actually think this is getting out of hand. It's been more than six weeks and there is no progress here at all. Again, I did review ​https://github.com/WordPress/wordpress-develop/pull/6211 and found the solution and the code unsatisfactory. Also there was a comment/review by @youknowriad that seems unsatisfactory too. However these reviews were ignored and ​https://github.com/WordPress/wordpress-develop/pull/6211 was committed just few days before the WP 6.5 release with the promise that this code will be fixed in 6.6.

So how are we fixing this code?

This is not what I meant. Let me explain again. I was giving an example of what would happen if other filters were removed by plugins, and how damaging that would be. Please try to imagine imagine what would happen if a plugin removes the upload_dir filter, not the font_dir filter.

I don't think bypassing the upload_dir filter is currently possible when calling the various uploading functions. It was designed differently to the font directory.

However, you raise a very good point. The changes in this PR allow for plugin authors to bypass the upload_dir filter and as a result bypass many of the protections in place when uploading files.

On this branch I was able to bypass the filter with the following code:

$uploaded_file = wp_handle_sideload(
	$file,
	array(
		'test_form'   => false,
		'upload_dir' => array(
			'path'    => ABSPATH,
			'url'     => site_url(),
			'subdir'  => '',
			'basedir' => ABSPATH,
			'baseurl' => site_url(),
			'error'   => false,
		),
	)
);

The changes in this PR allow for plugin authors to bypass the upload_dir filter

Yep, the changes here would allow plugins to reuse the wp_handle_upload() and wp_handle_sideload() and related functions, and set their own path and URL. Whether they would use wp-content/uploads and run the upload_dir filter is their choice. They can do that now too by using standard PHP functions.

However plugins will not be able to change anything, or bypass the upload_dir filter when the users are uploading through the media library, from block editor, etc. So I don't think this introduces any problems, it only opens some of the WP functionality to plugins.

Yep, the changes here would allow plugins to reuse the wp_handle_upload() and wp_handle_sideload() and related functions, and set their own path and URL. Whether they would use wp-content/uploads and run the upload_dir filter is their choice. They can implement uploading and store the files in any location now too by using standard PHP functions.

I think this change is far to broad and well beyond the scope of this ticket.

It represents a significant change to the uploads API that risks allowing plugins to bypass protections, have their data removed by deploys using rsync and make assumptions about the location of the uploads directory without consideration of changes to the wp-content location and/or bucket offloaders.

[peterwilsoncc]

As the WordPress 6.6 beta is approaching, I'll post my thoughts on the ​linked pull request:

The pull request significantly modifies the upload API to allow themes and plugins to bypass the default handling of WordPress uploads with the code that follows. Such code would allow themes and plugins to bypass bucket offloaders and make incorrect assumptions about the location of the upload directory.

Such changes are well beyond the scope of the changes this ticket aims to make.

<?php
$uploaded_file = wp_handle_sideload(
        $file,
        array(
                'test_form'   => false,
                'upload_dir' => array(
                        'path'    => ABSPATH,
                        'url'     => site_url(),
                        'subdir'  => '',
                        'basedir' => ABSPATH,
                        'baseurl' => site_url(),
                        'error'   => false,
                ),
        )
);

Effectively the font library introduces a context for uploads (the default being media library, the font library being fonts). The lightest touch changes for this ticket are to make use of a $context parameter/override in the various upload and media library related functions.

As this change is a refactor of existing code that is largely unchanged since it was introduced in ​GB#52704 and the code operates as intended (placing fonts in a separate directory), I've modified the ticket type to an enhancement.

I think this change is far to broad and well beyond the scope of this ticket.
..
It represents a significant change to the uploads API ...

I think you misunderstand this change. What is the difference between a plugin using the upload_dir filter or the new $upload_dir parameter?

None. There is no difference. There is no a "significant change" to anything, plugins have been able to do everything you mention for many years. The changes in this patch just makes things a little bit more convenient, and fix the bugs introduced in https://core.trac.wordpress.org/changeset/57868 which is a very inefficient fix for https://core.trac.wordpress.org/ticket/60652.

Frankly I don't understand why anybody would be sooo much against fixing this stuff, furthermore after the promise that it will be fixed in WP 6.6? This is a relatively simple patch that fixes several things, among them an anti-pattern that is a really bad example and should have never be added to WP in the first place.

[jorbin]

With no owner and no agreed upon patch, I'm punting to 6.7

[azaozz]

Replying to jorbin:

With no owner and no agreed upon patch, I'm punting to 6.7

Sorry for the delay, was away for few days (my son got married).

I am the "owner" of this ticket, have been since it was opened? :)

Also not sure what "no agreed upon patch" means here? Seems the patch works well after the two months long review by @peterwilsoncc?

From the ​review/discussion on the PR it became apparent that this actually fixes a pretty nasty bug introduced in [57868]. Because of that changing type to bugfix and returning to the 6.6 milestone.

Planning to commit it tomorrow.

[peterwilsoncc]

@azaozz Please do not commit this patch. As I have made very clear on the pull request, I have not approved the changes and have a big problem with the API changes being well beyond the scope of this ticket.

As mentioned on May 7, I think the following needs to occur before making changes to the font library uploads:

Before making any changes to how the font library works the following discussions need to occur:

1. What constitutes a first and second class object in WordPress
2. First class object that may be introduced in the future

In the same comment on May 7, I also mentioned that it may be worth considering adding a context to the various upload handlers. This feedback has also been ignored.

To be clear and unambiguous, to commit this ticket based on my review would be a significant misrepresentation of the outcome of that review.

Furthermore, I disagree with your comment that the discussion on the ticket has identified a bug introduced during the 6.5 cycle. Font uploads work, can be filtered by plugin authors and there have been no reports from end users to indicate otherwise.

[azaozz]

Replying to peterwilsoncc:

I have not approved the changes and have a big problem with the API changes being well beyond the scope of this ticket.

Yea, you've been firmly against this fix from the very beginning. I still don't understand why though, [57868] adds an anti-pattern to WP and these are always bad.

As mentioned on May 7, I think the following needs to occur before making changes

Sorry but this type of discussions seem out of bonds when fixing a bug? What difference does it make "What constitutes a first and second class object in WordPress" when fixing a bug?

To be clear and unambiguous, to commit this ticket based on my review would be a significant misrepresentation of the outcome of that review.

No worries, not going to commit this patch based on your review. There are other reviews on the PR.

Furthermore, I disagree with your comment that the discussion on the ticket has identified a bug introduced during the 6.5 cycle. Font uploads work...

As mentioned several times already the bug is mainly in adding a badly constructed code to WP that would likely prevent further development and/or enhancements there and is a bad example to follow. Furthermore fixing that code was agreed upon before committing [57868] to 6.5-RC3, right? So why all the attempts to not make that fix?

[azaozz]

Whether to decision to allow this was intentional or an unfortunate mistake is of no consequence...

Wait a minute. Are you suggesting that introducing _wp_filter_font_directory() in [57868] was also intended to allow plugins to bypass it by removing it from the upload_dir filter and prevent the font_dir filter from running? Looking at the patch again, and at the commit message, it seems that way!?

This was a refactoring of that part of the code, three days before the WP 6.5 release date that:

• Was not tested.
• Was not reviewed.
• Was not discussed.
• Doesn't even have a Trac ticket?

Even if all of the above points are ignored, this can only be considered an enhancement, not a bugfix. Such changes cannot be made during RC.

@swissspidy sorry for the ping but when reviewing [57868] for committing it to WP 6.5-RC3 were you aware it also contains that refactoring? This was committed from an unrelated ticket where the code was supposed to fix an infinite loop. Imho this is really improper. It is imperative for such (refactoring) changes to have Trac tickets where they can be tested and reviewed.

[peterwilsoncc]

[57868] was linked to #60652 and approved and tested on the pull request linked to the ticket. The change was to

• primarily, reduce the chance ongoing maintenance of the feature would not result in re-introducing the infinite loop bug
• secondly, to ensure that the hooks fired during deletion of a font matched the hooks fired during the uploading of a font

Allowing the font_dir filtering to be removed was introduced in ​gutenberg#52704 and unrelated to [57868].

To be honest, your unwillingness to listen to feedback on this ticket is, to me, illustrating why there where so many scathing comments about the introduction of this feature in the ​release retrospective.

Multiple contributors have indicated to me that they don't care to get involved in the feature as a result of this kind of attitude. This is why there are limited reviews of the PR linked to this ticket.

The changes in the linked pull introduce a significant API change to the uploading functions, well beyond the scope of a bug fix. In the last few comments, you've said there is a bug with font uploads but failed to identify it. What is the bug you are trying to fix?

[azaozz]

Replying to peterwilsoncc:

[57868] was linked to #60652 and approved and tested on the pull request...

Right, [57868] was committed as a fix for #60652. I.e. it was supposed to fix the infinite loop that may happen when calling a parent function from a filter callback. Nothing else!

However there is no mention ​on the PR that the code is intended to refactor how that part of the API works. This is understandable, such changes would need a Trac ticket and should not be done so close to release. Actually this seems to be an enhancement that should have been tested, reviewed and committed before beta-1?

Allowing the font_dir filtering to be removed was introduced in ​gutenberg#52704 and unrelated to [57868].

Sorry but that seems incorrect. The filter there uses a lambda function which makes it almost impossible to remove. (I am aware that there is a (hacky) workaround that could be used to remove filters with lambda callbacks. However it is inconsistent which makes it mostly unusable.)

To be honest, your unwillingness to listen to feedback on this ticket

I'm not really sure why would you say that after I've been listening to feedback for over two months? Both yours and others.

My mistake was that I assumed the API change that allows removal of the font_dir filter in [57868] was an unfortunate bug, but it seems it was intentional? Was it really?

There are also other intentional changes in that commit that are described in the commit message but are unrelated to the Trac ticket. As mentioned above these changes were not really reviewed and were never tested by users. In that terms they should have never be committed to WP few days before a release. My mistake comes from the fast that I could never imagine anybody would intentionally commit such code in such a way.

[peterwilsoncc]

The use of a lambda function was introduced in [57740] in an earlier commit for #60652, from memory this was during the late beta/early RC period. The change in the follow up commit reverted it back to the previous behaviour committed in the Gutenberg PR I've linked to a few times.

As I mentioned on the associated GB ticket, and linked to on the WP Trac ticket. Changing to a Lambda function worked around the problem rather than fix it. This introduced a maintenance burden for maintainers unaware of the requirement to go against the WordPress coding standards in order to avoid infinite loops.

The reason I've been asking for make posts is due to the comment on the PR for #60652 that it seems likely more directories such as the font directory are going to be introduced in the future and it should be discussed how to handle them.

Again, could you please specify exactly what the bug is that you are trying to fix by rewriting this code. As font files are being uploaded as intended, I think this needs to be clarified before considering rewriting the code anything but an enhancement.

[oglekler]

@azaozz it looks like we should move it to the 6.7.

[oglekler]

We reached RC1 commit freeze, and I am moving this ticket to the next milestone.

[azaozz]

Replying to oglekler:

@azaozz it looks like we should move it to the 6.7.

Sorry but I think this is a blocker for WP 6.6. Seems the current code was committed with only a partial review in [57868]. It was supposed to fix a possible infinite loop according to the Trac ticket #60652, but actually included a seemingly intentional change/refactoring of the WP Uploads API. That API change was never tested or reviewed.

As far as I understand code that has not been reviewed should not be committed to WP, ever. As this was already committed, the only option left is to be reviewed "after the fact". Imho this has to be resolved, one way or the other.

[joemcgill]

@azaozz I'm a bit concerned by your insistence that this PR is a blocker for 6.6. At this point, the current code has been in production since 6.5. Can you clarify what bug this fixes or feature this blocks that is critical to the 6.6 release?

While I'm not opposed to a refactor of the way font files are being handled, the changes you're proposing to wp_upload_bits() seem unwise to make at this point in the release cycle and would be better to land early in the 6.7 cycle.

[azaozz]

Again, could you please specify exactly what the bug is

Sure. I think you're right, there's been so many debates/comments/delays about this that it is easy to loose sight of the actual problems. Here's a tl;dr:

• [57868] was committed as a fix for #60652 that was supposed to fix the possibility of infinite loop. Instead of fixing the loop the code there only hides it from developers.
• The ​PR introducing the changes in [57868] was reviewed by five developers: @swissspidy, @mmaattiiaass, @grantmkin, @youknowriad, and myself. All reviewers found the code unsatisfactory, and thought there is a better way to do this. I even went ahead and tried to explain how this part of the code should work which was very different from the code in that PR. Please note: all reviews on the PR are about fixing the infinite loop as described in the Trac ticket. There are no reviews of that PR as an API change.
• At the end the PR was accepted as a fix for #60652 with reservations, simply because it was very near the WP 6.5 release date to come up with a better patch. As part of that acceptance there wass the promise to revisit this and fix it.
• After WP 6.5 was released I followed up on that promise and made the changes that fully fix the original #60652 bug (the infinite loop is prevented, not just hidden), and also the bug in the Uploads API that was introduced in [57868]: possibility of a part of the API to be disabled by a plugin.
• The ​current PR went through a very thorough review by two developers. It's been more than two months of questions and comments, and I believe it is a good solution for this part of the codebase.

To answer the question directly: The biggest bug here is that a part of the WP Uploads API can be disabled by a plugin. This may have been an intentional change that was committed without a review from an unrelated Trac ticket. However that improper commit doesn't make it less of a bug.

As a result from this API change a plugin can remove the font_dir filter and the wp-content/uploads/fonts directory completely (fonts will be saved mixed with all other files in the uploads directory). This hinders future development in WP and is unacceptable imho. For example, having a /fonts directory is needed to implement one of the most requested features: be able to drop fonts in the /fonts directory where WP will recognize them and make them available for use.

Another bug is that the solution to #60652 (possible infinite loop) in not-good-enough as found by the reviews there. This ticket was started as a fix for that.

Yet another bug here is that [57868] introduces an anti-pattern to WP, a filter is run in the callback to another filter. This causes several problems including a possibility for infinite loop and possibility for plugins to completely remove the "nested" filter. A plugin removing a filter that is a part of an API in WP is not acceptable imho. This is more of a "code architecture" bug but I think it has major drawbacks and such code should never exist in WP.

[azaozz]

Replying to joemcgill:

Thanks for your comment!

While I'm not opposed to a refactor of the way font files are being handled, the changes you're proposing to wp_upload_bits() seem unwise to make at this point in the release cycle and would be better to land early in the 6.7 cycle.

Could you please specify what changes exactly? I've been expecting a review for a month now, but better late than never, right? :)

[joemcgill]

@azaozz thank your for the summary, much appreciated.

The biggest bug here is that a part of the WP Uploads API can be disabled by a plugin.

Have any issues been reported that show that removing the font_dir filter is causing critical bugs in production? I'm not aware of any, and if not, this doesn't seem like a critical problem to me.

As a result from this API change a plugin can remove the font_dir filter and the wp-content/uploads/fonts directory completely (fonts will be saved mixed with all other files in the uploads directory). This hinders future development in WP and is unacceptable imho. For example, having a /fonts directory is needed to implement one of the most requested features: be able to drop fonts in the /fonts directory and WP recognizes them.

Again, this seems like an enhancement that is needed in order to support future functionality—not something required for 6.6, so it doesn't appear to be a blocking issue.

Could you please specify what changes exactly?

Sure! My understanding of the PR is that it modifies the uploads API by adding a new parameter to wp_upload_bits() so the directory where files will be uploaded is directly passed to the function rather than applied as a filter. This feels like a significant change to a low level (and foundational) API that should be made as part of an enhancement ticket, rather than as an implementation detail to a bug fix.

I think this would be better to go into trunk early so it has time to soak given additional changes to this API might be required to support additional use cases like the fonts directory.

[azaozz]

Replying to joemcgill:

Have any issues been reported that show that removing the font_dir filter is causing critical bugs in production?

I don't think so as there are no plugins that exhibit this "bad behavior" for now, as far as I see. However are you asking whether removing part of the Uploads API in WP should be considered a bug?

Again, this seems like an enhancement (preventing removal of the /fonts directory) that is needed in order to support future functionality—not something required for 6.6, so it doesn't appear to be a blocking issue.

Having a /fonts directory (whether it is wp-content/uploads/fonts or at any other place) is a base part of the WP Uploads API. Not having a /fonts directory is a major breakage of the design and of the intended behavior when storing fonts.

Of course, plugins can choose to do that. Plugins can also choose to remove the /themes and/or /plugins directories and store themes and plugins in /wp-content instead. That doesn't make it good or acceptable, and imho such plugins should be marked as exhibiting bad behavior.

Could you please specify what changes exactly?

Sure! My understanding of the PR is that it modifies the uploads API by adding a new parameter to wp_upload_bits() so the directory where files will be uploaded is directly passed to the function rather than applied as a filter. This feels like a significant change to a low level (and foundational) API that should be made as part of an enhancement ticket, rather than as an implementation detail to a bug fix.

Thanks! What is the difference (mostly from plugins point of view) between passing these settings as a param to the wp_upload_bits() function vs. using the upload_dir filter to pass them? Short answer: there is no difference.

Please note that plugins are able to use the new parameter on wp_upload_bits() and on _wp_handle_upload() only when using these functions directly. This is not possible when core is using these functions. In these cases there are no changes to the current behavior.

I think this would be better to go into trunk early so it has time to soak given additional changes...

Don't think there are any significant architectural changes in this patch. The only change is that it prevents plugins from making the WP Uploads API inconsistent. Also, this was once punted from #60652 as being too close to the WP 6.5 release. Now, despite all the delays, it is not so close to the WP 6.6 release expected in about three weeks. Do you think three weeks is not enough time for this to be reviewed?

[peterwilsoncc]

Yet another bug here is that [57868] introduces an anti-pattern to WP, a filter is run in the callback to another filter. This causes several problems including a possibility for infinite loop and possibility for plugins to completely remove the "nested" filter. A plugin removing a filter that is a part of an API in WP is not acceptable imho. This is more of a "code architecture" bug but I think it has major drawbacks and such code should never exist in WP.

As I've pointed out previously, this behaviour was introduced in the original pull request to Gutenberg and delivered in the ​original merge of the font API to WordPress-Develop.

The reason I reopened #60652 was because of the introduction of a lamda function in [57740]. This represented a maintenance burden to maintainers as it went against the coding standards, increasing the risk the infinite loop trap would be introduced.

Having a /fonts directory (whether it is wp-content/uploads/fonts or at any other place) is a base part of the WP Uploads API. Not having a /fonts directory is a major breakage of the design and of the intended behavior when storing fonts.

I don't necessarily disagree with this. As I mentioned in one of my earlier comments on the PR linked to this ticket, the introduction of a context to the various uploads APIs is something I am happy to work towards.

This would allow WordPress to prevent the bypassing of the font_dir filter.

Don't think there are any significant architectural changes in this patch. The only change is that it prevents plugins from making the WP Uploads API inconsistent.

My main issue with this ticket is that I don't think introducing an API that allows plugin authors to bypass the uploads_dir filter is a good solution to the issue that the original code allowed plugin authors to bypass the font_dir filter.

The use of uploads_dir to enable the use of offloading is a long term feature of WordPress that the changes in this ticket allows to be bypassed. This is the significant architectural change I am concerned about and it appears I am not alone in this.

[azaozz]

Replying to peterwilsoncc:

The reason I reopened #60652 was because of the introduction of a lamda function in [57740]. This represented a maintenance burden to maintainers as it went against the coding standards, increasing the risk the infinite loop trap would be introduced.

Yea, I agree a lambda callback to a filter is somewhat unusual in core. It is quite common in plugins. However I don't see how that is a "maintenance burden" especially after the decision to revisit this code early in WP 6.6. I also don't see how using a lambda function is "against the coding standards". It is undesirable as those filter callbacks cannot be removed (easily). However in this particular case the fact that this is non-removable prevents plugins from mangling the Uploads API.

Also changing from a lambda callback to a normal function callback doesn't change anything about an infinite loop possibility. Both are equally bad as both increase the risk that "the infinite loop trap would be introduced". As I've said several times already [57740] not only didn't fix that possibility of infinite loop, but also made it worse by hiding it from the developers.

I don't necessarily disagree with this. As I mentioned in one of my earlier comments on the PR linked to this ticket, the introduction of a context to the various uploads APIs is something I am happy to work towards.

This would allow WordPress to prevent the bypassing of the font_dir filter.

Great! So how come this patch couldn't be reviewed and committed for over three months now? Sounds bizarre, isn't it? :(

My main issue with this ticket is that I don't think introducing an API that allows plugin authors to bypass the uploads_dir filter is a good solution...

As I explained earlier this patch does not introduce a way for plugins to bypass the uploads_dir filter as used in core. Please have a look at the code again.

I'd like to repeat this again: The only way a plugin can use the new parameters added in this patch is by using either the _wp_handle_upload() or the wp_upload_bits() functions directly. A plugin cannot use these parameters when core is using these functions, hence a plugin cannot bypass any filters and cannot change anything else. The behavior is exactly the same as before the patch.

[azaozz]

As I mentioned above I'd consider this a blocker for WP 6.6. Seems that despite my best efforts there are still not enough contributors that would review the changes here. It's been three months. This is really troubling...

According to two of the three reviews here the code in this patch is "too much of a change" (again!) to be added during RC. In that case lets come up with a minimal patch that would fix the worst problem: allowing plugins to remove the /fonts directory and disable the font_dir filter.

A plugin can do this by removing the _wp_filter_font_directory() callback from the upload_dir filter. Something like this:

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

To fix that another plugin or core can do the opposite: i.e. re-add the callaback if it was removed. Something like this:

add_filter( 'upload_dir', function ( $upload_dir ) {
	if ( ! has_filter( 'upload_dir', '_wp_filter_font_directory' ) ) {
            add_filter( 'upload_dir', '_wp_filter_font_directory' );
        }
	return $upload_dir;
}, 9 );

This code is a normal use of the Hooks API in WP. A plugin that would remove a callback should expect that another plugin may re-add it.

However adding the above code to core would be quite a workaround. Core has access to better methods to stop the removal of a particular callback. Seems that can be done directly in remove_filter(), no need of workarounds. Patch coming up.

[azaozz]

In 60835.diff: Prevent removal of the '_wp_filter_font_directory' callback from the upload_dir filter.

This seems to be the most minimal change possible that would still fix the worst bug. The intention is to remove this in WP 6.7, deprecate this filter callback, and commit the patch from ​https://github.com/WordPress/wordpress-develop/pull/6407 (in its current form of after additional fixes/changes).

[peterwilsoncc]

My comment about preventing the removal of the font_dir filter was with regard to having done so during the introduction of the feature. That is, to have opened an enhancement at the time you approved the original PR adding a filter.

As I explained earlier this patch does not introduce a way for plugins to bypass the uploads_dir filter as used in core. Please have a look at the code again.

I am not concerned about bypassing the uploads_dir filter in Core. I am concerned about introducing an entirely knew API to allow themes and plugins to do so. Secondly, I am concerned about the introduction of such an API being a maintenance burden for contributors.

Yea, I agree a lambda callback to a filter is somewhat unusual in core. It is quite common in plugins. However I don't see how that is a "maintenance burden" especially after the decision to revisit this code early in WP 6.6. I also don't see how using a lambda function is "against the coding standards". It is undesirable as those filter callbacks cannot be removed (easily). However in this particular case the fact that this is non-removable prevents plugins from mangling the Uploads API.

Please refer to the coding standards on ​closures: "Closures should not be passed as filter or action callbacks, as removing these via remove_action() / remove_filter() is complex"

This is why it becomes a maintenance burden. Core contributors unaware that the decision to go against the coding standards was intentional in an attempt to avoid an infinite loop when using the filter as documented may reintroduce the bug.

---

I don't think that 60835.diff​ is appropriate code to introduce during the release candidate phase.

This ticket has had multiple committers say the proposed API is a problem, multiple contributors have moved it off the milestone and the release is now in a phase ​which neither enhancements or existing regressions from previous versions of WordPress can be committed.

I think the best thing you could do for now is move this ticket off the 6.6 milestone and spend some time reviewing feedback and reconsidering the approach. I've agreed to be co-tech lead on 6.7 and am willing to continue working towards a solution that doesn't introduce such a big change to the upload APIs.

[azaozz]

Replying to peterwilsoncc:

I am not concerned about bypassing the uploads_dir filter in Core. I am concerned about introducing an entirely knew API to allow themes and plugins to do so.

My understanding of the PR is that it modifies the uploads API by adding a new parameter to wp_upload_bits() so the directory where files will be uploaded is directly passed to the function rather than applied as a filter.

I see. So basically you are against themes and plugins being able to use wp_upload_bits() "directly" and having to do it "indirectly" by adding a callback to the upload_dir filter. Well, you're right that this sounds like an enhancement. Three months ago when this patch was created, this seemed like a good idea.

Not sure if this was your intention but I agree that wp_upload_bits() has nothing to do with how fonts are uploaded in WP. Going to remove that part and maybe open an "enhancement" ticket with that idea. Please feel free to comment about it there.

Please refer to the coding standards on ​closures: "Closures should not be passed as filter or action callbacks, as removing these via remove_action() / remove_filter() is complex"

This is why it becomes a maintenance burden. Core contributors unaware that the decision to go against the coding standards was intentional in an attempt to avoid an infinite loop when using the filter as documented may reintroduce the bug.

Ah, right, my bad. This made it in (was pushing for that change for years..). So basically the fix that prevents plugins from removing the /fonts directory that was committed in [57740] was reverted in [57868] to fix a coding standards issue. Do you think this was a good decision: reverting an important fix instead of adding a temporary coding standards exception?

Also, [57868] was committed to WP 6.5 as "temporary solution that needs fixing soon", right? I don't believe that the "core contributors would have been unaware" of it, especially with some more docs/inline comments :)

(Just for the record: I believe that the changes in [57868] were totally unnecessary and all that was needed was a coding standards exception and a bit more explanations that the code there will be updated soon to remove that exception, and how to avoid causing an infinite loop until the fix.)

So, again, the reason you are blocking the progress on the "promise to fix that code" as ​made on the PR for the [57868] commit by @youknowriad and @swissspidy are... What exactly?

Can you openly and honestly say why it is sooo important that this code remains unfixed, and what will WP gain by keeping it unfixed?

[azaozz]

Replying to peterwilsoncc:

I don't think that 60835.diff​ is appropriate code to introduce during the release candidate phase.

Why not? It does not change the way that code works at the moment. It actually doesn't change absolutely anything except to prevent plugins from removing the font_dir filter and the /fonts directory.

Do you think the changes in 60835.diff are more "inappropriate" than the changes in [57868]? Could you please review 60835.diff and if you find it "inappropriate" share here what exactly is inappropriate about it?

Btw, have you actually considered what would happen if a plugin removes the /fonts directory? This will alter the WP installation of the users of such plugin, most likely without them realizing it. The removal of /fonts would make it impossible for the users to use other plugins that expect that a /fonts directory exists. It will also make it impossible for the users to use future enhancements that require a /fonts directory, like for example being able to install fonts the same way as themes and plugins, and being able to manage installed fonts with git/svn.

Have you also considered what will happen to the users if they install such plugin after they have installed few fonts? Then the previously installed fonts will be in (now dysfunctional) /fonts directory, and the newly installed fonts will be dumped in the /year/month sub-directories in /uploads.

What about the opposite case: a user deactivates a plugin that removes the /fonts directory. Now they end up with previously installed fonts that are in few /year/month sub-directories and new fonts in a /fonts directory.

Generally plugin that remove the /fonts directory are by default incompatible with plugins that use it, and with future core enhancements.

Do you really believe this is a good way for WordPress to work? I believe this is a terrible way...

This ticket has had multiple committers say the proposed API is a problem

Ummm, this is NOT true :) One committer (you) has said everything possible to delay/block/prevent/derail/discourage fixing of this code. One committer, @joemcgill has asked a question that I just answered above, and one contributor, @grantmkin (@creativecoder on GH) who is one of the authors of the original code, has said this looks good.

I think the best thing you could do for now is move this ticket off the 6.6 milestone and spend some time reviewing feedback and reconsidering the approach. I've agreed to be co-tech lead on 6.7 and am willing to continue working towards a solution that doesn't introduce such a big change to the upload APIs.

Thanks for your advice! Just wondering, would you follow this advice if you were in my place?

I.e.

• Some no-good code was committed few days before the WP 6.5 release. It is no-good as all four reviews for it are negative, and was committed as "temporary patch that needs fixing".
• You attempt to fix it, but then the committer that wrote the no-good code and committed it starts to block any attempts for a fix with all means possible, and I mean all means!
• Eventually the patch is delayed soooo much as to make it impossible to commit to WP 6.6.
• You come up with an alternative patch that doesn't affect anything else just fixes the worse bug that was re-introduced in the 6.5 commit: plugins removing a WP directory.
• And finally the committer that wrote the no-good code, then committed it to 6.5 after 4 negative reviews, and then blocked all attempts to fix it gives you "advice" to just forget fixing it now, without even reviewing the latest patch?

Not sure about you but I think this is a really bad advice... Basically you're saying "do not attempt to make WP better!", isn't it?

[elrae]

I can't tell what the purpose of this PR is anymore, is it just to fix a potential filter issue and not actually change the core functionality of the fonts directory location? Or does this PR ALSO change the default directory of fonts from the uploads directory to move it outside of the uploads directory again by default?

Demonstrate issue with patch

Trac ticket: https://core.trac.wordpress.org/ticket/60835

[peterwilsoncc]

Can you openly and honestly say why it is sooo important that this code remains unfixed, and what will WP gain by keeping it unfixed?

I think the code can be improved (it's not buggy, in that font uploads work as expected), I believe I said as much on the original PR.

Pascal's comment on the original PR was that a make post was needed to discuss how such directories were to be handled in the future. This is to allow for a general approach rather than a font specific approach and why I asked for it in my first comment. (Pascal is on leave at the moment, thus no mention.)

Not sure about you but I think this is a really bad advice... Basically you're saying "do not attempt to make WP better!", isn't it?

This is why I have been reviewing the PR and requesting a different approach. I don't think your PR is an improvement as it introduces a new API that allows the upload_dir filter to be bypassed by plugin authors. This introduces the very issue to another filter you are trying to avoid.

I've marked this as a enhancement (it refactors code), Jorbin's moved it off the milestone, Joe has requested clarity and Olga (triage lead) has also moved it off the milestone.

I (and I presume others) are trying to work with you to improve the patch but hitting roadblocks again and again.

Do you think the changes in 60835.diff are more "inappropriate" than the changes in [57868]? Could you please review 60835.diff and if you find it "inappropriate" share here what exactly is inappropriate about it?

The patch introduces code to remove_filter() targeting a specific filter added and removed by core functions. At a glance it is clear this would cause problems for wp_font_dir(). At a fundamental level introducing such code to the plugins API is overreach regardless of the filter.

It's not really my responsibility to write tests for your patches but I have done so in ​PR#6995. The tests should probably be committed as part of test improvements for 6.7.

I can't tell what the purpose of this PR is anymore, is it just to fix a potential filter issue and not actually change the core functionality of the fonts directory location? Or does this PR ALSO change the default directory of fonts from the uploads directory to move it outside of the uploads directory again by default?

@elrae It does not move the font directory, that will continue to the live in uploads or uploads/site/siteid on multisite installs. The patch refactors the uploads code and introduces a new API.

[audrasjb]

As per today's pre-RC3 bug scrub, the decision was to move this ticket to milestone 6.7.
Given the discussion that's happening in this ticket (and also its tone), we shouldn't commit and of course backport anything in the current state of the ticket.
​https://wordpress.slack.com/archives/C02RQBWTW/p1720542500271149

[youknowriad]

Hey folks, there's obviously a lot going on here, a lot of back and forth, so I might be missing some context but I do recall that for the 6.5 release, we discussed that having a way to change the upload folder when calling the low level upload function was definitely needed but we didn't want to introduce that late in the cycle.

Looking at the PR shared by @azaozz it does address that concern and it's clear to me that the current code is too convoluted to understand even for someone that participated in it. I think the PR is a large improvement in terms of maintainability and I would support it. It's very unclear to me what the drawbacks of the change are reading the comments before, I'd appreciate a TLDR there if possible.

I wish it was committed earlier in the 6.6 cycle though as it seems that it was ready so I'd propose that we commit it early in the 6.7 cycle to assess for any impact.

[peterwilsoncc]

@youknowriad This is the tl;dr as I see it.

• one of, if not the main goal, of this ticket is to prevent plugin authors from removing the font directory filter from uploads_dir
• the linked PR does this by introducing/extending the uploads API in a manner that allows plugin authors to bypass the upload_dir filter
• I see this as problematic because the upload_dir filter is used by plugins to offload uploads to S3, Azure and other bucket services. This has been the case for many years.
• I also see this as an API that extends the problem being identified for font directory uploads
• the suggestion I made very early on during the review process was to consider introducing context to the uploads API to allow for directories such as uploads/fonts. This would improve maintainability without the issue of bypassing upload offloads.

During the 6.5 RC period I did look at adding context to the uploads API but didn't push the code as I felt it was too late to extend the APIs.

The ability to remove the font directory filtering was introduced in the original PR on the Gutenberg repo (the code was refactored but the ability remained), so making the change is a back-compat break but it may be acceptable. At worst it will leave files on the server once a font is deleted in the font library.

In the discussions during the 6.5 cycle it was mentioned that it was likely additional directories similar to fonts would be introduced. Pascal asked for a make/core post to expand on this. I can not assist with this as I don't know what is being proposed, the example of AI engines was given but only in the abstract.

I am on holidays until July 22 and would like to see a make post of what is being proposed so it can be discussed among all contributors and the architecture discussed independently of code. This will allow Kira and me (core tech leads) and Rob and Kai (editor tech leads) to determine the best approach to refactoring this code in association with other contributors.

[youknowriad]

Thanks @peterwilsoncc

I had a discussion with Peter and wanted to share my take aways at the moment.

• The PR doesn't not allow font uploads to bypass the uploads_dir filter.
• The PR doesn't not allow other regular uploads to bypass the uploads_dir filter.
• Plugin authors can use the new upload_dir argument to upload files to any location of their choosing without relying on the uploads_dir filter.
• Plugin authors can already do the same thing today, but they don't have a low-level provided API for it. They can either choose to use their own upload function, a composer package or also use the existing upload_dir to do so.

So to summarize, it seems the crux of the disagreement is whether we think that opening an API for plugins to upload files to a folder of their choosing knowing that they can already do it (in a a less straightforward way) is a big enough downside compared to the code quality improvement and maintainability offered by the change in the PR.

My opinion is still the same, I think the PR is worth it and the downside is not big enough to prevent it from landing. I can understand that folks may feel otherwise though.

[azaozz]

Replying to youknowriad:
Thanks @youknowriad and @peterwilsoncc for the review.

So to summarize, it seems the crux of the disagreement is whether we think that opening an API for plugins to upload files to a folder of their choosing knowing that they can already do it (in a less straightforward way) is a big enough downside compared to the code quality improvement and maintainability offered by the change in the PR.

My opinion is still the same, I think the PR is worth it and the downside is not big enough to prevent it from landing. I can understand that folks may feel otherwise though.

Just a note that letting plugins (and core) have more granular control when they are using wp_upload_bits(), etc. can also be seen as an improvement, not as a downside? As mentioned above this seems to make wp_upload_bits() a bit more useful to plugins, and has no backwards compatibility concerns.

An alternative solution I was looking at was to "split" wp_upload_bits() in two (similarly to _wp_handle_upload() and wp_handle_upload()):

• A new, private function that will have most of the existing code. It will only have the $upload_dir parameter and will not use wp_upload_dir().
• The existing function would become a wrapper, will call wp_upload_dir() and pass on the value to the new function.

Something like this (mostly pseudo-code):

function wp_upload_bits( $name, $deprecated, $bits, $time = null ) {
        if ( ! empty( $deprecated ) ) {
                _deprecated_argument( __FUNCTION__, '2.0.0' );
        }

        if ( empty( $name ) ) {
                return array( 'error' => __( 'Empty filename' ) );
        }

        $upload_dir = wp_upload_dir( $time );

        if ( false !== $upload_dir['error'] ) {
                return $upload_dir;
        }

        return _wp_handle_sideload( $bits, $name, $upload_dir );
}

function _wp_handle_sideload( $file, $name, $upload_dir ) {
        // Rest of the existing code from wp_upload_bits().
}

Then core will be able to use the new private function directly.

[peterwilsoncc]

it seems the crux of the disagreement is whether we think that opening an API for plugins to upload files to a folder of their choosing knowing that they can already do it (in a a less straightforward way).

This is an accurate comment.

The question I've been asking is different, though: is preventing plugins from bypassing the font_dir improved by easily allowing plugins to bypass the uploads_dir filter (and therefore offloading plugins)?

Making a decision based exclusively on the code required for the font library, I don't think it is. Knowing what will be required by future core features may change my opinion but I don't know what is being proposed.

Historically I have been the person to suggest my own commits be reverted or improved, I regret comments that I care more about having lines of code in core than improving WP but what is said is said.

Can I please get an answer to my question above before proceeding furhter.