WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 7 years ago

#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:
PR Number:

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 7 years ago.
22469_untrailingslash_UPLOADS.diff (433 bytes) - added by jbrinley 7 years ago.
patch to untrailingslash UPLOADS

Download all attachments as: .zip

Change History (11)

#1 @nacin
7 years ago

  • Milestone changed from Awaiting Review to 3.5

@nacin
7 years ago

#2 @nacin
7 years 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.

@jbrinley
7 years ago

patch to untrailingslash UPLOADS

#3 @jbrinley
7 years 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.

#4 @nacin
7 years ago

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

#5 @nacin
7 years 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.

#6 @ryan
7 years 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.

#7 @markjaquith
7 years 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.

#8 @markjaquith
7 years ago

  • Keywords commit added

jbrinley's patch looks like the correct solution.

#9 @markjaquith
7 years 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.