WordPress
Fix handling of uploading of font files
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
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.
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 azaozz, peterwilsoncc, grantmkin.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
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.
@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? If not enough, there might be another place the current WP code may need fixing/updating.
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 the code from the example 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 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 eventual 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_dirfilter (that is being removed by the example code) or theupload_dir, and why? Seems the importer(s) should be using thefont_dirfilter 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:
- What constitutes a first and second class object in WordPress
- 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 to the plugins and themes developers, 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. Imho a proper fix in that situation would have been to fix the docs, and add inline comments explaining why an infinite loop could happen or fix the core code to prevent such loops in the first place. Not change the code to interrupt the loop.
You seem intent on making the font library code more difficult to maintain...
Exactly the opposite. The current code is very hard 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' ) );inhandle_font_file_upload()was needed? The same array of allowed mime types is already passed towp_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 sense 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 can be improved and what changes you're actually requesting. 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 no 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:
- Avoid breaking backward compatibility for plugins using the
upload_dirfilter. Removing the filter is a legitimate way for developers who wish to use the standard upload destination for font files, eg2024/05. This PR breaks backward compatibility for plugin authors doing that (yes,font_dirwill no longer fire but that's fine if that's the developer's intent) - Before making any changes to how the font library works the following discussions need to occur: a. What constitutes a first and second class object in WordPress b. 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.
@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 PR implements a better fix for https://core.trac.wordpress.org/ticket/60652 and 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 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.
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_dirfilter. 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_dirto 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.
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.