Make WordPress Core

Opened 8 years ago

Last modified 5 years ago

#36931 new defect (bug)

wp_upload_dir caches calls with default arguments separately from their explicit equivalent

Reported by: stephenharris's profile stephenharris Owned by:
Milestone: Priority: normal
Severity: normal Version: 4.5
Component: Media Keywords: has-patch has-unit-tests
Focuses: Cc:


This a fairly minor bug. Prior to [36565] the following were equivalent

$upload = wp_upload_dir();
$upload2 = wp_upload_dir( gmstrftime('%Y/%m') );

(In deed, even $upload3 = wp_upload_dir( gmstrftime('%Y/%m yabadabadoo') ); is equivalent, but that behaviour is undocumented.)

Since caching has been introduced the first call to wp_upload_dir() is stored separately in the cache to the second call, despite the fact that they are otherwise interpreted the same.

This causes a disparity in behaviour when one of the caches is cleared after a setting has been changed:

$upload  = wp_upload_dir();
$upload2 = wp_upload_dir( gmstrftime('%Y/%m') );

// $upload == $upload2

//Disable year/month folders and clear the cache
add_filter( 'pre_option_uploads_use_yearmonth_folders', '__return_null' );
wp_upload_dir( null, false, true );

$upload  = wp_upload_dir( null );
$upload2 = wp_upload_dir( gmstrftime('%Y/%m') );

//$upload != $upload2

remove_filter( 'pre_option_uploads_use_yearmonth_folders', '__return_null' );

Attachments (1)

36931.diff (1.4 KB) - added by stephenharris 8 years ago.

Download all attachments as: .zip

Change History (4)

8 years ago

#1 @ocean90
8 years ago

  • Keywords has-patch has-unit-tests added
  • Version changed from 4.5.2 to 4.5

#2 @joemcgill
8 years ago

@stephenharris Thanks for the report.

I'd like to see @azaozz's feedback on this since he did most of the work on [36565].

Your explanation and patch both make sense, though I'm a bit uneasy about essentially forcing an undocumented default value for the $time parameter. Also, there is a slight difference in the way _wp_upload_dir() calculates the time string if it's set to null, using current_time( 'mysql' ). It would probably be best to remain consistent between these two functions.

#3 @stephenharris
8 years ago

@joemcgill Yes, it uses current_time( 'mysql' ) but then just extracts the year and month part. So you can pass it 2016/09 something else and it will extract 2016 and 09 as the year / month to form the subdirectory. So I don't think there should be an issue with forcing that as a default value.

Last edited 8 years ago by stephenharris (previous) (diff)
Note: See TracTickets for help on using tickets.