WordPress.org

Make WordPress Core

Opened 8 months ago

Closed 7 months ago

#25170 closed enhancement (fixed)

Add actual file size to attachment publish meta box

Reported by: desrosj Owned by: 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 8 months ago.
media.php.2.patch (733 bytes) - added by desrosj 8 months 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 8 months ago.
Updated patch using size_format().
25170.3.patch (708 bytes) - added by desrosj 8 months 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 7 months 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 7 months ago.
file_exists check before outputting file size data

Download all attachments as: .zip

Change History (26)

desrosj8 months ago

comment:1 desrosj8 months ago

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

desrosj8 months 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.

comment:2 follow-up: SergeyBiryukov8 months ago

  • Type changed from feature request to enhancement

Can we use size_format() here?

comment:3 in reply to: ↑ 2 desrosj8 months ago

Replying to SergeyBiryukov:

Can we use size_format() here?

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

desrosj8 months ago

Updated patch using size_format().

comment:4 desrosj8 months ago

  • Keywords has-patch added

comment:5 SergeyBiryukov8 months ago

  • Milestone changed from Awaiting Review to 3.7

comment:6 helen8 months ago

I don't mind this, and it is more of an average user thing than stuff like 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.

Last edited 8 months ago by helen (previous) (diff)

comment:7 follow-up: desrosj8 months 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.

desrosj8 months ago

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

comment:8 in reply to: ↑ 7 ; follow-up: helen8 months 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.

comment:9 in reply to: ↑ 8 desrosj8 months 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.

comment:10 follow-up: helen8 months 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.

comment:11 in reply to: ↑ 10 desrosj7 months 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.

desrosj7 months ago

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

comment:12 helen7 months 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.

comment:13 helen7 months 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.

comment:14 follow-up: DrewAPicture7 months 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.

comment:15 in reply to: ↑ 14 ; follow-up: nacin7 months 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.

comment:16 DrewAPicture7 months 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.

comment:17 in reply to: ↑ 15 DrewAPicture7 months 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.

DrewAPicture7 months ago

file_exists check before outputting file size data

comment:18 DrewAPicture7 months ago

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

comment:19 DrewAPicture7 months ago

  • Keywords commit added

comment:20 helen7 months 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.