Make WordPress Core

Opened 11 years ago

Closed 11 years ago

#25170 closed enhancement (fixed)

Add actual file size to attachment publish meta box

Reported by: desrosj's profile desrosj Owned by: helen's profile helen
Milestone: 3.7 Priority: normal
Severity: normal Version:
Component: Media Keywords: has-patch commit
Focuses: Cc:

Description

When editing an attachment, you are presented with a lot of useful information.

File URL, file name, and file type at the most basic level. Images add dimensions, and video & audio attachments add length, year, genre, etc., if that information exists in the file's _wp_attachment_metadata post meta entry.

The most important piece of information about a file is missing though, the actual file size.

I have created a plugin that solves this at http://wordpress.org/plugins/display-file-sizes/.

Attachments (6)

media.php.patch (792 bytes) - added by desrosj 11 years ago.
media.php.2.patch (733 bytes) - added by desrosj 11 years ago.
Alternative approach. Uses the filesize index of _wp_attachment_metadata if set (audio & video) instead of always calculating it. Falls back if file is an image.
25170.2.patch (704 bytes) - added by desrosj 11 years ago.
Updated patch using size_format().
25170.3.patch (708 bytes) - added by desrosj 11 years ago.
Adjustment based on helen's feedback. Only display this field if a file size is in fact retrieved.
25170.4.patch (2.3 KB) - added by desrosj 11 years ago.
Only show file type if an extension is found. Remove audio codec, mime type from default fields.
25170.diff (626 bytes) - added by DrewAPicture 11 years ago.
file_exists check before outputting file size data

Download all attachments as: .zip

Change History (26)

@desrosj
11 years ago

#1 @desrosj
11 years ago

This patch adds a "File size" line below "File type".

@desrosj
11 years ago

Alternative approach. Uses the filesize index of _wp_attachment_metadata if set (audio & video) instead of always calculating it. Falls back if file is an image.

#2 follow-up: @SergeyBiryukov
11 years ago

  • Type changed from feature request to enhancement

Can we use size_format() here?

#3 in reply to: ↑ 2 @desrosj
11 years ago

Replying to SergeyBiryukov:

Can we use size_format() here?

Sure can, didn't find that initially. Let me update the patch now.

@desrosj
11 years ago

Updated patch using size_format().

#4 @desrosj
11 years ago

  • Keywords has-patch added

#5 @SergeyBiryukov
11 years ago

  • Milestone changed from Awaiting Review to 3.7

#6 @helen
11 years ago

I don't mind this, and it is more of an average user thing than mime-type (which we do show), but I'm not super sure we *need* it, either. What makes this the most important piece of data?

Patch-wise, since filesize() could possibly return false, I might suggest getting the file size and making sure it's something worth showing before any output.

Version 0, edited 11 years ago by helen (next)

#7 follow-up: @desrosj
11 years ago

I find it useful because there is nowhere in the admin to see an attachment's file size. You would have to use FTP, inspect it with developer tools, or actually save the file locally and look. So end users that do not have this knowledge or access are completely shielded from file sizes, even if they understand what they mean.

It could also be very useful for sites on a multisite install or hosting plan that have reached their upload quota. They can use this to identify and delete larger, unused files to make room without needing to have access to, and muck around with FTP.

@desrosj
11 years ago

Adjustment based on helen's feedback. Only display this field if a file size is in fact retrieved.

#8 in reply to: ↑ 7 ; follow-up: @helen
11 years ago

  • Keywords commit added

Replying to desrosj:

It could also be very useful for sites on a multisite install or hosting plan that have reached their upload quota. They can use this to identify and delete larger, unused files to make room without needing to have access to, and muck around with FTP.

I think that's an interesting example. This brings up something else I was wondering about (and have no intention of displaying anywhere by default) - that generated image sizes also take up space.

Reasoning is good enough for me - looking at the filters patch on #25171 right now, then will tend to this.

#9 in reply to: ↑ 8 @desrosj
11 years ago

Replying to helen:

Replying to desrosj:

It could also be very useful for sites on a multisite install or hosting plan that have reached their upload quota. They can use this to identify and delete larger, unused files to make room without needing to have access to, and muck around with FTP.

I think that's an interesting example. This brings up something else I was wondering about (and have no intention of displaying anywhere by default) - that generated image sizes also take up space.

