Opened 11 years ago
Closed 11 years ago
#27574 closed enhancement (fixed)
ID3 data should be editable
Reported by: | wonderboymusic | Owned by: | |
---|---|---|---|
Milestone: | 3.9 | Priority: | normal |
Severity: | normal | Version: | 3.6 |
Component: | Media | Keywords: | has-patch |
Focuses: | Cc: |
Description
For 2 reasons:
- It may not exist
- It may be wrong
When a user uploads an MP3, we slurp out the data, if it is present. If it isn't, you currently get nothing. On top of getting nothing, there is no way to add your own.
My patch adds a meta box to the Edit Media screen for Audio and Video. This is a pre-cursor to messing with the Attachment Settings in the modal for Audio and Video
Attachments (5)
Change History (22)
This ticket was mentioned in IRC in #wordpress-dev by wonderboymusic. View the logs.
11 years ago
#3
in reply to:
↑ 2
@
11 years ago
Replying to helen:
Is there anything we can map to existing post fields? Band and length fields also seem unnecessary by default. Also,
wp_get_relevant_id3_keys()
is screaming for a filter.
Agreed.
Further:
- Rather than
foreach ( array_flip( wp_get_relevant_id3_keys() ) as $key )
I'd doforeach ( wp_get_relevant_id3_keys() as $key => $label )
and just not use $label.
- id3data_meta_box() could use a better name, at the very least something like attachment_id3_data_meta_box().
- Since we have an opportunity to give a core meta box a non-horrible name (which they almost universally have), maybe we can change "id3datadiv" to something like "attachment-id3"
- Should we expect ID3 data to have HTML in it? Or block HTML? Or worse, unsafe HTML? Let's go with wp_kses_data() if we expect HTML, that avoids block-level and unsafe stuff. Note also that wp_filter_post_kses() works on slashed data (and returns slashed data), so the right function would have been wp_kses_post(). Yeah, they're all terrible names too.
#4
@
11 years ago
27574.2.diff refreshes against feedback. Map to existing fields? Audio files produce 6 fields, it's tough to decide what goes where. Obvs title to post_title, will keep thinking...
#5
@
11 years ago
27574.3.diff makes metadata way more sane for audio files. Videos do not need the same treatment. Here is how the fields break down:
- The Title of the attachment is always the title everywhere. When creating a playlist, the title is what is shown below each track, that syncs back to the server. We use that title in the track list, in the view for the currently playing track, everywhere. There is a title pulled out of ID3 data on upload, that is used to populate the title originally.
- The Caption of the attachment is used to override the value shown in the track list of a playlist. I have added text on the Edit Media page for Audio/Video to explain this.
- The Description is auto-populated on upload and is the text that is shown on the Attachment Page on the front-end for the attachment. I have added text on the Edit Media page for Audio/Video to explain this.
The new "Metadata" meta box shows the values for Artist, Album, Genre, Year, and Formatted Length. These values are editable. If your caption is empty, the value shown for each item in the playlist is: "{attachment.Title}" by {data.artist}. The artist can be toggled on and off in the Playlist Settings. The value for the currently playing item is:
"{attachment.Title}"
{data.album} if exists
{data.artist} if exists
Previously, none of the ID3 data was editable. Prior to 3.6, no attachment metadata was generated for audio and video files. This gives the user a chance to upgrade their data. New to 3.9, if you visit the Edit Media page for an audio or video file that doesn't have metadata, we will attempt to generate it for you.
#9
@
11 years ago
27574.4.diff drops formatted length, year, and genre from editable fields. Someone can always add them back (either for edit or for view) using the filter. We don't actually use or otherwise display this data anywhere, though. It's also really weird in the meta box as the rest of that information is about the file itself (bitrate, type, length, etc.) — none of it is about the content. We can add it back there though. [27862] had removed it.
#10
@
11 years ago
27574.5.diff stops using interpolation. I was able to get (via unfiltered HTML) things to start alerting at me through the caption field, and that's no good. We must presume all of these values are bad until we can prove they are not. I'd rather have > and < than the alternative.
#13
@
11 years ago
- Resolution set to fixed
- Status changed from new to closed
Marking this as fixed. We went several rounds.
#15
@
11 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
[27960] has security implications and must be studied. There's a reason I had it in my patch.
#16
@
11 years ago
We're going to need to revert [27960] and restoring escaping added in [27869].
This allows for XSS within the editor. Our rule is that even an admin with unfiltered HTML cannot cause admin-area XSS. This obviously is not a major vulnerability, but rather defense-in-depth.
We would have to go between {{ and {{{ based on is_admin(), I guess. I don't love that, either. It also means HTML will be represented as HTML rather than rendered (not a big deal). I don't know the solution for this. Let's have a new, targeted ticket and we can figure it out during 3.9 RC.
Is there anything we can map to existing post fields? Band and length fields also seem unnecessary by default. Also,
wp_get_relevant_id3_keys()
is screaming for a filter.