Make WordPress Core

Opened 11 months ago

Closed 9 months ago

Last modified 8 months ago

#58657 closed defect (bug) (fixed)

Add in missing caching to `get_item_schema` method to new endpoint

Reported by: spacedmonkey's profile spacedmonkey Owned by: spacedmonkey's profile spacedmonkey
Milestone: 6.3 Priority: normal
Severity: normal Version: 5.0
Component: REST API Keywords:
Focuses: performance Cc:

Description

In [45811] the schema was cached in a property of a the class. This was done for performance reasons. However new endpoints I have been added and this caching was not added. Reviewing code, the following endpoints are missing the caching.

WP_REST_Block_Pattern_Categories_Controller
WP_REST_Menus_Controller
WP_REST_Menu_Items_Controller
WP_REST_Blocks_Controller
WP_REST_Block_Pattern_Categories_Controller

Change History (17)

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


11 months ago
#1

  • Keywords has-patch added; needs-patch removed

#3 @kadamwhite
11 months ago

  • Keywords commit added
  • Owner set to kadamwhite
  • Status changed from new to accepted

#4 @kadamwhite
11 months ago

  • Owner changed from kadamwhite to spacedmonkey
  • Status changed from accepted to assigned

#5 @kadamwhite
11 months ago

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

In 56093:

REST API: Cache schema in block pattern and menu item endpoints.

Performance improvement to add schema caching to pattern and menu item REST endpoints, so identical schema object are not needlessly regenerated.

Props spacedmonkey.
Fixes #58657. See [45811].

#6 follow-up: @david.binda
10 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reviewing the r56093 , it's adding a caching to the WP_REST_Blocks_Controller::get_item_schema right above a comment which says: // Do not cache this schema because all properties are derived from parent controller. ( see https://core.trac.wordpress.org/browser/trunk/src/wp-includes/rest-api/endpoints/class-wp-rest-blocks-controller.php?rev=56093#L75 )

I haven't been able to figure out whether the caching was omitted here as redundant or whether there are any functional limits to the caching.

IMHO, either the comment should be removed as misleading, or the caching should not have be added there. There might a similar case in WP_REST_Menus_Controller::get_item_schema which is also deriving the properties from parent controller ( see https://core.trac.wordpress.org/browser/trunk/src/wp-includes/rest-api/endpoints/class-wp-rest-menus-controller.php?rev=56093#L530 )

#7 in reply to: ↑ 6 @kirasong
10 months ago

Replying to david.binda:

Reviewing the r56093 , it's adding a caching to the WP_REST_Blocks_Controller::get_item_schema right above a comment which says: // Do not cache this schema because all properties are derived from parent controller. ( see https://core.trac.wordpress.org/browser/trunk/src/wp-includes/rest-api/endpoints/class-wp-rest-blocks-controller.php?rev=56093#L75 )

Thanks for bringing this up!

I'm also not entirely clear on the consequences, so pinging folks with props + comments on that particular line for feedback.

cc: @spacedmonkey @kadamwhite @joehoyle @TimothyBlynJacobs @johnjamesjacoby

Edit to add:
See #47871

Last edited 10 months ago by kirasong (previous) (diff)

#8 @audrasjb
10 months ago

  • Keywords has-patch commit removed
  • Milestone changed from 6.3 to 6.3.1

As we're very close to 6.3 final release, let's move this ticket to milestone 6.3.1.
By the way, it would probably be better to open a follow-up ticket to address this issue so we can keep this one fixed in milestone 6.3.

#9 @TimothyBlynJacobs
10 months ago

Agreed, no reason this can't be done in 6.3.1.

#10 @audrasjb
9 months ago

  • Milestone changed from 6.3.1 to 6.3.2

Moving to milestone 6.3.2 as WP 6.3.1 is going to be released in the next few days.

#11 @kadamwhite
9 months ago

My memory is that we felt at the time like the caching was unnecessary because it would already be in place at a higher level, but I don't personally see any downside to having added it -- it feels like a micro-optimization with uncertain payoff, which is not worth having one or two controllers work different from all the others. @johnjamesjacoby called me out on the comment at the time and I now agree that the uncertainty it introduces is not good.

@TimothyBlynJacobs My instinct is to remove the comment and leave the caching in place as-is, but I would like a second opinion.

This ticket was mentioned in Slack in #core-restapi by kadamwhite. View the logs.


9 months ago

#14 @TimothyBlynJacobs
9 months ago

Yeah it seems safe enough to me.

#15 @kadamwhite
9 months ago

  • Milestone changed from 6.3.2 to 6.3
  • Resolution set to fixed
  • Status changed from reopened to closed

Moving this ticket back to 6.3 and re-closing per @audrasjb's request that we separate out the comment removal into a new ticket. Created #59193 to track that removal.

#16 @kadamwhite
9 months ago

In 56459:

REST API: Remove misleading comment in WP_REST_Blocks_Controller->get_item_schema.

In r56093 schema caching was added above a comment instructing developers not to cache that controller's schema. However, there is no obvious penalty for re-caching schema that is partially derived from a parent.

Caching schema in the same way in every controller is beneficial consistency, and discussion at WCUS2023 contributor day concluded we could remove this comment.

Props ahardyjpl, davidbinda, johnjamesjacoby, TimothyBlynJacobs.

Fixes #59193. See #58657.

#17 @audrasjb
8 months ago

In 56781:

REST API: Remove misleading comment in WP_REST_Blocks_Controller->get_item_schema.

In r56093 schema caching was added above a comment instructing developers not to cache that controller's schema. However, there is no obvious penalty for re-caching
schema that is partially derived from a parent.

Caching schema in the same way in every controller is beneficial consistency, and discussion at WCUS2023 contributor day concluded we could remove this comment.

Props ahardyjpl, davidbinda, johnjamesjacoby, TimothyBlynJacobs, kadamwhite.
Fixes #59193.
See #58657.

Note: See TracTickets for help on using tickets.