Make WordPress Core

Opened 14 years ago

Last modified 5 years ago

#15149 new enhancement

image_get_intermediate_size partly ignores 'path' and 'url' fields and has inconsistent return values

Reported by: whoismanu's profile whoismanu Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.1
Component: Media Keywords: has-patch
Focuses: Cc:

Description

the current implementations of image_get_intermediate_size and image_downsize in wp-includes/media.php have several inconsistencies.

image_get_intermediate_size() returns the following information about an attachment for registered sizes: file, width, height, path, url. for ad-hoc sizes (if the size argument is an array) it returns: file, width, height.

in the second case, path and url information that may have been set by a plugin is lost.

further, the calling function, image_downsize(), in any case regenerates the url like this

$img_url = str_replace(basename($img_url), $intermediate['file'], $img_url);

this means that now also for the registered sizes, the 'url' information is discarded. further, this means that for registered sizes, the url is computed twice, once inside image_downsize as detailed above, once in image_get_intermediate_size() like this

$data['url'] = path_join( dirname($file_url), $data['file'] );

to me, a cleaner approach would be to return the full information (file, width, height, path, url) independent of whether an image size is registered or not. all these fields can then also be directly calculated inside image_get_intermediate_size() and the caller does not have to bother with, e.g., generating paths and urls. further, this would allow to accept path and url information that is already present in the db, also in the case of ad-hoc image sizes.

i put together a patch that provides a potential solution.


Attachments (1)

image-intermediate-size-url-patch.diff (3.4 KB) - added by whoismanu 14 years ago.

Download all attachments as: .zip

Change History (11)

#1 @Denis-de-Bernardy
14 years ago

  • Cc Denis-de-Bernardy added

Doesn't the suggested patch break BC with plugins that play around with uploads folder?

#2 @whoismanu
14 years ago

unless i overlooked something, the patch shouldn't break anything. here is my reasoning:

before the patch, the url is computed in image_downsize() like this:

$img_url = str_replace(basename($img_url), $intermediate['file'], $img_url);

where

$img_url = wp_get_attachment_url($id);

and $intermediate[''file''] is the file field returned by image_get_intermediate_size(). it follows that the result is equivalent to dirname($img_url).$intermediate[''file''].

with the patch, the url is calculated inside image_get_intermediate_size(). there we have that $file_url is equivalent to $img_url above and $data[''file''] corresponds to $intermediate[''file'']. the url is computed as

$data['url'] = path_join( dirname($file_url), $data['file'] );

looking at path_join(), this results in

a) dirname($file_url).$data[''file''] if $data[''file''] is not absolute, which is thus the same as dirname($img_url).$intermediate[''file''] without the patch

b) $data[''file''] if $data[''file''] is absolute. here the result is thus not the same as without the patch. however, without the patch the result here must be wrong anyway since it corresponds to a concatenation of a path with an absolute path ( dirname($img_url).$intermediate[''file''] and $intermediate[''file''] here is absolute) which doesn't make sense.

so up to now the patch returns either exactly the same as what was returned before, or it returns something that makes sense where something that was obviously wrong was returned before.

there is one more case, where the patch returns something different from what was returned before. this is if the ''path'' or ''url'' fields are already set in the metadata. with the patch, the url corresponds to the one from the metadata. without the patch, the metadata is ignored. so the only way backwards compatibility could be broken is if a plugin requires that the 'path' and ''url'' fields in the metadata must be ignored. now if this really is the case, then ''path'' and ''url'' should be removed altogether from image_get_intermediate_size() because right now they are never used, as i detailed in my initial posting above.

#3 @whoismanu
14 years ago

the more i think about this, the more i am convinced that in the end there is a more fundamental underlying question that should be answered:

does one want to give plugin authors the freedom to choose the location of intermediate image sizes or not?

right now this is not the case (mainly because of what i detailed in my posts above) and the file/directory structure is the following:

uploaddir/yyyy/mm/image.jpg
uploaddir/yyyy/mm/image-size1.jpg
uploaddir/yyyy/mm/image-size2.jpg

where image.jpg is the original image, the other two are resized versions of it. finally, size1 and size2 usually have the form {width}x{height}, but they could also be set to something more readable like "thumbnail" via filters. in the attachment metadata "sizes" field only "width", "height" and "file" are stored and "file" is the filename of the resized image. the path to the resized image is constructed as detailed above, by taking the path of the original image and exchanging the filename with the one found in "file".

when dealing with images, i think, however, that other structures can make a lot of sense and it would therefore be good to have some flexibility in this respect. in particular i am thinking about a structure like this:

