Make WordPress Core

Opened 6 years ago

Last modified 2 years ago

#43009 new defect (bug)

Create Video/Audio Playlist hooks not working as expected

Reported by: iforrest's profile iforrest Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 4.9.1
Component: Media Keywords: has-patch
Focuses: administration Cc:

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 6 years ago.

Download all attachments as: .zip

Change History (11)

#1 @birgire
6 years 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
6 years 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 6 years ago by adamsilverstein (previous) (diff)

#3 @iforrest
6 years 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
6 years 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
6 years 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
6 years 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 Playlist" links. i.e. to hide it 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?

Version 0, edited 6 years ago by birgire (next)

#7 @adamsilverstein
6 years 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
6 years 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.

#9 @isuke01
4 years ago

Will it be ever fixed?

This ticket was mentioned in PR #2461 on WordPress/wordpress-develop by erengy.


2 years ago
#10

  • Keywords has-patch added; needs-patch removed

### How to reproduce

On a fresh installation of WordPress 5.9.2 with Classic Editor plugin enabled, add the following lines to the default theme's functions.php:

{{{php
add_filter('media_library_show_audio_playlist', 'return_false');
add_filter('media_library_show_video_playlist', '
return_false');
}}}

Then go to /wp-admin/post-new.php and click the Add Media button.

These filters are supposed to allow hiding the Create Audio Playlist and Create Video Playlist buttons, which is not the case. The buttons are still visible in the Add Media modal dialog.

Workarounds include filtering media_view_strings instead, or enqueing additional styles to hide the buttons.

The discussion at Ticket 43009 focuses on the behavior *after* uploading an audio/video file rather than the initial state, hence the new ticket.

### Reason

PHP: wp_enqueue_media function sets attachmentCounts according to the filter results.

JavaScript: menuItemVisibility function is called, which adds hidden class to the elements.

❌ CSS: In media-views.css, .media-menu .media-menu-item selector wins over .media-frame .hidden, setting the elements' display property to block instead of none.

### Proposed fix

Since there are similar rules of higher specificity (e.g. .attachments-browser .uploader-inline.hidden, .media-embed .setting input.hidden) in the same CSS file, I thought adding the following rule would be appropriate:

{{{css
.media-menu .media-menu-item.hidden {

display: none;

}
}}}

Trac ticket: https://core.trac.wordpress.org/ticket/55465

Note: See TracTickets for help on using tickets.