Make WordPress Core

Opened 12 years ago

Closed 12 years ago

Last modified 12 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's profile mikeschinkel Owned by: nacin's profile 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 12 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)

@mikeschinkel
12 years ago

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

#1 @mikeschinkel
12 years ago

  • Cc mikeschinkel@… added

#2 @naomicbush
12 years ago

  • Cc naomicbush added

#3 @gandham
12 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 );

#4 follow-up: @gandham
12 years ago

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

#5 in reply to: ↑ 4 @mikeschinkel
12 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.

#6 @braydonf
12 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.

#7 @nacin
12 years ago

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

#8 @nacin
12 years ago

  • Keywords needs-unit-tests added

#9 @nacin
12 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.

#10 @nacin
12 years ago

Still could use unit tests :-)

Note: See TracTickets for help on using tickets.