WordPress.org

Make WordPress Core

Opened 22 months ago

Last modified 18 months ago

#43009 new defect (bug)

Create Video/Audio Playlist hooks not working as expected

Reported by: iforrest Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 4.9.1
Component: Media Keywords: needs-patch
Focuses: administration Cc:
PR Number:

Description

WordPress now provides the following hooks:

https://developer.wordpress.org/reference/hooks/media_library_show_video_playlist/
https://developer.wordpress.org/reference/hooks/media_library_show_audio_playlist/

We can use these to hide the “Create Video Playlist” (and Audio) button in the media library.

These only seem to work on initial page load. The “Create Video Playlist” button still appears for me even though I'm using these filters.

To reproduce from a fresh install.

  1. Set these lines in your theme functions.php
add_filter( 'media_library_show_audio_playlist', function() { return false; });
add_filter( 'media_library_show_video_playlist', function() { return false; });
  1. Go to wp-admin/post-new.php.
  1. Click 'Add Media', notice that the playlist buttons are missing.
  1. Upload a video file (I used .mov). Notice that the 'Create Video Playlist' button now appears.

This likely exists in versions earlier than 4.9.1, but this was the version I tested in.

Attachments (1)

with_media_library_show_audio_playlist_filter_set_to_zero.gif (231.8 KB) - added by birgire 18 months ago.

Download all attachments as: .zip

Change History (9)

#1 @birgire
18 months ago

  • Focuses administration added
  • Keywords needs-patch added

@iforrest thanks for the ticket and welcome to Trac.

That sounds like a bug.

The with_media_library_show_audio_playlist_filter_set_to_zero.gif screen-cast shows the issue, given the filtering:

add_filter( 'media_library_show_audio_playlist', '__return_false' );

When a new audio file is uploaded, then the "Create Audio Playlist" link shows up, regardless of the above filtering.

The reason for this comes within the bindHandlers function here:

https://core.trac.wordpress.org/browser/tags/4.9.5/src/wp-includes/js/media-views.js#L3391

For example the audio/video playlist menu items are hidden/displayed via:

https://core.trac.wordpress.org/browser/tags/4.9.5/src/wp-includes/js/media-views.js#L3452

	activate: function() {
		// Hide menu items for states tied to particular media types if there are no items
		_.each( this.counts, function( type ) {
			if ( type.count < 1 ) {
				this.menuItemVisibility( type.state, 'hide' );
			}
		}, this );
	},

	mediaTypeCounts: function( model, attr ) {
		if ( typeof this.counts[ attr ] !== 'undefined' && this.counts[ attr ].count < 1 ) {
			this.counts[ attr ].count++;
			this.menuItemVisibility( this.counts[ attr ].state, 'show' );
		}
	},

where e.g. the count is updated via this event listener:

https://core.trac.wordpress.org/browser/tags/4.9.5/src/wp-includes/js/media-views.js#L3403

if ( typeof checkCounts !== 'undefined' ) {
	this.listenTo( wp.media.model.Attachments.all, 'change:type', this.mediaTypeCounts );
}

The attachmentCounts settings comes from:

'attachmentCounts' => array(
	'audio' => ( $show_audio_playlist ) ? 1 : 0,
	'video' => ( $show_video_playlist ) ? 1 : 0,
),

https://core.trac.wordpress.org/browser/tags/4.9.5/src/wp-includes/media.php#L3451

This shows that there are two state-values possible for audio and video in attachmentCounts, but we have possible 3 state-values from the filtering (e.g. audio):

$show_audio_playlist = apply_filters( 'media_library_show_audio_playlist', true );
if ( null === $show_audio_playlist ) {
	$show_audio_playlist = $wpdb->get_var(...);
}

#2 @adamsilverstein
18 months ago

Hi @iforrest - thanks for the report, and thanks @birgire for the additional sleuthing.

