WordPress.org

Make WordPress Core

#22469 closed defect (bug) (fixed)

Remove trailing slash from UPLOADS constant

Reported by: jbrinley Owned by: ryan
Milestone: 3.5 Priority: normal
Severity: critical Version: 3.5
Component: Media Keywords: has-patch commit
Focuses: Cc:

Description

When building the uploads URL for multisite, wp_upload_dir() does this:

$url = str_replace( UPLOADS, 'files', $url );

Problem is, UPLOADS has a trailing slash; $url does not. That means that all image URLs look like http://example.com/wp-content/blogs.dir/123/files/YYYY/MM/image-file.png

I'm not sure if UPLOADS should be set without the trailing slash, or if wp_upload_dir() should untrailingslashit() before doing the str_replace()

Attachments (2)

uploads-wpcom.diff (407 bytes) - added by nacin 17 months ago.
22469_untrailingslash_UPLOADS.diff (433 bytes) - added by jbrinley 17 months ago.
patch to untrailingslash UPLOADS

Download all attachments as: .zip

Change History (11)

comment:1 nacin17 months ago

  • Milestone changed from Awaiting Review to 3.5

nacin17 months ago

comment:2 nacin17 months ago

ryan mentioned they needed some extra code on WordPress.com to make this work. Maybe they were working around the same bug. Not sure. I've uploaded the patch they're using here.

jbrinley17 months ago

patch to untrailingslash UPLOADS

comment:3 jbrinley17 months ago

  • Keywords has-patch added

Looks like it works pretty well to just apply untrailingslashit() when doing the str_replace(). I can't find any other places in core where UPLOADS is used that the trailing slash might matter. Patch attached.

comment:4 nacin17 months ago

I'm trying to figure out what is different from 3.4 to 3.5. I don't see it.

comment:5 nacin17 months ago

  • Owner set to ryan
  • Status changed from new to assigned

ryan, any thoughts here? Our ms-files.php + switched changes made the changes in logic rather difficult to follow.

comment:6 ryan17 months ago

dotcom uses that block of code from uploads-wpcom.diff​ to override all of the juggling wp_upload_dir() does, juggling that ends in the wrong result. It wasn't just slashes that were wrong, IIRC.

dotcom doesn't put trailing slashes on either UPLOADS or BLOGUPLOADDIR.

22469_untrailingslash_UPLOADS.diff​ seems safe although I too am failing to see what would cause a slashing change vs. 3.4.

comment:7 markjaquith17 months ago

I've confirmed the issue.

In 3.4.2, both have trailing slashes.

In trunk, $url does not have a trailing slash.

Looking into an appropriate solution.

comment:8 markjaquith17 months ago

  • Keywords commit added

jbrinley's patch looks like the correct solution.

comment:9 markjaquith17 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 22736:

Remove trailing slashes from UPLOADS before trying a str_replace() in wp_upload_dir(). props jbrinley. fixes #22469

  • In 3.4.x, both $url and UPLOADS had trailing slashes
  • Due to refactoring, $url is no longer expected to have a trailing slash
  • Because of the mismatch, the str_replace() was not working, resulting in an incorrectly verbose upload dir URL
Note: See TracTickets for help on using tickets.