Fix handling of uploading of font files

[azaozz]

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.

[github-actions]

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

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

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

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.

[azaozz]

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

[peterwilsoncc]

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:

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

[azaozz]

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.

[peterwilsoncc]

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?

[peterwilsoncc]

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

[azaozz]

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

[azaozz]

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.

[peterwilsoncc]

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.

[azaozz]

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.

[peterwilsoncc]

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.

[azaozz]

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.

[peterwilsoncc]

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.

[azaozz]

The plugins are using the filter in the intended fashions, as seen on WordCamp (since reverted following updates in Core), pantheon-systems/pantheon-mu-plugin#29 and Automattic/vip-go-mu-plugins#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 #6407 (review), 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 #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.

[creativecoder]

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

[creativecoder]

This is working in my testing

• ✅ Unfiltered font uploads place font files in wp-content/uploads/fonts
• ✅ Can use the font_dir filter to change the location where fonts are uploaded (such as wp-content/fonts)
• ✅ Can use the wp_get_upload_dir() function when filtering font_dir without an infinite loop

---

One unfortunate back compatibility issue I found is with font_dir filtering.

In trunk, a function like this works to change the font directory (notice the lack of changing basedir and baseurl). The path will be created if it does not exist.

function alter_wp_fonts_dir( $defaults ) {
	$defaults['path'] = WP_CONTENT_DIR . '/fonts';
	$defaults['url']  = WP_CONTENT_URL . '/fonts';
	return $defaults;
}
add_filter( 'font_dir', 'alter_wp_fonts_dir' );

Using that same filter with this PR, the font upload will fail if the directory doesn't already exist. I haven't tracked down why this works in trunk (seems like it shouldn't), but I can see the change causing breakage because of wp_font_dir newly depending on basedir to create the font directory when it doesn't exist.

[azaozz]

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.

[azaozz]

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

[azaozz]

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

[azaozz]

Here's a review for bugs in the PR.

Thanks for the review. Replies are inline.

[azaozz]

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.

[azaozz]

Oops didn't mean to close :(

[creativecoder]

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.

[peterwilsoncc]

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.

[azaozz]

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.

[azaozz]

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.

[peterwilsoncc]

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.

wordpress-develop/src/wp-includes/nav-menu.php

Line 850 in 45a3d53

$pre_menu_item = apply_filters( 'pre_wp_setup_nav_menu_item', null, $menu_item );

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.

[azaozz]

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?

[peterwilsoncc]

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.

[azaozz]

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 #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 #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?

[peterwilsoncc]

This function attempts to create the directory but it doesn't appear that _wp_handle_upload() does (correct me if I am wrong).

Given these changes allow developers to define the uploads dir, I think it should either:

• attempt to create a directory similar to the sideloading function
• add a new error message if the directory doesn't exist, it can use the same string as in the other functions.

If the second approach is taken then the override docs will need to include a note that the directory needs to be created before attempting an upload.

[azaozz]

Good catch! Yes, you're right, there is some repetition in wp_upload_bits() as the directory should be checked or created when calling wp_upload_dir(), and is later checked again and eventually created with wp_mkdir_p(). Thinking perhaps the use of wp_mkdir_p() here is mostly to catch edge cases when the upload_dir filter was used.

On the other hand _wp_handle_upload() relies only on the call to wp_upload_dir() to create the directory or ensure that it exists. That seems to be working well enough, been like that "forever" :)

Frankly I'm a bit unsure which fix would be better here.

1. As you mention we can add a (well documented) requirement that the directory as passed in $upload_dir param must exist. This would give the developers a bit more freedom in how to check, and how to create it if needed.
2. As far as I see the other option is to use wp_mkdir_p() to confirm or create the directory, same as in wp_upload_bits(). Think there was a slight difference with how is_dir() works with wrappers (perhaps that was only in older PHP versions) so using it to confirm might not be advisable as it might return a different result.

Which do you think is better?

[peterwilsoncc]

I'm a bit unsure which fix would be better here.

Same to be 100% honest, I could go either way too. If the upload handlers attempt to create the directory then it seems redundant that wp_upload|font_dir() does too, but this change would complicate that.

Let's sleep on it and hopefully our unconscious can decide for us overnight.

[azaozz]

Let's sleep on it and hopefully our unconscious can decide

Great idea! Unfortunately my subconscious did not take that opportunity :)

Joking aside, I tend to very slightly prefer letting plugins handle creation and confirmation of the uploads directory. That would give them a little more freedom in how to do it and whether and how to cache it. Of course they can just use wp_mkdir_p(). Thinking that checking the type and with is_dir(), and adding a longer description there would be sufficient.

Also tried to figure out if there are any differences between PHPs mkdir() and is_dir() when using wrappers. Think there may have been in the past but seems now there isn't (or at least I wasn't able to find anything).

[peterwilsoncc]

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

[azaozz]

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 implement uploading and store the files in any location 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.

[peterwilsoncc]

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.

[azaozz]

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.