WordPress.org

Make WordPress Core

Opened 8 years ago

Closed 5 years ago

#2682 closed defect (bug) (fixed)

Paths should be relative for images and uploads

Reported by: ryanscheuermann Owned by: ryanscheuermann
Milestone: 2.8 Priority: high
Severity: major Version: 2.1
Component: Upload Keywords: upload, migration, has-patch, needs-testing
Focuses: Cc:

Description

I don't know about anybody else, but this annoys the people who migrate WP installs from one server to another. The current code for uploading files stores the absolute path to the file in the database (postmeta) when it's possible to just store the relative path.

The way it is now, the code serializes an array into the database, and when migrating, you have to update the byte counts in the DB for the uploads to display on the new server. By using relative paths off the ABSPATH variable, this is unnecessary.

I've attached a patch, but it needs serious testing.

Attachments (4)

remove_upload_abspath.patch (4.6 KB) - added by ryanscheuermann 8 years ago.
references uploads with ABSPATH and stores relative path in db
2682a.patch (16.0 KB) - added by ryanscheuermann 8 years ago.
refactor attachments for new hooks & remove db store of ABSPATH
2682b.patch (15.8 KB) - added by ryanscheuermann 8 years ago.
refactor last patch: wp_delete_attachment uses get_attachment now (for filtering)
2682c.patch (15.9 KB) - added by ryanscheuermann 8 years ago.
don't concat ABSPATH if db value already contains it

Download all attachments as: .zip

Change History (35)

ryanscheuermann8 years ago

references uploads with ABSPATH and stores relative path in db

comment:1 leftjustified8 years ago

  • Keywords bg|has-patch bg|needs-testing added

If successful, this patch would also resolve #2631

comment:2 random8 years ago

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.

comment:3 ryanscheuermann8 years ago

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.

comment:4 westi8 years ago

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?

comment:5 random8 years ago

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

comment:6 ryanscheuermann8 years ago

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.

comment:7 random8 years ago

Single quotes around attributes are valid XHTML.

comment:8 ryanscheuermann8 years ago

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.

comment:9 westi8 years ago

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

comment:10 ryanscheuermann8 years ago

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?

comment:11 ryanscheuermann8 years ago

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.

comment:12 davidhouse8 years ago

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';
}
  1. The ABSPATH repition has got to go. Your idea about tossing around relative paths sounds good.
  2. More filters = more fun. This is one area where filters are lacking. It's "define the whole function or nothing".

comment:13 ryanscheuermann8 years ago

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.

ryanscheuermann8 years ago

refactor attachments for new hooks & remove db store of ABSPATH

comment:14 ryanscheuermann8 years ago

  • Owner changed from anonymous to ryanscheuermann

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

ryanscheuermann8 years ago

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

comment:15 ryanscheuermann8 years ago

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.

comment:16 ryanscheuermann8 years ago

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.

ryanscheuermann8 years ago

don't concat ABSPATH if db value already contains it

comment:17 Libertus8 years ago

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?

comment:18 ryanscheuermann8 years ago

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.

comment:19 Libertus8 years ago

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.

comment:20 matt7 years ago

  • Milestone changed from 2.1 to 2.2

comment:21 foolswisdom7 years ago

  • Keywords has-patch needs-testing added; bg|has-patch bg|needs-testing removed
  • Summary changed from Remove db store of absolute path for uploads to Paths should be relative for images and uploads

Summary in plain speak

comment:22 foolswisdom7 years ago

  • Milestone changed from 2.2 to 2.3

comment:23 ryan7 years ago

  • Milestone changed from 2.3 to 2.4 (next)

comment:24 markjaquith7 years ago

  • Priority changed from normal to high
  • Severity changed from normal to major

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

comment:25 lelion6 years ago

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

comment:26 nbachiyski6 years ago

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.

comment:27 lloydbudd6 years ago

  • Milestone changed from 2.5 to 2.6

comment:29 DD325 years ago

  • Component changed from Administration to Upload

This is semi-implemented in the current trunk, The images are stored as relative paths in the database, The URL's are correctly generated for files from old hosts, However, If a post includes reference to a URL which no longer exists, or has moved (since the domain name changed for example) then thats not dealt with.

I have a feeling this might finally be implemented in the great media overhaul of 2.9? If so, This ancient ticket could probably be closed?

comment:30 Denis-de-Bernardy5 years ago

agreed. should be closed as fixed.

comment:31 Denis-de-Bernardy5 years ago

  • Milestone changed from 2.9 to 2.8
  • Resolution set to fixed
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.