WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 2 months ago

#41675 reviewing enhancement

Add filter to bypass display/query for Media Months filter in media modal

Reported by: CrazyJaco Owned by: joemcgill
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Media Keywords: good-first-bug needs-patch
Focuses: performance Cc:
PR Number:

Description

I am investigating slow running queries to try and cut down the page load time for edit pages in the admin.

The media months dropdown filter runs a somewhat expensive query for what it is.
On our production database, it adds about 500 ms of query execution time to the page load.

There doesn't seem to be a way to be able to disable this drop down.
The query that is run populates the field.
But when there are a mass quantity of media, this is not an optimal method of search.

There should be an option/filter to disable the display and query for this.

Attachments (2)

41675.diff (7.2 KB) - added by herregroen 2 years ago.
Adds a 'media_library_show_months_select' which returns a boolean.
#41675-v2.patch (28.1 KB) - added by justinahinon 2 months ago.

Download all attachments as: .zip

Change History (10)

#1 @CrazyJaco
2 years ago

This filter hook appears to exist:
media_library_months_with_files

If the returned value is anything other than an array, it will execute default behavior (running the query and displaying the drop down)

If the returned value is an array, it will skip the query and populate the drop down with whatever is in the array.
If an empty array is passed, the drop down will still appear with no options to select.

#2 @johnbillion
2 years ago

  • Focuses performance added
  • Keywords needs-patch good-first-bug added

Thanks for the report, @CrazyJaco .

This needs a patch to clarify the media_library_months_with_files filter description, and to change the dropdown behaviour so it's not shown when there are no elements in the corresponding array.

#3 @CrazyJaco
2 years ago

Thanks!

I spoke with @joemcgill at contributor day for WordCamp for Publishers about this. I would like to work on this with some guidance from him.

Last edited 2 years ago by CrazyJaco (previous) (diff)

@herregroen
2 years ago

Adds a 'media_library_show_months_select' which returns a boolean.

#4 @herregroen
2 years ago

  • Keywords has-patch added; needs-patch removed

Just added a patch which adds a 'media_library_show_months_select' filter which returns a boolean determining whether or not to show the months select. Returning false will not mean 'media_library_months_with_files' filter is not run and the default query is not run either.

In the browser the months select and it's accompanying accessibility label are not rendered if false is returned.

I felt it was clearer to add a new filter in the line of media_library_show_video_playlist and media_library_show_audio_playlist with a singular behaviour than to expand the behaviour of the media_library_months_with_files filter to include new behaviour.

Patch could also serve as an example for a patch that does change the behaviour of media_library_months_with_files.

Last edited 2 years ago by herregroen (previous) (diff)

#5 @joemcgill
2 years ago

  • Owner set to joemcgill
  • Status changed from new to reviewing

Thanks for the great start on this @herregroen!

I could see how a separate filter for this case might be helpful, but I think we can support this right now with the current filter if, as @CrazyJaco suggests, someone were to return an empty array, then the UI should not be shown.

There are a few places in the AttachmentsBrowser view where a new wp.media.view.DateFilter view is rendered, which will need to be accounted for — the grid view in the media library, and the default one used by the wp.media.frame. In the latter, there is already a check to se if this.options.date is true before rendering the DateFilter view. I think we could add a check for the length of wp.media.view.settings.months as well before rendering that view.

Additionally, in the list view of the media library, you can disable the months dropdown using the disable_months_dropdown filter. Seems like we could take advantage of this opportunity to unify the process for disabling the months dropdown between the media views and the WP_List_Table.

#6 follow-up: @joemcgill
2 years ago

  • Keywords needs-patch added; has-patch removed

@herregroen or @CrazyJaco – either of you interested in working on an update on the initial patch? We could still get this in 4.9 if there's a new patch in the next week.

#7 in reply to: ↑ 6 @kjphilips
6 months ago

Replying to joemcgill:

@herregroen or @CrazyJaco – either of you interested in working on an update on the initial patch? We could still get this in 4.9 if there's a new patch in the next week.

Is this still open and needing attention? I want to get involved in core and not really sure how to grab a ticket. Thanks!!

#8 @justinahinon
2 months ago

I dig into this recently and I think @herregroen 's patch can be adapted lightly for the existing filter.
Also, I found that the structure of the JavaScript files in the wp-includes change a lot, from WordPress 4.7.4 to 5.2.2 and also on the trunk SVN.

So it's could possible that I missed a JS file where changes are needed. Would be great if someone can double check my patch.

Note: See TracTickets for help on using tickets.