I think this is actually the expected behavior of the filter, it only affects what happens on initial page load and is not designed to hide the button always, although I can see how the naming might indicate that. These filters were introduced in https://core.trac.wordpress.org/ticket/31071 to enable developers to re-enable the (expensive) query for sound/video files when the page loads. By returning false, you are setting the button initial state to hidden, however once you change the media collection by uploading audio, the Create Audio Playlist option should appear.

See docs at https://developer.wordpress.org/reference/hooks/media_library_show_audio_playlist/ & https://developer.wordpress.org/reference/hooks/media_library_show_video_playlist/

Last edited 18 months ago by adamsilverstein (previous) (diff)

#3 @iforrest
18 months ago

I understand that this is how it's coded to work, but the filters have descriptions like "Allows showing or hiding the “Create Video Playlist” button in the media library." To me it seems like an incomplete feature.

#4 @birgire
18 months ago

Thanks @adamsilverstein, I remember watching that ticket at the time but I never realized that this was the intended behavior of the filter ;-)

If this is the intended behavior, then adjusting the filter doc comments would help here.

#5 @adamsilverstein
18 months ago

I agree, the doc block is misleading. We should probably update it to reflect the actual behavior. Do you think completely disabling these links is useful?

#6 @birgire
18 months ago

@adamsilverstein I think I agree with you.

In versions before 4.8, the previous "Create Audio Playlist" link was displayed if the Media Library contained any audio, else it was hidden. Same for videos.

There we had:

'attachmentCounts' => array(
    'audio' => ( $has_audio ) ? 1 : 0,
    'video' => ( $has_video ) ? 1 : 0
),

with $has_audio and $has_video from possible very heavy database queries.

So looking at ticket #31071 again, I think the intention there wasn't to change the original behavior of the "Create Audio/Videos Playlist" links, were it was hidden in the absence of audios/videos.

In hindsight I think the previous $has_audio and $has_video naming was more clear,
compared to the changes in 4.8:

'attachmentCounts' => array(
	'audio' => ( $show_audio_playlist ) ? 1 : 0,
	'video' => ( $show_video_playlist ) ? 1 : 0,
),

I noticed that if we delete the last audio/video then the create playlist menu items are not hidden.

Maybe there's a way to check this.counts[ attr ].count < 1 on audio/video delete and if so, hide the menu item with this.menuItemVisibility( this.counts[ attr ].state, 'hide' );

Would that be consistent with the display of the create playlist menu items, when the first audio/video are uploaded?

Last edited 18 months ago by birgire (previous) (diff)

#7 @adamsilverstein
18 months ago

I noticed that if we delete the last audio/video then the create playlist menu items are not hidden.

Yea, the inconsistencies are a bit frustrating, in part because we've hijacked these $has_ variables with a setting that i think really only applies on load. I'm going to dig in to see why that happens.

The docs should make it clear you can pass three things to the filter: true, false or null - each with different behaviors.

  • null: perform a query to see if media exists, initially show button if present
  • true (default): always show button (initially)
  • false: never show button (initially)

Thinking about this more, maybe we could consider changing how the filter works so that true and false actually persist beyond initial load, which is the core expectation that raised this ticket. This actually seems more useful unless I'm missing some use case.

The filter action would then become:

  • null: perform a query to see if media exists, initially show button if present
  • true (default): always show button (even if all items removed)
  • false: never show button (even if new items added)

What do you think?

#8 @birgire
18 months ago

I think your doc comment proposal looks good.

At least it looks like it's easier to explain the (persistent) behaviour rather than the (initially) behaviour ;-) because the user might wonder what happens after (initally) ;-)

I tried to see if it's possible to add some extra logic into the callback here:

this.listenTo( wp.media.model.Attachments.all, 'change:type', this.mediaTypeCounts );

but it looks like it will not fire when audio/video is deleted. The deleteAttachment event fires though.

Note: See TracTickets for help on using tickets.