WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#29013 closed defect (bug) (fixed)

MP3s in Media Library do not open edit modal

Reported by: 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 4 years ago.
Screenshot of error console for reference
29013.diff (593 bytes) - added by ericmann 4 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 4 years ago.
Test artist and album tags separately.
29013.3.diff (563 bytes) - added by ericmann 4 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
4 years ago

Screenshot of error console for reference

@ericmann
4 years ago

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

#1 @ericmann
4 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
4 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
4 years ago

  • Keywords has-patch added

#4 in reply to: ↑ 2 @ericmann
4 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
4 years ago

Test artist and album tags separately.

@ericmann
4 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
4 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
4 years ago

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

Fixed in [29281].

#7 @programmin
4 years ago

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

Note: See TracTickets for help on using tickets.