Make WordPress Core

Opened 14 months ago

Closed 10 months ago

Last modified 10 months ago

#58327 closed enhancement (duplicate)

Remove unneeded sanitize term in `populate_terms` method

Reported by: spacedmonkey's profile spacedmonkey Owned by: spacedmonkey's profile spacedmonkey
Milestone: Priority: normal
Severity: normal Version: 6.0
Component: Taxonomy Keywords: has-patch
Focuses: performance Cc:

Description

In [52836] ( #37189 ) the WP_Term_Query class was converted to just get ids and then convert those ids into WP_Term objects using get_term. But in the case where fields all_with_object_id is passed, this results in lots of calls to sanitize_term. These calls can be extensive and effect performance. all_with_object_id expects an object as a result, but does not necessarily need to be sanitized.

Change History (10)

This ticket was mentioned in PR #4460 on WordPress/wordpress-develop by @spacedmonkey.


14 months ago
#1

  • Keywords has-patch added

#2 @spacedmonkey
11 months ago

  • Milestone changed from Future Release to 6.4

#3 @spacedmonkey
10 months ago

  • Owner set to spacedmonkey
  • Status changed from new to assigned

This ticket was mentioned in Slack in #core-performance by spacedmonkey. View the logs.


10 months ago

@spacedmonkey commented on PR #4460:


10 months ago
#5

@felixarntz @joemcgill

Basically, get_term has some overhead attached to it. It has filters and after the filtering, it rerun the sanitisation. When all_with_object_id is requsted ( in update_object_term_cache ), you only need the raw object. update_object_term_cache only needs to the term ids, so those filters and sanitisation are not needed. The sanitisation is run in WP_Term::get_instance() meaning no break in B/C. But as update_object_term_cache run on every WP_Query, which can be run 10+ times on many themes, ( block template parts, menus etc ), these calls to get_term can really really mount up, resulting in thousands of calls to sanative_term and apply_filters.

This change is a side effect of https://github.com/WordPress/wordpress-develop/commit/4a9f5fe3be47914b3f2b30aaf863761335cf1ad2 , resulting in the unexpected overhead.

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


10 months ago

@peterwilsoncc commented on PR #4460:


10 months ago
#7

@spacedmonkey I think get_post had the same overhead and was solved by bypassing the filtering when possible:

https://github.com/WordPress/wordpress-develop/blob/5ebe28966e5decbe1862c147bf98db0d8094038e/src/wp-includes/class-wp-post.php#L251-L253

Would this work in WP_Term::get_instance instead of changing WP Query? The terms are cached with raw sanitization so this should save a bunch of work for sites using get_term() too.

  • src/wp-includes/class-wp-term.php

    diff --git a/src/wp-includes/class-wp-term.php b/src/wp-includes/class-wp-term.php
    index 0f5631353e..f98c132017 100644
    a b final class WP_Term { 
    182182                }
    183183
    184184                $term_obj = new WP_Term( $_term );
    185                 $term_obj->filter( $term_obj->filter );
     185                if ( empty( $term_obj->filter ) || 'raw' !== $term_obj->filter ) {
     186                        $term_obj->filter( $term_obj->filter );
     187                }
    186188
    187189                return $term_obj;
    188190        }

@spacedmonkey commented on PR #4460:


10 months ago
#8

@peterwilsoncc I have created a PR based on your idea. https://github.com/WordPress/wordpress-develop/pull/5260. This solution multiple problems, of double sanitation. Please take a look.

#9 @spacedmonkey
10 months ago

  • Milestone 6.4 deleted
  • Resolution set to duplicate
  • Status changed from assigned to closed

Marking this as a duplicate as this issue will be fixed in #58329.

Note: See TracTickets for help on using tickets.