Make WordPress Core

Opened 10 months ago

Closed 5 months ago

Last modified 4 months ago

#58329 closed enhancement (fixed)

Double sanitization in get_term function

Reported by: spacedmonkey's profile spacedmonkey Owned by: spacedmonkey's profile spacedmonkey
Milestone: 6.4 Priority: normal
Severity: normal Version:
Component: Taxonomy Keywords:
Focuses: performance Cc:

Description

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.

Change History (30)

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


10 months ago
#1

  • Keywords has-patch added

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


7 months ago
#2

  • Keywords has-unit-tests added

#3 @mukesh27
7 months ago

  • Keywords changes-requested added

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.

@spacedmonkey commented on PR #4874:


7 months ago
#4

Can you define not working? Steps to replicate!

@mukesh27 commented on PR #4874:


7 months ago
#5

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

#6 @mukesh27
7 months ago

  • Milestone changed from Awaiting Review to 6.4

Adding this to the 6.4 milestone for consideration.

#7 @spacedmonkey
6 months ago

  • Keywords dev-feedback added; changes-requested removed

#8 @spacedmonkey
6 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.


6 months ago

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


6 months ago

#11 @spacedmonkey
6 months ago

Awaiting feedback from @costdev & @mukesh27

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


6 months ago

#13 @oglekler
5 months ago

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.

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


5 months ago
#14

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

@spacedmonkey commented on PR #4874:


5 months ago
#15

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

#16 @spacedmonkey
5 months ago

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

#17 @spacedmonkey
5 months ago

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

@spacedmonkey commented on PR #5260:


5 months ago
#18

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

Actioned feedback.

@spacedmonkey commented on PR #5260:


5 months ago
#19

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.

@spacedmonkey commented on PR #5260:


5 months ago
#20

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

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

#21 @spacedmonkey
5 months ago

#58327 was marked as a duplicate.

#22 @spacedmonkey
5 months ago

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

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.

#25 @spacedmonkey
5 months ago

  • Keywords needs-dev-note added; has-unit-tests dev-feedback removed

#26 @codente
5 months ago

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.

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


4 months ago

#28 @webcommsat
4 months ago

  • Keywords has-patch removed

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.

#29 @joemcgill
4 months ago

  • Keywords needs-dev-note removed

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

#30 @Cybr
4 months ago

Previous: #50568

Note: See TracTickets for help on using tickets.