uploaddir/original/yyyy/mm/image.jpg
uploaddir/thumbnail/yyyy/mm/image.jpg
uploaddir/medium/yyyy/mm/image.jpg

why is this in some cases a better suited structure? well, it allows you to immediately find all files that belong to a certain size. for example, it makes it very easy to delete all medium images. just delete the corresponding directory. or what about renaming a size? just rename the corresponding directory. with the structure wordpress imposes right now, i have to traverse all of the year-month directories to find the corresponding files and treat them one by one, based on their filename.

so, for some plugins it certainly makes sense to have a different directory structure. therefore i think wordpress should offer this flexibility. further, this could be quite easily achieved by storing the "path" and "url" information in addition to the "file", "width" and "height" information inside the metadata. it looks like someone already thought about this, otherwise why would there be references to a "path" and an "url" field inside image_get_intermediate_size().

whatever the decision is on this, i think that in any case the intention needs to be made clearer. if sizes are to be distinguished only based on their filename, references to fields like "path" or "url" that suggest the opposite should go away. if paths are to be flexible, the above patch is probably not enough, but it would be better to make sure that all sizes have the corresponding fields and that the core as well as plugins rely on these fields to find the location of a size, rather than computing it from the "file" field.

i am looking forward to hear your thoughts on this.

#4 follow-up: @Denis-de-Bernardy
14 years ago

Well, the reason I asked is I wrote a plugin (Uploads Folder) which tinkers around with these pieces of data and moves files around in the uploads folder to reflect the URL of whichever piece of data they're attached to. And no later than yesterday, I was writing a different one which organizes files as so:

  • wp-content/images/fullsize
  • wp-content/images/midsize
  • wp-content/images/thumbnails
  • wp-content/videos
  • etc., while keeping the existing date or URL-based organization, if any.

Both plugins cater to the need of access-restricting some files but not others.

Both are a bit hacky, in the sense that I had to make do with the lacking of the API, but they work.

To be frank, the key points that bothered me had more to do with missing filters than with anything. In particular:

  • the thumbnail URL filter isn't always applied (image downsize needs to be used in some cases to work around this)
  • filters are missing to properly change where the file gets stored without things interfering with file deletion when attachments are deleted.
  • the send to editor filters are inconsistently applied throughout the application.
  • hard-coded links in posts aren't automatically changed when attachment file urls change, leading me to think that some URL rewriting would be welcome to abstract this stuff in posts.

Getting back to your patch, it seems to me that we should also (or at least) split _wp_attached_file and the resulting meta in two, so the subfolder can be filtered based on the file being displayed. And the same for what is returned by wp_uploads_dir() -- the latter currently lacks context. This would also beak backwards compatibility, but in a way that makes things, I suspect, more flexible when we mess around with uploaded files.

#5 in reply to: ↑ 4 @whoismanu
14 years ago

Replying to Denis-de-Bernardy:

denis,

thanks for your answer. i will try to address all of the points you raise.

Well, the reason I asked is I wrote a plugin (Uploads Folder) which tinkers around with these pieces of data and moves files around in the uploads folder to reflect the URL of whichever piece of data they're attached to. And no later than yesterday, I was writing a different one which organizes files as so:

  • wp-content/images/fullsize
  • wp-content/images/midsize
  • wp-content/images/thumbnails
  • wp-content/videos
  • etc., while keeping the existing date or URL-based organization, if any.

Both plugins cater to the need of access-restricting some files but not others.

bc of plugins like uploads folder. i had a look at your plugin and it doesn't use the "path" and "url" fields. all it does with respect to attachment metadata is to fetch the filename from the "file" field of the "sizes" metadata. as i wrote in my second post this should be no problem, it is not affected by the patch.

(btw, i guess you have a bug in your plugin in the get_path() function where it says:

$path = get_option('upload_path');
$path = trim($upload_path);

this is somewhat unrelated to this discussion, but i figured it might be helpful for you to know :-))


Both are a bit hacky, in the sense that I had to make do with the lacking of the API, but they work.

sure, you can make it work. i also didn't want to imply that it is not possible to organize attachments in a structure different from the standard one. in fact i am doing exactly this in the currently available beta version of my photoq plugin. but like you say, it is only possible through "hacks". it thus works for my plugin, however, it stops working when interacting with other plugins that deal with attachments. the reason is that throughout wordpress it is assumed that the resized images are files with a different suffix in the same directory. while i can make my plugin respect the "path" and "url" fields that i populated, other plugins do not care about them because wordpress itself does currently not properly support them.

