Double sanitization in get_term function

[spacedmonkey]

In the get_term function, the method filter is called. However, in most cases WP_Term::get_instance is called in get_term which also calls filter. This results in the term being sanitization twice. This filter method can be extensive and can be called thousands of times on an average page load.

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

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

[mukesh27]

Thanks @spacedmonkey for the ticket and PR.

I reviewed the latest PR and left some nit-pick feedback. The major issue is get_term filter is not working after the new code changes.

Can you define not working? Steps to replicate!

@spacedmonkey Could you please check the unit test that are failing.

[mukesh27]

Adding this to the 6.4 milestone for consideration.

[spacedmonkey]

Awaiting feedback from @costdev & @mukesh27

[oglekler]

I see the reason for sanitation due to the possibility that data stored in cache can be modified by something else. Please consider the solution when double sanitization can be avoided but still covering all returned data.

We are getting closer to Beta 1, and I am tempted to move this to the next milestone.

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

This PR replaces ​https://github.com/WordPress/wordpress-develop/pull/4874. Basically, only run the filter method once in get_term. Run if the object has been filtered or the filter parameter does match the one passed in the function.

This fix would also fix the issues found in https://core.trac.wordpress.org/ticket/58327

This PR has become overly complex and hard to follow. I don't think it cleanly sorted the problem at hand.

For that reason, I have started afresh with ​https://github.com/WordPress/wordpress-develop/pull/5260. I closing this PR out.
@mukeshpanchal27 @joemcgill @OlaIola

[spacedmonkey]

I have created a simpler PR that is ready for review. ​https://github.com/WordPress/wordpress-develop/pull/5260

[spacedmonkey]

Blackfire profile - ​https://blackfire.io/profiles/compare/79ce730a-a7f6-4f38-841f-a7481b290c45/graph - 2020 theme.

Benchmark data. 1000 runs - local docker - PhP 7.4

	Trunk - 2020 theme	PR - 2020 theme	Trunk - 2023 theme	PR - 2023 theme
Response Time (median)	121.28	115.35	77.56	77.65
wp-load-alloptions-query (median)	0.62	0.61	0.59	0.6
wp-before-template (median)	21.42	20.36	32.2	32.31
wp-template (median)	90.36	86.02	40.15	40.3
wp-total (median)	112.24	106.51	72.54	72.81

There are clear benefits for classic themes. CC @flixos90 @joemcgill

Thanks @spacedmonkey for the PR. Left some unit test related feedbacks.

Actioned feedback.

I like this approach much better. The tests that you've added already pass without this change in trunk, so it would be nice to add a test that demonstrates the exact problem you're trying to fix. I've been trying to work something up, but haven't had success yet. Will keep messing with it, but you may beat me to it.

@joemcgill The fact that the test works in trunk was the point. To see no breakage. I have added one more unit test, that proved that sanativation is only run once. Can you take another look at this PR.

Thanks for the updates @spacedmonkey! Just a few final notes 🙂

Thanks @costdev Feedback actioned. Once I get an approval from you, I will commit.

[spacedmonkey]

#58327 was marked as a duplicate.

[spacedmonkey]

In 56650:

Taxonomy: Stop double sanitization in get_term function.

In the get_term function, the filter method is invoked on the WP_Term object, which subsequently triggers the execution of sanitize_term. The filter method is also executed within WP_Term::get_instance.

A common scenario when calling the get_term function is to invoke the function with an integer ID for the term and a filter set to "raw." This results in a call to WP_Term::get_instance. However, since both get_term and WP_Term::get_instance invoke the filter method, it leads to double sanitization of the term.

Considering that get_term may be called thousands of times on a page, especially when priming a large number of terms into memory, this redundancy can result in thousands of unnecessary calls to sanitize_term. Performing the same sanitization operation twice with the same parameters is wasteful and detrimental to performance.

To address this issue, the code has been updated to execute the filter method only when the filter parameter does not match or when changes have been made to the term object within the get_term hook. This optimization ensures that the filter is applied selectively, mitigating performance concerns and avoiding unnecessary sanitization calls.

Props spacedmonkey, flixos90, costdev, mukesh27, joemcgill, oglekler, peterwilsoncc.
Fixes #58329.

Commited ​https://github.com/WordPress/wordpress-develop/commit/c1247f6a278bd6a5fec799964177743b5d26a2aa

Committed ​https://github.com/WordPress/wordpress-develop/commit/c1247f6a278bd6a5fec799964177743b5d26a2aa

[codente]

I'm reviewing tickets with @webcommsat for dev notes, if there is not already a dev note started for this, could someone use the information in https://core.trac.wordpress.org/ticket/58329#comment:22 to draft one?

The dev note for review and publishing purposes is being tracked at this [link]​https://github.com/WordPress/Documentation-Issue-Tracker/issues/1182[].
Thanks for your help.

We have noted that performance have clustered a number of tickets together for the dev notes.
We will cross-reference the issues to reflect this.

[webcommsat]

The following has been added to the Field Guide performance section based on the commit message in comment 22. We could not find this changed reference in the other mixed dev notes relating to performance. Please add any future dev note on this to the [documentation tracking ticket]​https://github.com/WordPress/Documentation-Issue-Tracker/issues/1182] so it can be updated in the Field Guide too. Thank you.

"Taxonomy: The double sanitization in get_term function has been stopped. This will prevent the unnecessary calls to sanitize_term which was detrimental to performance. #58329.
"

I have removed 'has-patch' keyword for housekeeping. I will leave the 'needs-dev-note' keyword for now.

[joemcgill]

This will be included in the Field Guide in the performance section.

[Cybr]

Previous: #50568