Opened 5 years ago
Last modified 7 days ago
#50568 assigned enhancement
Improve WP_Term's sanitization calls
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 6.9 | Priority: | normal |
Severity: | normal | Version: | 5.5 |
Component: | Taxonomy | Keywords: | has-patch early |
Focuses: | Cc: |
Description
Akin to WP_Post::filter()
, I think WP_Term::filter()
should have a bypass, too. This will significantly improve performance wherever terms are used (list edit screens, category widgets, blocks) because the bypass prevents redundant re-sanitization.
The attached patch will shave off a 30% load time at wp-admin/edit.php
after #50567's implementation. This patch tests for the WP_Term
object's ::$filter
state, and only re-sanitizes the term when the state differs from the input argument.
You'll also find that sanitize_term()
now looks a lot like sanitize_post()
--the same goes for get_term()
's resemblance to get_post()
. Overall, it seems posts have received a lot more love over the years, and this patch steals some of that.
There are a few issues with terms, however. For example, update_term_cache()
caches terms regardless of being shared, while WP_Term::get_instance()
tries to prevent that. The $filter = 'display'
argument is used in contexts where raw
would do fine, too (e.g. @ WP_Terms_List_Table::single_row()
). If we iron those issues out, we can fully phase out the re-sanitization of terms.
Attachments (1)
Change History (18)
This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.
5 years ago
This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.
5 years ago
This ticket was mentioned in Slack in #core by lukecarbis. View the logs.
5 years ago
This ticket was mentioned in Slack in #core by metalandcoffee. View the logs.
5 years ago
This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.
4 years ago
#8
@
4 years ago
- Milestone changed from 5.7 to Future Release
This ticket is marked for early
in the alpha cycle. We are now in the Beta cycle and long past early
. Moving this ticket and #50567 to Future Release
. However, would be great to get both committed early in 5.8 alpha cycle if possible.
If any maintainer or committer feels this can be resolved in time, or wishes to assume ownership during a specific cycle, feel free to update the milestone accordingly.
#9
@
3 years ago
@Cybr would you mind creating a PR on github.com/wordpress/wordpress-develop? #50567 is already merge so we can target this for next milestone? cc. @peterwilsoncc
#10
@
16 months ago
Here's a snippet of a bypass I've been using for the past 4 years.
I put in a collective fix-up plugin called "WP Fix: Unified Core Kit," which I might publish here one day.
add_filter(
'get_term',
/**
* Properly primes term cache. Otherwise, terms get resanitized every time they're called.
*
* @since 1.0.0
* @link <https://core.trac.wordpress.org/ticket/50568>
*
* @param WP_Term $term Term object.
* @param string $taxonomy The taxonomy slug.
*/
function( $term, $taxonomy ) {
$_term = wp_cache_get( $term->term_id, 'terms' );
if ( $_term && empty( $_term->filter ) ) {
$term = sanitize_term( $term, $taxonomy, 'raw' );
wp_cache_replace( $term->term_id, $term, 'terms' );
}
return $term;
},
-100, // Low, for others might add data to the term object, which we don't want to filter out...
2
);
Saves 2 redundant database requests on my tiny homepage (running WP 6.4.3).
This ticket was mentioned in Slack in #core-performance by pbearne. View the logs.
5 months ago
#12
@
5 months ago
- Focuses performance removed
- Milestone changed from Future Release to 6.9
- Owner set to pbearne
- Status changed from new to assigned
- Type changed from defect (bug) to enhancement
I'm unsure about the performance value of this ticket without a good set of metrics to back up the proposal. That said, the proposed change is merely achieving parity of WP_Term::filter()
with what WP_Post::filter()
already does. So from that perspective, it makes sense to me, regardless of whether this has a notable performance benefit or not.
Assigning to @pbearne as discussed on Slack.
#13
follow-up:
↓ 14
@
5 months ago
Pushed a first version
I had an issue with the slugs. I assumed that they should always be sanitized
this wasn't happening
#14
in reply to:
↑ 13
@
3 weeks ago
Replying to pbearne:
Thank you, everyone, for working on this.
Hi @pbearne, since this ticket has been inactive for some time, I would like to ask if you're still working on it.
#15
@
3 weeks ago
The pull request is here
https://github.com/pbearne/wordpress-develop/pull/135
Sorry, it wasn't in the ticket.
The tests are failing. we need to look at why
Paul
#16
@
3 weeks ago
- Keywords needs-unit-tests removed
Thanks @pbearne. I am gonna go ahead and remove the needs-unit-tests
tag since your PR already includes the required tests.
Additionally, since there have been numerous changes since you raised the PR, I think a rebase against the latest trunk might give a clearer picture of which tests are actually failing due to the changes made.
From today's 5.6 core scrub, Sergey:
Moving this ticket to
early
5.7
milestone along with #50567.If any maintainer or committer feels this can be resolved in time, or wishes to assume ownership during a specific cycle, feel free to update the milestone accordingly.