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

[spacedmonkey]

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

Trac ticket: https://core.trac.wordpress.org/ticket/58657#ticket

[spacedmonkey]

Gutenberg PR - ​https://github.com/WordPress/gutenberg/pull/52045

[kadamwhite]

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].

[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 )

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 )

[kirasong]

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

[audrasjb]

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.

[TimothyBlynJacobs]

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

[audrasjb]

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

[kadamwhite]

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.

Committed in https://core.trac.wordpress.org/changeset/56093

[TimothyBlynJacobs]

Yeah it seems safe enough to me.

[kadamwhite]

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.

[kadamwhite]

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.

[audrasjb]

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.