WordPress.org

Make WordPress Core

Opened 9 months ago

Closed 5 months ago

Last modified 5 months ago

#51660 closed task (blessed) (fixed)

Adding a filter to WP_List_Table::months_dropdown() to allow overriding the list of months displayed

Reported by: geoffguillain Owned by: SergeyBiryukov
Milestone: 5.7 Priority: normal
Severity: normal Version:
Component: Administration Keywords: has-patch commit has-dev-note
Focuses: performance Cc:

Description

In WP_List_Table::months_dropdown(), a query is run to determine the months that have post items.

This query can be expensive for large posts/custom posts listing, so it may be desirable for sites to override this behavior. I am monitoring around 500ms for 500k+ posts.

The filter available, disable_months_dropdown, only allows removing the dropdown not overriding the query.

A similar filter (media_library_months_with_files) has been set previously for the media library. I was expecting a similar behavior for the month dropdown on post listing pages

Attachments (2)

51660-before-after-pr967.gif (2.0 MB) - added by hellofromTonya 6 months ago.
Tested before and after applying PR 967 along with a callback to modify the months. Works as expected.
51660.diff (1.0 KB) - added by hellofromTonya 6 months ago.
Restores the "months_dropdown_results" filter back into main logic flow for it to be applied.

Download all attachments as: .zip

Change History (33)

This ticket was mentioned in PR #767 on WordPress/wordpress-develop by geoffguillain.


8 months ago

  • Keywords has-patch added

In WP_List_Table::months_dropdown(), a query is run to determine the months that have post items.
This query can be expensive for large posts/custom posts listing, so it may be desirable for sites to override this behavior. I am monitoring around 500ms for 500k+ posts.
The filter available, disable_months_dropdown, only allows removing the dropdown not overriding the query.

This modification will enable to override the query. A similar filter (media_library_months_with_files) has been set previously for the media library.

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

#2 @prbot
8 months ago

github-actions[bot] commented on PR #767:

Hi @geoffguillain! 👋

Thank you for your contribution to WordPress! 💖

It looks like this is your first pull request to wordpress-develop. Here are a few things to be aware of that may help you out!

No one monitors this repository for new pull requests. Pull requests must be attached to a Trac ticket to be considered for inclusion in WordPress Core. To attach a pull request to a Trac ticket, please include the ticket's full URL in your pull request description.

Pull requests are never merged on GitHub. The WordPress codebase continues to be managed through the SVN repository that this GitHub repository mirrors. Please feel free to open pull requests to work on any contribution you are making.

More information about how GitHub pull requests can be used to contribute to WordPress can be found in this blog post.

Please include automated tests. Including tests in your pull request is one way to help your patch be considered faster. To learn about WordPress' test suites, visit the Automated Testing page in the handbook.

If you have not had a chance, please review the Contribute with Code page in the WordPress Core Handbook.

The Developer Hub also documents the various coding standards that are followed:

Thank you,
The WordPress Project

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


7 months ago

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


7 months ago

#5 @SergeyBiryukov
7 months ago

  • Milestone changed from Awaiting Review to 5.7

Hi there, welcome to WordPress Trac! Thanks for the ticket and the PR.

Yes, I think a filter makes sense there. It would need a documentation block as per the documentation standards, looks good to me otherwise.

#6 @SergeyBiryukov
7 months ago

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

#7 @Hareesh Pillai
7 months ago

  • Keywords needs-dev-note added

#8 @geoffguillain
7 months ago

A documentation block has been added to the PR.

#9 @Hareesh Pillai
7 months ago

Thanks for the update. It would be great if we could maintain consistency with the doc block for media_library_months_with_files filter.

#10 @geoffguillain
7 months ago

Thanks for pointing this out @hareesh-pillai.
The documentation block has been updated to be more consistent with the media_library_months_with_files filter.

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


6 months ago

#12 @hellofromTonya
6 months ago

Adding a code suggestion to the PR https://github.com/WordPress/wordpress-develop/pull/767#discussion_r567882010:

The 1st argument passed to apply_filters should be the value being filtered. In this case, the filter is filtering months, not the post type.

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


6 months ago

#14 @TimothyBlynJacobs
6 months ago

The naming for this feels like it'd be filtering the returned months. Should this instead be named like our other short-circuit filters and use pre in the name?

This ticket was mentioned in PR #967 on WordPress/wordpress-develop by hellofromtonya.


6 months ago

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

Updates PR #767 to modify the added filter to be a short-circuit for the query.

#16 @hellofromTonya
6 months ago

