Make WordPress Core

Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#29013 closed defect (bug) (fixed)

MP3s in Media Library do not open edit modal

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

Description

Unlike images, documents, and other static media, MP3s are throwing an error in the Media Library when clicked to edit. This JS error means the MP3 will be highlighted, but no modal window is rendered to allow editing meta information about the attachment.

The exact error is:
Uncaught TypeError: Cannot read property 'artist' of undefined

It's triggered on /wp-admin/upload.php?item={item Id}:107

Screenshot of Chrome debugger console attached.

Attachments (4)

Screenshot 2014-07-23 19.04.21.png (83.9 KB) - added by ericmann 10 years ago.
Screenshot of error console for reference
29013.diff (593 bytes) - added by ericmann 10 years ago.
Patch media template file so missing artist/album ID3 tags don't trigger an error.
29013.2.diff (1.4 KB) - added by ericmann 10 years ago.
Test artist and album tags separately.
29013.3.diff (563 bytes) - added by ericmann 10 years ago.
As an alternative, let's just be sure all ID3 tags are printed to JS, even if they're empty. This removes the burden of checking against undefined out of the JS templates (since logic should be abstracted from templates anyway).

Download all attachments as: .zip

Change History (11)

@ericmann
10 years ago

Screenshot of error console for reference

@ericmann
10 years ago

Patch media template file so missing artist/album ID3 tags don't trigger an error.

#1 @ericmann
10 years ago

  • Keywords has-patch added

29013.diff follows the pattern of changeset [29208] by making sure data.artist and data.album are defined before trying to use them.

#2 follow-up: @helen
10 years ago

  • Keywords has-patch removed
  • Milestone changed from Awaiting Review to 4.0

Can we do the checks separately so an artist can be shown even if an album isn't? Or are both always defined when one is, just empty?

This also makes me wonder if we can generate ID3 data behind the scenes intelligently - we didn't do it when loading the library in the modal because it could trigger a bunch at once, but maybe there is something we can do further. Previously: #26825.

#3 @helen
10 years ago

  • Keywords has-patch added

#4 in reply to: ↑ 2 @ericmann
10 years ago

Replying to helen:

Can we do the checks separately so an artist can be shown even if an album isn't? Or are both always defined when one is, just empty?

We can, it'll break the elegance of using a foreach here though. Creating a second patch ...

This also makes me wonder if we can generate ID3 data behind the scenes intelligently - we didn't do it when loading the library in the modal because it could trigger a bunch at once, but maybe there is something we can do further. Previously: #26825.

I would argue something like this would be well-suited for an upgrade routine.

@ericmann
10 years ago

Test artist and album tags separately.

@ericmann
10 years ago

As an alternative, let's just be sure all ID3 tags are printed to JS, even if they're empty. This removes the burden of checking against undefined out of the JS templates (since logic should be abstracted from templates anyway).

#5 @adamsilverstein
10 years ago

Great!

I like this fix much better than adding the logic to the template. It makes sense that these values are blank when unset, and now they can be edited in the modal.

#6 @helen
10 years ago

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

Fixed in [29281].

#7 @programmin
10 years ago

This exception is happening in video playlist code still, #29125

Note: See TracTickets for help on using tickets.