Paths should be relative for images and uploads

references uploads with ABSPATH and stores relative path in db

If successful, this patch would also resolve #2631

IIRC it's currently possible to use whatever upload directory you want by plugging into the 'upload_dir' filter; will this patch mess with that? (A quick look makes me think it would.)

+1 for storing relative paths.

With the patch, the 'upload_dir' filter is simply passed an array value 'path' with the relative path instead of the absolute path. If you were plugging into that filter, you'd have to return a relative upload directory for the 'path' array key as the 'handle_upload' function now assumes this value is relative. 'url' is not disturbed as it's already relative off the siteurl.

I'm not sure this patch is taking the correct approach to fixing this issue.

We need all the filesystem operations to work with absolute paths so why strip off ABSPATH so early and then keep adding it back on?

Why not strip ABSPATH off at the end of wp_handle_upload to return a relative url then?

Oh, to clarify: I use that filter (rather than WP's built-in option) because my upload directory is nowhere near the WordPress directory. I'm not enthusiastic about passing '../../../../somedir/anotherdir/' or whatever as the path.

Also, with the patch applied, thumbnails are created but WP can't find them.

Maybe the array from wp_upload_dir() could store a 'relative_path' as well as 'path'?

OK, so instead of having wp_upload_dir return a relative path (and in turn the 'upload_dir' filter), we rework wp_handle_upload to strip the ABSPATH and return the relative path. Inline_uploading.php would then store the relative path in the DB, but would still have to add ABSPATH after the fact for thumbnail creation. Ya know, something about this whole process seems like a weak link. I'm going to think about this a little deeper later on. I think there are more issues here than we are seeing.

Also, I am going to patch the issue where inline_uploading stores invalid XHTML for the height/width properties (it uses single quotes around attributes).

And yes, I see where the thumbnail problem (WP can't find them) is occurring. I'll submit a new patch in a little while.

Single quotes around attributes are valid XHTML.

You know, you're right and I did not know that. I was taught to always use double quotes and I guess over the years I started to assume it was the standard. Thanks for teaching me something new.

And actually, the 'hwstring_small' meta attribute (that has the single quoted attributes) isn't even used by any WP core code. (inline_uploading.php: line 80) The height and width attributes for thumbnails are recreated upon display anyway.

I think this new hook added in [3754] might make this plugin territory now?

The new hook makes it possible for wp_handle_upload to return a relative path, but inline-uploading.php still does file operations (creating thumbnails) after wp_handle_upload is called. To create a "relativepath" plugin, one would have to rewrite inline-uploading.php as well (using the 'uploading_iframe_src' filter).

I'm thinking the thumbnail creation code should be moved out of inline-uploading.php and possibly even the wp_insert_attachment call? Maybe a "wp_attach_upload" function?

Forward-thinking this a bit, putting the code that creates the attachment object into a function w/ hook(s) would allow a plugin to "categorize" attachments and still follow the WP attachment db model.

Having the thumbnail creation code hookable would also allow the creation of alternate thumbnails and the ability to edit the thumbnail size.

The idea is to allow plugins to hook into the WP upload process and still have it degrade nicely with built-in WP uploading code.

Hi Ryan, three things:

1.

-	if ( $dir == ABSPATH ) { //the option was empty
-		$dir = ABSPATH . 'wp-content/uploads';
-	}

Why are you stripping that code? I would expect it to become:

if ( empty($dir) ) {
	$dir = 'wp-content/uploads';
}

2. The ABSPATH repition has got to go. Your idea about tossing around relative paths sounds good. 3. More filters = more fun. This is one area where filters are lacking. It's "define the whole function or nothing".

Hey David,

Thanks for the comments. As to 1, I stripped that code because I did the check a few lines above on the $path variable. But no worries, this was a quick patch, and I'm quite impressed and glad you guys are looking it over. This is the first open source project I've contributed to, and boy it's nice to have more than a few eyes on my code. You'll never get better if no one ever reviews your work, right?

I am working on a new patch that would strip the ABSPATH after wp_handle_upload (no repetition for file ops) - and create a new function to handle this process with more filters. Work comes first, I'm afraid.

refactor attachments for new hooks & remove db store of ABSPATH

I've created a new patch for this issue. I think it should handle most of the previous concerns. Here is what it does:

1. creates new function (wp_attach_upload) for attaching uploads with action hook 2. creates new function (get_attachment) for retrieving attachments with filter hook 3. deprecates get_udims for wp_shrink_dimensions 4. refactors inline-uploading.php to use new functions 5. removes the db storage of 'file' from serialized array in _wp_attachment_metadata (uses _wp_attached_file instead - allows easier migration if storing full path in DB (no byte counts)) 6. refactors other functions to use get_attachment (for filtering) 7. removes ABSPATH storage in wp_attach_upload, and adds it back in get_attachment

refactor last patch: wp_delete_attachment uses get_attachment now (for filtering)

Stupid Wiki Formatting, OK, here it is again:

1. creates new function (wp_attach_upload) for attaching uploads with action hook
2. creates new function (get_attachment) for retrieving attachments with filter hook
3. deprecates get_udims for wp_shrink_dimensions
4. refactors inline-uploading.php to use new functions
5. removes the db storage of 'file' from serialized array in _wp_attachment_metadata (uses _wp_attached_file instead - allows easier migration if storing full path in DB (no byte counts))
6. refactors other functions to use get_attachment (for filtering)
7. removes ABSPATH storage in wp_attach_upload, and adds it back in get_attachment

The new patch replaces the last one, and it changes wp_delete_attachment to use get_attachment (for filtering). Instead of adding the ABSPATH back in there, it allows get_attachment to do it.

Oh, 1 more thing, this patch doesn't address #2631 directly, but it's entirely possible to write a plugin to also remove the storage of siteurl from post->guid. But that would only fix attachments being added after the siteurl changes. It wouldn't fix all the uploads currently referenced in posts.

Another function would have to do a str_replace on the content for all the posts whenever siteurl is changed.

don't concat ABSPATH if db value already contains it

Ryan,

+if ( @file_exists($thumb) ) {
+    $attachdata['thumb'] = basename($thumb);
+}

This has the unintended side-effect of forcing thumbnails to be in the same directory as the image, even if a 'thumbnail_filename' filter says otherwise.

To fix, I modified wp_create_thumbnail() to return $thumb; instead of return $thumbpath; and your patch to read:

+if ( @file_exists(dirname($relfile).'/'.$thumb) ) {
+    $attachdata['thumb'] = $thumb;
+}

What do you think?

Libertus,

That code was taken from the original method in inline-uploading.php. Have you looked at mdawaffe's patch in #2794? I am recommending that my patch and his be integrated before addressing this issue. His patch also stores the basename of the thumb. With his new thumbnail filtering methods, allowing thumbnails in a different directory would be plugin territory.

That being said, I think wp_create_thumbnail should always return the full path of the file. That function could be used by plugins or elsewhere in the core, so it should return the fullpath to the created thumbnail. Especially for legacy support. The relative filtering can be done afterward.

Ryan,

Thanks for pointing me towards #2794. Having reviewed that ticket, I'm happy to wait for the overhaul of inline uploading. That looks like it will satisfy my requirements.

Summary in plain speak

I'm bumping the serverity on this one, as it can really screw up a domain migration.

Hello,

May I ask, if the discussion here relates to another problem ("Cannot define an upload directory one ore more levels up from WP blog directory"), and the fixes proposed may be useful to fixing that problem as well? :-)

Here's a link to the other ticket:

http://trac.wordpress.org/ticket/4427

Since we have shortcodes library, why don't we just put [attachment <id>] or if it isn't flexible enough (sometimes one wants to tune image attributes, etc.) at least use shortcode for its path/blog url.