Notes from today's scrub:

We agreed the intent of this ticket is to bypass/skip the query. A filter already exists for filtering the months result, i.e. months_dropdown_results.

To expedite this ticket to be included in tomorrow's 5.7 Beta 1, I created PR 967 to capture the changes.

@hellofromTonya
6 months ago

Tested before and after applying PR 967 along with a callback to modify the months. Works as expected.

#17 @hellofromTonya
6 months ago

Testing information:
See the attached gif for before and after.

Step 1: Log into your test site.

Step 2: Go to the Posts.

Step 3: Select the "All dates" dropdown.

Step 4: Apply the PR by doing:

npm run grunt patch:https://github.com/WordPress/wordpress-develop/pull/967

Step 5: Add the following code to a must-use file or in the theme:

add_filter(
	'pre_months_dropdown_query',
	function( $months, $post_type ) {
		if ( 'post' !== $post_type ) {
			return $months;
		}

		return array(
			(object) array( 'year' => 2020, 'month' => 10 ),
			(object) array( 'year' => 2020, 'month' => 11 ),
			(object) array( 'year' => 2020, 'month' => 12 ),
			(object) array( 'year' => 2021, 'month' => 1 ),
			(object) array( 'year' => 2021, 'month' => 2 ),
		);
	},
	10,
	2
);

Step 7: Refresh the Posts page.

Step 8: Select the "All dates" dropdown. Notice the selections have changed.

#18 @hellofromTonya
6 months ago

  • Keywords commit added

PR reviewed and approved by Timothy. Marking this one for commit.

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


6 months ago

#20 @whyisjake
6 months ago

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

In 50163:

Administration: New filter ahead of the months drop-down.

As this can cause large, long running queries on sites with many posts, this filter allows the query to be modified, bypassing entirely if needed.

Fixes #51660.

Props geoffguillain, SergeyBiryukov, hareesh-pillai, hellofromTonya, TimothyBlynJacobs, whyisjake.

#23 follow-up: @SergeyBiryukov
6 months ago

  • Keywords commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

With [50163], the months_dropdown_results filter is no longer applied if the new pre_months_dropdown_query filter is used. I think it should only short-circuit the query and not the other filter.

@hellofromTonya
6 months ago

Restores the "months_dropdown_results" filter back into main logic flow for it to be applied.

#24 in reply to: ↑ 23 @hellofromTonya
6 months ago

  • Keywords commit added

Replying to SergeyBiryukov:

With [50163], the months_dropdown_results filter is no longer applied if the new pre_months_dropdown_query filter is used. I think it should only short-circuit the query and not the other filter.

51660.diff restores/reverts the logic position of the months_dropdown_results filter. It will be applied regardless of whether the $months array of objects is sourced from the pre_months_dropdown_query filter or db query.

Marking for commit.

#25 follow-up: @TimothyBlynJacobs
6 months ago

I'm not strongly opposed. But the reason I recommended it behave this way is that the vast majority of core pre_ short-circuit filters I saw did not have their "standard" filter applied.

#26 in reply to: ↑ 25 ; follow-up: @SergeyBiryukov
6 months ago

Replying to TimothyBlynJacobs:

I'm not strongly opposed. But the reason I recommended it behave this way is that the vast majority of core pre_ short-circuit filters I saw did not have their "standard" filter applied.

Ah, good point. At a glance, there are some that immediately return without any other filters applied, and some that just short-circuit the query or heavy processing and continue to other filters. I can circle back after a closer review.

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


5 months ago

#28 @francina
5 months ago

  • Type changed from enhancement to task (blessed)

#29 in reply to: ↑ 26 @SergeyBiryukov
5 months ago

Replying to SergeyBiryukov:

Replying to TimothyBlynJacobs:

I'm not strongly opposed. But the reason I recommended it behave this way is that the vast majority of core pre_ short-circuit filters I saw did not have their "standard" filter applied.

Ah, good point. At a glance, there are some that immediately return without any other filters applied, and some that just short-circuit the query or heavy processing and continue to other filters. I can circle back after a closer review.

I still don't have a full list at this point, but the pre_months_dropdown_query filter seems closer to the latter pattern (also used by the pre_move_uploaded_file filter, for example), so I think 51660.diff would be preferred here.

#30 @SergeyBiryukov
5 months ago

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

In 50362:

Administration: Apply the months_dropdown_results filter separately from pre_months_dropdown_query.

Follow-up to [50163].

Props hellofromTonya.
Fixes #51660.

Note: See TracTickets for help on using tickets.