WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 3 years ago

#8753 closed enhancement (wontfix)

Add a context flag to the 'upload_dir' filter

Reported by: ulfben Owned by:
Milestone: Priority: low
Severity: normal Version: 2.7
Component: Upload Keywords: needs-patch close
Focuses: Cc:

Description

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?

Change History (12)

comment:1 DD325 years ago

  • Keywords dev-feedback needs-patch added; upload_dir custom upload dir filter media library empty runs removed

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

comment:2 ulfben5 years ago

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

comment:3 DD325 years ago

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)

comment:4 ulfben5 years ago

  • Priority changed from normal to low
  • Type changed from defect (bug) to enhancement

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

comment:5 DD325 years ago

  • Component changed from General to Upload
  • Milestone changed from 2.8 to 2.9
  • Owner anonymous deleted
  • Summary changed from Filter 'upload_dir' is called excessively for (seemingly) no reason to Add a context flag to the 'upload_dir' filter

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

comment:6 ulfben5 years ago

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

comment:7 DD325 years ago

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.

comment:8 ulfben5 years ago

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.

comment:9 Denis-de-Bernardy5 years ago

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.

comment:10 Denis-de-Bernardy5 years ago

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.

comment:11 Denis-de-Bernardy5 years ago

  • Keywords close added; dev-feedback removed
  • Milestone changed from 2.9 to Future Release

comment:12 scribu3 years ago

  • Milestone Future Release deleted
  • Resolution set to wontfix
  • Status changed from new to closed

No traction in almost two years. Closing.

Note: See TracTickets for help on using tickets.