Make WordPress Core

Opened 7 years ago

Last modified 16 months ago

#43658 assigned defect (bug)

Media attachment filter drop-down mime types in grid mode is not properly filtered by available mime types

Reported by: itzmekhokan's profile itzmekhokan Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 4.9.4
Component: Media Keywords: has-screenshots phase-3-media-triage
Focuses: Cc:

Description

In Gird mode media attachment filter is not properly filtered by available mime types, it shows all mime types by default via -

get_post_mime_types()

But in List mode its already filtered by checking

if ( ! wp_match_mime_types( $mime_type, $avail_post_mime_types ) ) {
	continue;
}

Attachments (7)

media-grid-attachment-filter-dropdown.png (330.0 KB) - added by itzmekhokan 7 years ago.
Grid view attachments filters drop-down
media-list-attachment-filter-dropdown.png (141.6 KB) - added by itzmekhokan 7 years ago.
List view attachment filters drop-down
43658.patch (1.3 KB) - added by itzmekhokan 7 years ago.
43658.1.patch (1.3 KB) - added by itzmekhokan 7 years ago.
Corrected coding standards by added whitespace
43658.diff (3.0 KB) - added by adamsilverstein 6 years ago.
43658.2.diff (9.3 KB) - added by adamsilverstein 6 years ago.
43658.3.diff (16.1 KB) - added by adamsilverstein 6 years ago.

Download all attachments as: .zip

Change History (33)

@itzmekhokan
7 years ago

Grid view attachments filters drop-down

@itzmekhokan
7 years ago

List view attachment filters drop-down

@itzmekhokan
7 years ago

#1 @itzmekhokan
7 years ago

  • Keywords has-patch added

#2 @itzmekhokan
7 years ago

  • Keywords has-screenshots added

#3 @adamsilverstein
7 years ago

  • Keywords needs-testing needs-unit-tests added
  • Milestone changed from Awaiting Review to 5.0
  • Owner set to adamsilverstein
  • Status changed from new to assigned

@itzmekhokan thanks for the bug report and patch!

Overall your patch looks good and I see the issue you are trying to resolve. Can you please provide the code snippet you are using to alter the dropdown for testing? Also, it would be nice to add a a unit test confirming the filter works as expected and changes the return from wp_enqueue_media.

One small coding standards correction, this line is missing some whitespace before/after brackets: unset($mimeTypes[$mime_type]);

@itzmekhokan
7 years ago

Corrected coding standards by added whitespace

#4 @itzmekhokan
7 years ago

I have tested the flow thoroughly with the 43658.1.patch and it is working properly, as expected and changes the return from wp_enqueue_media.

However, I faced an issue with unit testing. Can you please guide me with the flow, i need to follow for the same.

Looking for your co-operation dearly.

#5 @pento
6 years ago

  • Milestone changed from 5.0 to 5.1

#6 @adamsilverstein
6 years ago

  • Milestone changed from 5.1 to 5.2

This one needs some more attention before its ready. Punting to 5.2.

#7 @adamsilverstein
6 years ago

43658.diff includes unit tests that verify the correct mime types are available to the grid. these tests fail before the patch and succeed after it.

@itzmekhokan can you take a look at what I did for the unit tests - let me know if you have any questions - and also verify this resolves the issue you were seeing.

#8 @adamsilverstein
6 years ago

Noting in travis I'm seeing phpcs errors that can't be fixed automatically: https://travis-ci.org/adamsilverstein/wordpress-develop-fork/jobs/500433986. I'm assuming I need to fix these before committing.

This ticket was mentioned in Slack in #core-committers by adamsilverstein. View the logs.


6 years ago

#10 @adamsilverstein
6 years ago

Travis was failing (https://travis-ci.org/adamsilverstein/wordpress-develop-fork/jobs/500433986) because the linting errors in wp-includes/media.php could not be fixed automatically.

I ran phpcs --standard=phpcs.xml.dist src/wp-includes/media.php and manually addressed all errors. Errors fixed included "Assignments must be the first block of code on a line" and "Use Yoda Condition checks, you must" as well as "Use placeholders and $wpdb->prepare(); found $sql"

43658.3.diff includes code fixes in response to phpcs

travis build started: https://travis-ci.org/adamsilverstein/wordpress-develop-fork/builds/501627830

#11 @adamsilverstein
6 years ago

  • Keywords has-unit-tests added; needs-testing needs-unit-tests removed

#12 @adamsilverstein
6 years ago

  • Keywords commit added

43658.diff is ready: not going to worry about the phpcs errors for unchanged code.

#13 @adamsilverstein
6 years ago

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

In 44947:

Media: grid view attachment filter drop down - only show available mime types.

In the media library gird view, properly limit the media attachment filter to available mime types, matching the list view.

Props itzmekhokan.
Fixes #43658.

#14 @RogerTheriault
6 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

It seems the use of get_available_post_mime_types here is potentially an issue for sites with many posts - in testing we saw the query scan the whole posts table and for sites with > 1 million rows, that's measured in seconds - the query here could possibly benefit from some caching.

SELECT DISTINCT post_mime_type FROM wp_posts WHERE post_type = 'attachment'

I reopened #44675 because it appeared to be related, but it's likely this ticket is the nexus of that change, can it be reconsidered or tested for larger sites before including in 5.2?

#15 @pento
6 years ago

  • Keywords has-patch has-unit-tests commit removed

Much like #31071, I think the only reasonable option is to remove the query that does the table scan.

In this instance, I'm inclined to revert [44947] for 5.2. We can explore removing it from WP_Media_List_Table later, as the performance will be similarly bad in the list form, too.

This ticket was mentioned in Slack in #core by pento. View the logs.


6 years ago

#17 @azaozz
6 years ago

+1 on reverting for now and adding this after the way get_available_post_mime_types() works is fixed/improved.

#18 @pento
6 years ago

  • Owner changed from adamsilverstein to pento
  • Status changed from reopened to assigned

#19 @pento
6 years ago

In 45270:

Media: Revert [44947].

get_available_post_mime_types() uses a query that's extremely slow on sites with lots of posts. The original fix can be revisited after those performance issues are tackled.

See #43658.

#20 @pento
6 years ago

  • Milestone changed from 5.2 to 5.3

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


5 years ago

#22 @kirasong
5 years ago

@pento Would you like to do anything here for 5.3?

#23 @kirasong
5 years ago

  • Milestone changed from 5.3 to Future Release

As we're preparing for Beta 3, there hasn't been movement, and this looks like a more complex change, I'm going to go ahead and move this to Future Release, but feel free to move it back for discussion (or to 5.4) if desired.

#24 @pento
5 years ago

  • Owner pento deleted

This ticket was mentioned in Slack in #core-media by sabernhardt. View the logs.


4 years ago

#26 @joedolson
16 months ago

  • Keywords phase-3-media-triage added
Note: See TracTickets for help on using tickets.