To be frank, the key points that bothered me had more to do with missing filters than with anything. In particular:

  • the thumbnail URL filter isn't always applied (image downsize needs to be used in some cases to work around this)
  • filters are missing to properly change where the file gets stored without things interfering with file deletion when attachments are deleted.
  • the send to editor filters are inconsistently applied throughout the application.
  • hard-coded links in posts aren't automatically changed when attachment file urls change, leading me to think that some URL rewriting would be welcome to abstract this stuff in posts.

so there are more problems in this respect than i thought. however, i am not sure that one can work around my problem easily. i think it is really more a fundamental problem than the issue of adding a few filters. to me the question really is, whether a resized image is a (renamed) file in the same directory as the original, or whether there is flexibility as to where it can be located. you can also nicely see this in the image_make_intermediate_size() function where it says

$resized_file = apply_filters('image_make_intermediate_size', $resized_file);
return array(
	'file' => basename( $resized_file ),
	'width' => $info[0],
	'height' => $info[1],
);

although there is a filter, its effect is partly reversed by the hardcoded call to basename and the fact that other fields like 'path' or 'url' that could hold the directory information are missing (i.e. it implies that a resized image is represented through the tuple 'file', 'width', 'height' only, where a better name for 'file' whould actually be 'filename').

Getting back to your patch, it seems to me that we should also (or at least) split _wp_attached_file and the resulting meta in two, so the subfolder can be filtered based on the file being displayed. And the same for what is returned by wp_uploads_dir() -- the latter currently lacks context. This would also beak backwards compatibility, but in a way that makes things, I suspect, more flexible when we mess around with uploaded files.

not sure i am able to follow your thoughts here. _wp_attached_file and _wp_attachment_metadata are already separate custom fields. also, i am less concerned about the uploads dir, which i can easily set/change through the upload_dir filter, than about the fact that currently wordpress seems to be missing a standardized way to retrieve the location of resized images.

#6 follow-up: @Denis-de-Bernardy
14 years ago

_wp_attached_file is what gets used for the actual file management part internally. Playing with the upload dir is, to a large extent, desirable when spreading files around if you want images in wp-content/images rather than wp-content/uploads/images.

I think you misunderstood what I was meaning. Currently, the base image file corresponds to _wp_attached_file, and we toggle the file name. What I suggested in my above comment was to split _wp_attached_file into _wp_attached_file and _wp_attached_dir, and to have the main image map that, and to have the smaller sized images map that as well. And all of it filtered:

$thumbnail_image_url = apply_filters('whatever', $filtered_base_url . $filtered_dir . $file_name);

And similar code for the path, so as to allow plugins to change where it gets stored if relevant.

It's like, if we break backwards compatible, might as well do it completely but in a way that allows full flexibility.

#7 @Denis-de-Bernardy
14 years ago

Just for reference, since I hadn't released the second plugin I mentioned. You can download it here:

http://forum.semiologic.com/discussions/?PostBackAction=Download&AttachmentID=545

#8 in reply to: ↑ 6 @whoismanu
14 years ago

Replying to Denis-de-Bernardy:

first of all i am sorry for my late reply. i am in the middle of relocating and changing jobs, so not that much time at hand these days...

_wp_attached_file is what gets used for the actual file management part internally. Playing with the upload dir is, to a large extent, desirable when spreading files around if you want images in wp-content/images rather than wp-content/uploads/images.

I think you misunderstood what I was meaning.

Possible, but i think we are slowly getting there :-)

Currently, the base image file corresponds to _wp_attached_file, and we toggle the file name. What I suggested in my above comment was to split _wp_attached_file into _wp_attached_file and _wp_attached_dir, and to have the main image map that, and to have the smaller sized images map that as well. And all of it filtered:

$thumbnail_image_url = apply_filters('whatever', $filtered_base_url . $filtered_dir . $file_name);


sounds good and definitely more flexible.

And similar code for the path, so as to allow plugins to change where it gets stored if relevant.

It's like, if we break backwards compatible, might as well do it completely but in a way that allows full flexibility.

sure, that sounds perfectly reasonable to me. i guess that a lot of the image_ functions (image_downsize(), image_get_intermediate_size(), image_make_intermediate_size()...) inside wp-includes/media.php will be affected by such a change.

#9 @markoheijnen
12 years ago

Any change that you can look at the changes that are made in 3.5 for WP_Image_Editor. We do unset the path in multi_resize since it was causing bugs and wasn't there before.

Note: See TracTickets for help on using tickets.