-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Get archives: add filter to customize query LIMIT clause #10552
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
Conversation
This new filter will allow site admins to customize the LIMIT clause for all wp_get_archives requests on their site. Trac ticket: https://core.trac.wordpress.org/ticket/64304
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
src/wp-includes/general-template.php
Outdated
| * @param string $limit The limit of the query for the `wp_get_archives` function. | ||
| * @param array $parsed_args An array of default arguments. | ||
| */ | ||
| $limit = apply_filters( 'getarchives_limit', $parsed_args['limit'], $parsed_args ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a need to filter the actual LIMIT clause? Instead of filtering the clause, what about filtering the number for the limit? So up above instead of:
wordpress-develop/src/wp-includes/general-template.php
Lines 2034 to 2037 in 4cac4dc
| if ( ! empty( $parsed_args['limit'] ) ) { | |
| $parsed_args['limit'] = absint( $parsed_args['limit'] ); | |
| $parsed_args['limit'] = ' LIMIT ' . $parsed_args['limit']; | |
| } |
It could be:
/**
* Filters the limit for the number of posts to include in the archive.
*
* @since 7.0.0
*
* @param int $limit The limit for the number of posts to include in the archive. Default 0.
* @param array $parsed_args An array of default arguments.
*/
$limit_number = (int) apply_filters( 'getarchives_limit', absint( $parsed_args['limit'] ), $parsed_args );
if ( $limit_number > 0 ) {
$limit = " LIMIT $limit_number";
} else {
$limit = '';
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I updated the code sample to set the $limit variable.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can certainly do that. I pushed b4c617d with your exact suggestion.
I wondered if we needed to both cast as an integer and use absint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I personally wouldn't have used absint() to begin with.
Co-authored-by: Weston Ruter <[email protected]>
|
Something that just came to mind: instead of introducing a filer specifically for the |
Co-authored-by: Weston Ruter <[email protected]>
Co-authored-by: Weston Ruter <[email protected]>
This would work ; I suppose it would then not be consistent with the existing filters for the archives though. |
|
I think that's OK. The other filters are explicitly lower-level for manipulating the SQL, whereas this |
|
I just pushed a new commit to try that. |
westonruter
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any thoughts about placement of the filter before or after the $parsed_args are collected? Core is inconsistent on this, it seems. The wp_nav_menu_args filter applies after merging the passed $args with the $defaults (and some validation), whereas the wp_terms_checklist_args filter applies before merging the args with the defaults.
| return; | ||
| } | ||
|
|
||
| $parsed_args['post_type'] = $post_type_object->name; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a curious bit of code, isn't it. Wouldn't this already be the same as $parsed_args['post_type']?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose that may be a way to get capitalization right? It looks like it was added in https://core.trac.wordpress.org/ticket/21596
Co-authored-by: Weston Ruter <[email protected]>
Co-authored-by: Weston Ruter <[email protected]>
Co-authored-by: Weston Ruter <[email protected]>
I thought about it too, and figured it may be nice to have the filtering happen before, so we still come out with a consistently formatted |
wp_get_archivesalready comes with 2 filters allowing one to customize some of the aspects of the queries to fetch archives on a site:getarchives_whereallows you to customize theWHEREclausegetarchives_joinallows you to customize theJOINclauseI think it would be useful to add a new filter, to allow customizing the
LIMITclause as well. In some environments, with sites with lots of posts with longform content, some of those queries can be really resource-intensive.Note
I have 2 notes about my PR:
7.0.0as the since value. I'm not sure if I'm supposed to use a placeholder until this is ready for commit.wp_prefix of other, newer filters. Let me know if I should change that.Trac ticket: https://core.trac.wordpress.org/ticket/64304