Reasoning is good enough for me - looking at the filters patch on #25171 right now, then will tend to this.

I had thought of that as well actually. My original idea for this was to make the field expandable, mimicking the post status, visibility, and post date fields.

A majority of times, you don't display the image in its full size on the front end anyways.

When I originally thought of this, I was setting up a slider. It was not loading very fast, and I wanted to check the image sizes to see how large they were. Because I was showing a custom image size on the front end, the overall image size would have only given me a hint as to whether the files were too large, but it would be more useful to have the file size of the actual image size I was displaying.

#10 follow-up: @helen
11 years ago

I'm down with showing original file size, maybe removing a couple other sections by default (file type, mime type). Don't think we should do anything about intermediate image sizes here - that sounds like a plugin or in an improved image editor thing.

#11 in reply to: ↑ 10 @desrosj
11 years ago

Replying to helen:

I'm down with showing original file size, maybe removing a couple other sections by default (file type, mime type). Don't think we should do anything about intermediate image sizes here - that sounds like a plugin or in an improved image editor thing.

I agree with mime type, but I feel that file type could still be useful to many users. Is a file a PDF or a DOC? Is an image a PNG or JPG?

If I were to suggest other fields that would should be considered for removal, I would suggest audio codec. A value such as ISO/IEC 14496-3 AAC is probably not useful to a majority of people.

Also noticed while digging through this that for audio & video files, mime type could potentially be displayed twice. The current file type field outputs the file extension, or the mime type if that fails. Patching it so file type will only be shown if an extension is found in case someone decides to re-include mime type with the filters from #25171.

@desrosj
11 years ago

Only show file type if an extension is found. Remove audio codec, mime type from default fields.

#12 @helen
11 years ago

Going to leave the codec/mime-type stuff for now, I guess people who go into that screen for audio and video are probably power users, anyway.

#13 @helen
11 years ago

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

In 25403:

Add file size to attachment publish meta box. props desrosj. fixes #25170.

#14 follow-up: @DrewAPicture
11 years ago

  • Keywords commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Looks like filesize() tosses a warning in this context if the file doesn't actually exist in a media item that does exist.

Warning: filesize(): stat failed for /srv/www/mutrunk/src/wp-content/uploads/2013/07/image.jpg in /srv/www/mutrunk/src/wp-admin/includes/media.php on line 2457

We could either
1) Silence the errors on filesize() with an @, though this really just hides the larger problem
2) Add a file_exists() check in get_attached_file() to return false if the file doesn't exist.

#15 in reply to: ↑ 14 ; follow-up: @nacin
11 years ago

Replying to DrewAPicture:

Looks like filesize() tosses a warning in this context if the file doesn't actually exist in a media item that does exist.

When does this occur?

And, yes, a file_exists() would be fine. I *don't* think it makes sense inside get_attached_file(), though, for replication reasons. get_attached_file() returns the path to where the file should be, but I'm not sure that's a "promise".

Either way, we need to make sure get_attached_file() returns a valid value before calling filesize() on it.

#16 @DrewAPicture
11 years ago

The case for comment:14 was that I have a media item where the source file doesn't actually exist anymore. The file meta is stored with the media item and get_attached_file() still returns the path to the nonexistent file, which in-turn causes filesize() to toss a warning.

#17 in reply to: ↑ 15 @DrewAPicture
11 years ago

Replying to nacin:

<snip>
And, yes, a file_exists() would be fine. I *don't* think it makes sense inside get_attached_file(), though, for replication reasons. get_attached_file() returns the path to where the file should be, but I'm not sure that's a "promise".

Either way, we need to make sure get_attached_file() returns a valid value before calling filesize() on it.

I guess we'll need to change the return doc for get_attached_file() then as well:

* @return string|bool The file path to the attached file, or false if the attachment does not exist.

@DrewAPicture
11 years ago

file_exists check before outputting file size data

#18 @DrewAPicture
11 years ago

25170.diff adds a file_exists check before outputting file size data.

#19 @DrewAPicture
11 years ago

  • Keywords commit added

#20 @helen
11 years ago

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

In 25608:

Make sure the attachment file exists before calling filesize() on it. props DrewAPicture. fixes #25170.

Note: See TracTickets for help on using tickets.