WordPress.org

Make WordPress Core

Opened 10 years ago

Closed 5 months ago

#16849 closed enhancement (fixed)

Add a filter for $overrides in wp_handle_upload()

Reported by: iandunn Owned by: johnbillion
Milestone: 5.7 Priority: normal
Severity: normal Version: 3.1
Component: Upload Keywords: has-patch
Focuses: Cc:

Description

I'm writing a plugin that needs to set a $unique_filename_callback in wp_handle_upload() during a normal post attachment upload (not a custom upload form), but there's no way to do it without a filter like this one:

$overrides = apply_filters( 'wp_handle_upload_overrides', $overrides );

For now I just modded my /wp-admin/includes/file.php to add it, but obviously that's not an ideal solution, especially since I plan on adding the plugin to the repository for others to use.

Attachments (4)

16849.patch (513 bytes) - added by jakub.tyrcha 10 years ago.
16849.2.patch (843 bytes) - added by iandunn 8 years ago.
16489.3.patch (804 bytes) - added by Mte90 4 years ago.
refreshed patch
16849.diff (4.5 KB) - added by johnbillion 5 months ago.

Download all attachments as: .zip

Change History (33)

#1 @mikeschinkel
10 years ago

  • Cc mikeschinkel@… added

#2 @jakub.tyrcha
10 years ago

  • Keywords has-patch needs-testing added

Sounds reasonable, doesn't it?

#3 @iandunn
10 years ago

Hey Jakub, thanks for writing the patch. I'm not sure it's correct, though. I think the filter should actually go before if ( is_array( $overrides ) ), shouldn't it?

We want to let a plugin modify/overwrite $overrides before $overrides gets used. The way the patch is written now, the filter could only be used if $overrides was already passed in, which won't always be the case.

Also, you forgot to wrap the if block in brackets, so the extract() statement would get called even if the condition was false.

I think below $file = apply_filters( 'wp_handle_upload_prefilter', $file ); would be a good place for it. It doesn't need to be wrapped in an if statement or anything, so it would just be

$file = apply_filters( 'wp_handle_upload_prefilter', $file );
$overrides = apply_filters( 'wp_handle_upload_overrides', $overrides );

#4 follow-up: @jakub.tyrcha
10 years ago

Ah, I'm deeply sorry - it must be the pre-gsoc rush. As for the place in code - I thought it was needed only when there are some existing overrides - I moved it to the suggested place now

@jakub.tyrcha
10 years ago

#5 in reply to: ↑ 4 @iandunn
10 years ago

Looks great, thanks :)

#6 @DrewAPicture
9 years ago

  • Keywords dev-feedback added

#7 @jcow
9 years ago

This would be a really useful addition for something so simple. I almost always prefer the renaming of files as it stands in Wordpress, but now and then I need the ability to control a filename, and this would come in handy. I hope this is being considered for addition. Thanks.

#8 @mordauk
9 years ago

I see this was originally slated for 3.1, any chance we can get this pushed into 3.5?

#9 @nacin
9 years ago

  • Keywords commit added; needs-testing dev-feedback removed
  • Milestone changed from Awaiting Review to 3.5

We should also add an overrides filter to wp_handle_sideload(). But since it doesn't have a prefilter filter, I'm fine with just this for now.

#10 @iandunn
9 years ago

Awesome, thanks nacin :)

#11 @nacin
9 years ago

  • Keywords 3.6-early added

#12 @nacin
9 years ago

  • Milestone changed from 3.5 to Future Release

#13 @martey
8 years ago

  • Cc martey added

@iandunn
8 years ago

#14 @iandunn
8 years ago

I refreshed the patch and added filters for $file and $overrides in wp_handle_sideload().

Would this be a good candidate for 3.7? It seems like a decent number of people think it'd be useful, and it's required for the Overwrite Uploads plugin to work without having to mod Core.

#15 @SergeyBiryukov
8 years ago

  • Milestone changed from Future Release to 3.7

#16 @nacin
8 years ago

We really, really need to change how this works. Deliberately allowing people to specify variables so we can EXTR_OVERWRITE them is a recipe for disaster — or at least, in this case, inflexibility. This function really scares me because it can encourage bad or insecure code. That, and it doesn't follow a design pattern we use almost everywhere else.

Is this something we can convert to a more standard $defaults and wp_parse_args() situation? Then we can do an apply_filters() on that, followed by extract() with EXTR_SKIP.

#17 @wonderboymusic
8 years ago

  • Keywords needs-docs added; commit removed

There's a new filter

#18 @jeremyfelt
8 years ago

  • Keywords 3.6-early removed
  • Milestone changed from 3.7 to Future Release

Punting to future release for now. Seems like something that should have a plan early in the cycle. See concerns in Nacin's last comment.

#19 @nacin
7 years ago

  • Component changed from Plugins to Upload
  • Keywords needs-patch added; has-patch needs-docs removed

#21 @wonderboymusic
7 years ago

_wp_handle_upload() is now a mashup of the two, the overrides are gross, but it is physically impossible to change the value of them without a filter because media_handle_upload() never passes that argument.

#22 @chriscct7
5 years ago

  • Keywords needs-refresh has-patch added; needs-patch removed
  • Severity changed from minor to normal

@Mte90
4 years ago

refreshed patch

#23 @Mte90
4 years ago

  • Keywords dev-feedback added; needs-refresh removed

I refreshed the patch with the latest version.

This ticket was mentioned in Slack in #core by mte90. View the logs.


3 years ago

#25 @iandunn
3 years ago

r36310 (#19121) added the wp_unique_filename filter, which satisfies some of the use cases for filtering $overrides.

#26 @CrazyJaco
7 months ago

Bumping this issue.
I work for a regional publisher.
Our uploads directory for a given month is quite large. Large enough that wp_unique_filename is a bottleneck. I want to short circuit the scandir() in the function.

I'd like to add a unique_filename_callback, but there is no way to override that.
Using WP-CLI's wp media import , it invokes media_handle_sideload(). There is no way to override or pass in the $overrides['unique_filename_callback'] in wp_unique_filename() using these higher level functions.

I believe the patch above (16489.3.patch) would allow for that, though.

#27 @johnbillion
5 months ago

  • Milestone changed from Future Release to 5.7
  • Owner set to johnbillion
  • Status changed from new to reviewing

I'd just like to point out that @iandunn opened this ticket ten years ago and eight years later he was still writing patches for it. Is this a phenomenon specific to the WordPress project? Who knows. Thanks for sticking with it.

I've run into this problem several times, let's see if we can get it in.

@johnbillion
5 months ago

#28 @johnbillion
5 months ago

  • Keywords dev-feedback removed

In 16849.diff I've done the following:

  • Added the $file variable as a parameter to make contextual filtering of the overrides easier
  • Documented the possible elements of the $overrides array
  • Improved some other docs

#29 @johnbillion
5 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 49845:

Upload: Introduce the {$action}_overrides filter that allows the overrides parameter for file uploads and file sideloads to be filtered.

The dynamic portion of the hook name, $action, refers to the post action.

Props iandunn, jakub.tyrcha, nacin, wonderboymusic, Mte90, johnbillion

Fixes #16849

Note: See TracTickets for help on using tickets.