WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#18855 closed defect (bug) (fixed)

get_attached_file( $post_id ) wrongly(?) returns the uploads directory when $post_id is not a valid attachment

Reported by: mikeschinkel Owned by: nacin
Milestone: 3.4 Priority: normal
Severity: normal Version: 3.2.1
Component: Post Thumbnails Keywords: has-patch needs-unit-tests
Focuses: Cc:

Description

The function get_attached_file( $post_id ) wrongly(?) returns the uploads directory when $post_id is not a valid attachment. For example, if I run the following code:

$attached_file = get_attached_file(0);

It might return the following on my machine:

/Users/mikeschinkel/Sites/mysite/public_html/wp-content/uploads/

I would instead expect it to simply return false indicating that no attached file was found:

false

Here is the code I wrote that caused me to recognize the problem:

function has_valid_file_attached( $attachment_id ) {
  return file_exists( get_attached_file( $attachment_id, true ) ) );
}

Here's the hack I had to write instead:

function has_valid_file_attached( $attachment_id ) {
  $attached_file = get_attached_file( $attachment_id, true );
  if ( '/' == substr( $attached_file, -1, 1 ) )
    return false;

  if ( ! file_exists( $attached_file ) )
    return false;

  return true;
}

I have attached a patch that simply returns false when the value returned by the internal call to get_post_meta( $attachment_id, '_wp_attached_file', true ) is an empty string. I would be surprised to find it many people (anyone?) used this function for it's side-effect to return the root upload path so I doubt using this path would break anything.

It's not a critical patch because there is an easy workaround, but it cost me extra time to figure out why my has_valid_file_attached() function wasn't reporting false when the attachment was missing, so it would be nice to keep someone else from having to waste the same time in the future.

Attachments (1)

get_attached_file-returns-false.diff (1.1 KB) - added by mikeschinkel 4 years ago.
Fixes get_attached_file() so that it returns false when the attachment_id is not valid.

Download all attachments as: .zip

Change History (11)

@mikeschinkel4 years ago

Fixes get_attached_file() so that it returns false when the attachment_id is not valid.

comment:1 @mikeschinkel4 years ago

  • Cc mikeschinkel@… added

comment:2 @naomicbush4 years ago

  • Cc naomicbush added

comment:3 @gandham4 years ago

This function uses get_post_meta and some images meta data does not include _wp_attached_file , in this case also get_attached_file is returning the incorrect path.

	$file = get_post_meta( $attachment_id, '_wp_attached_file', true );

comment:4 follow-up: @gandham4 years ago

Why is the get_attached_file using get_post_meta when we have wp_get_attachment_metadata($id) ??

comment:5 in reply to: ↑ 4 @mikeschinkel4 years ago

Replying to gandham:

Why is the get_attached_file using get_post_meta when we have wp_get_attachment_metadata($id) ??

wp_get_attachment_metadata() uses get_post_meta() too.

comment:6 @braydonf4 years ago

How can we get this moving forward? I've just run into this problem, and was surprised that get_attached_file did not return false.

comment:7 @nacin4 years ago

  • Keywords dev-feedback removed
  • Milestone changed from Awaiting Review to 3.4
  • Owner set to nacin
  • Status changed from new to accepted

comment:8 @nacin3 years ago

  • Keywords needs-unit-tests added

comment:9 @nacin3 years ago

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

In [20613]:

Return false in get_attached_file() when the file does not exist, rather than a path to the base uploads directory. props mikeschinkel. fixes #18855.

comment:10 @nacin3 years ago

Still could use unit tests :-)

Note: See TracTickets for help on using tickets.