Make WordPress Core

Opened 11 years ago

Closed 11 years ago

#27574 closed enhancement (fixed)

ID3 data should be editable

Reported by: wonderboymusic's profile wonderboymusic Owned by:
Milestone: 3.9 Priority: normal
Severity: normal Version: 3.6
Component: Media Keywords: has-patch
Focuses: Cc:

Description

For 2 reasons:

  1. It may not exist
  2. 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)

27574.diff (4.2 KB) - added by wonderboymusic 11 years ago.
27574.2.diff (4.4 KB) - added by wonderboymusic 11 years ago.
27574.3.diff (8.1 KB) - added by wonderboymusic 11 years ago.
27574.4.diff (9.3 KB) - added by nacin 11 years ago.
27574.5.diff (15.7 KB) - added by nacin 11 years ago.

Download all attachments as: .zip

Change History (22)

This ticket was mentioned in IRC in #wordpress-dev by wonderboymusic. View the logs.


11 years ago

#2 follow-up: @helen
11 years ago

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.

#3 in reply to: ↑ 2 @nacin
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 do foreach ( 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 @wonderboymusic
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 @wonderboymusic
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.

#6 @wonderboymusic
11 years ago

In 27862:

Metadata for audio and video files:

  • Make attachment metadata for audio files editable by providing a metabox on the Edit Media page
  • Standardize on using the attachment title everywhere
  • Label the Caption and Description fields for audio and video appropriately
  • Make the playlist Underscore templates more straightforward

See #27574.

#7 @nacin
11 years ago

In 27864:

Cleanups for audio/video metadata, see [27862].

see #27574.

#8 @nacin
11 years ago

In 27866:

Use correct formatting function. see [27864], see #27574.

@nacin
11 years ago

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

@nacin
11 years ago

#11 @wonderboymusic
11 years ago

In 27868:

Favor escaping over interpolation in Underscore templates for media. Translate some strings directly instead of passing via JS.

Props nacin.
See #27574.

#12 @wonderboymusic
11 years ago

In 27869:

Cleanup up the display, escaping, and handling of ID3 data for media. Rename wp_get_relevant_id3_keys() to wp_get_attachment_id3_keys().

Props nacin.
See #27574.

#13 @wonderboymusic
11 years ago

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

Marking this as fixed. We went several rounds.

#14 @wonderboymusic
11 years ago

In 27960:

Revert the changes made to data.title in playlist Underscore templates in [27869]. {{ data.title }} causes the title to be double-encoded on the front end.

See #27574.

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

#17 @nacin
11 years ago

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

Ticket for the escaping: #27710.

Note: See TracTickets for help on using tickets.