WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#30346 closed defect (bug) (fixed)

Attachments not being found with attachment_url_to_postid()

Reported by: bradyvercher Owned by: SergeyBiryukov
Milestone: 4.1 Priority: normal
Severity: normal Version: 4.0
Component: Media Keywords: needs-unit-tests
Focuses: Cc:

Description

The second parameter for ltrim() is a character mask, not a string, which currently causes issues for domains with a "2" (such as IP addresses). For instance, this URL:

http://192.168.1.20/wp-content/uploads/2014/04/image.png

Is converted to 4/04/image.png, but it should be 2014/04/image.png.

The new WP_Customize_Upload_Control uses attachment_url_to_postid() to convert URLs to attachments for the Customizer and this bug prevents those controls from working properly.

I haven't looked into it, but this may also affect custom upload paths.

Attachments (3)

30346.patch (495 bytes) - added by bradyvercher 4 years ago.
Replace ltrim() with str_replace()
30346.2.patch (649 bytes) - added by bradyvercher 4 years ago.
30346.tests.patch (1.0 KB) - added by bradyvercher 4 years ago.

Download all attachments as: .zip

Change History (10)

#1 @bradyvercher
4 years ago

The easiest solution would replace ltrim() with str_replace(), but I'm not sure if there are limitations with that as well:

$path = str_replace( $dir['baseurl'] . '/', '', $url );

That also doesn't seem to account for all the scenarios covered by wp_get_attachment_url().

@bradyvercher
4 years ago

Replace ltrim() with str_replace()

This ticket was mentioned in Slack in #core by bradyvercher. View the logs.


4 years ago

#3 @dd32
4 years ago

  • Milestone changed from Awaiting Review to 4.1

Good catch, I'd probably replace this with a leading strpos/substr instead: (untested)

if ( 0 === strpos( $path, $dir['baseurl'] . '/' ) ) {
  $path = substr( $path, strlen( $dir['baseurl'] . '/' ) );
}

#4 @SergeyBiryukov
4 years ago

  • Keywords needs-unit-tests added

#5 @bradyvercher
4 years ago

Thanks for taking a look @dd32. I'm not able to look into the unit tests at the moment, but I added a patch using your approach (it worked on a couple of my local installs).

#6 @bradyvercher
4 years ago

I got a chance to look into adding the unit tests. They should fail before applying 30346.2.patch and pass afterward. Let me know if there's a better pattern for testing something like this.

#7 @SergeyBiryukov
4 years ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

In 30501:

Replace invalid use of ltrim() in attachment_url_to_postid() with substr().

props bradyvercher.
fixes #30346.

Note: See TracTickets for help on using tickets.