Add a context flag to the 'upload_dir' filter

[ulfben]

Since 2.7 I started getting a lot of bug reports from users of my plugin: ​Custom Upload Dir. It hooks into the 'upload_dir'-- filter and massages the paths according to the users preferences.

The problem is that the filter seems to be run excessively and from some very strange places:

Since 2.7 upload_dir is run whenever we visit the WordPress' Media Library. On my development install with ~1400 files and 160 pages in the Media Library, the filter is run about 160 times for viewing the first page...

It is run whenever we delete an attachment. It is run from wp_get_attachment_url and from wp_get_attachment_image_src - meaning that anyone using the ​Cleaner Wordpress Gallery-plugin will have upload_dir called four times per thumbnail displayed...

Naturally I have updated my plugin to quick bail and do nothing on these dry runs. But my question is - is the behaviour intentional?

[DD32]

indeed it is intentional.

the upload_dir() function needs to be called to find out the uploads directory, the uploads url, etc. In the case of the media library, each thumbnail and link needs to call it.. if theres not some form of caching in place on it already, there should probably be some..

[ulfben]

I don't want to come off as a complete tool, but it is confusing me. WP <2.7 (it seems) did not run this filter as frequently, and the various paths and URLs for uploads are stored in the database either way, right?

Isn't it completely redundant to run wp_upload_dir() when "reading"?

Thing is; there's no context to the filter call now. For the vast majority of runs in 2.7 my plugin can not return a useful path (no post is being worked with, so no dates, author name or other "path variables" available). I return the default one as a bailout, and WP seems to run on just fine still. So... is it really using these returns for anything? (remember - the default upload dir would be kind of useless on a system where everything is stored according to Custom Upload Dir-settings - so WP must be getting the correct paths from somewhere else).

I fully embrace the possibility that I'm just misusing the filter or doing it wrong™ - but I hope the issue is relevant to more than just my little plugin. :)

[DD32]

The URL and location of uploaded images are no longer stored in the database, well, not as absolute paths at least, This is designed to allow for WordPress installations to be moved between different hosts/domains/file locations easier.

The URL and Absolute url of images are dynamically created based on the 'basedir' and 'baseurl' results from upload_dir(), For example:

Stored in the database is an attachment, WP stores '2008/12/image-filename.jpg' as the filename, When it comes time to display it, WP generates the url by upload_dir() => URL = baseurl? . '2008/12/image-filename.jpg' => ​http://...../uploads/2008/... In the event that something needs to read the actual image, It can locate it by basedir? . 2008/12/image... etc.

I'm pretty sure this was done when it was changed to -not- rely upon 'GUID' for the url of images, GUID in the database will still be set to the current URL of the image, but it cant be relied upon in the future to remain the correct url if the WP install changes, or the upload dir changes, etc.

It looks like it probably wont affect your plugin, other than needing to short-circuit the filter, since its only using the basedir and baseurl pieces of the result.

Possibly a context value should be passed for future versions? ie. to tell the difference between an write and a read? (upload and a display)

[ulfben]

Thanks for explaining.

A context flag, or perhaps an entirely new filter? 'upload_to' or somesuch. Might be entirely redundant though - I haven't got the insight or overview to say for sure. What I do know is that for now I'm checking ($_POSTUpload? != "Submit Query") to avoid working on anything but uploads - and it's quick n dirty for sure...

I've got my answer and this was not a bug - what's the custom here - should I close the ticket? Should you or I post a request for such a context flag? (or can you see a better solution altogether?)

[DD32]

Just leave this open, It contains the background info that'd be needed for a new ticket anyway.

I'll set the milestone to 2.9 until theres a patch, I guess we need to work out where and what the contexts are :)

Just a FYI for your plugin, You might want to handle non-english setups by checking against the internationalized variant of the string, so ('Submit Query or whatever it is');

[ulfben]

I haven't checked where all the invocations of this filter happens, but perhaps we could just include the current post ID with the call?

All attachments in WordPress belongs to a post in some way, right? (if not, we could send null for those rogue items)

That would give me all the context I need anyway. :)

[DD32]

I haven't checked where all the invocations of this filter happens, but perhaps we could just include the current post ID with the call?

No. The whole problem in this ticket is that the uploads_dir filter is called in more locations than just when an attachment is being uploaded.

[ulfben]

Yes, I realize that. But whenever we're dealing with attachments, we're also (by inference) working with posts - since all attachments are associated to one. (I think)

Another solution perhaps, would be to have entirely separate methods: one to get the path of an existing attachment and another to create a path for a new attachment.

[Denis-de-Bernardy]

There aren't that many calls to wp_upload_dir() in WP. They'd only occur frequently on blogs that make heavy use of attachments.

[Denis-de-Bernardy]

ticket should probably be closed as invalid, too. @ulfben: I've a plugin that does similar things to yours, and what I do is, I cache the upload dir for individual posts as a postmeta.

[scribu]

No traction in almost two years. Closing.