Make WordPress Core

Opened 6 months ago

Closed 5 months ago

#59193 closed defect (bug) (fixed)

Remove misleading comment in class-wp-rest-blocks-controller.php

Reported by: kadamwhite's profile kadamwhite Owned by: kadamwhite's profile kadamwhite
Milestone: 6.3.2 Priority: low
Severity: normal Version: 6.3
Component: REST API Keywords: has-patch commit fixed-major
Focuses: rest-api Cc:

Description

In https://core.trac.wordpress.org/ticket/58657#comment:6 @david.binda calls out the confusing comment,

// Do not cache this schema because all properties are derived from parent controller.

@johnjamesjacoby flagged this comment as potentially misleading when we originally introduced schema caching in #47871. Upon reflection, in a comment I just left on #58657,

My [@kadamwhite's] 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 r56093 -- 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.

@audrasjb requested that we separate any comment changes out into a new ticket to allow #58657 to remain marked as fixed in 6.3.0, so I am creating this ticket to propose removing the comment as misleading. @TimothyBlynJacobs has agreed.

Change History (9)

This ticket was mentioned in PR #5074 on WordPress/wordpress-develop by addisonhardy.


6 months ago
#2

  • Keywords has-patch added

Remove misleading comment in class-wp-rest-blocks-controller.php

Trac ticket: [](https://core.trac.wordpress.org/ticket/59193)

#3 @kadamwhite
6 months ago

  • Keywords commit added

kadamwhite commented on PR #5074:


6 months ago
#4

The "welcome new contributors" action has failed to complete, so @addisonhardy please let me fill in in place of the bot!

Hi @addisonhardy! 👋

Thank you for your contribution to WordPress! 💖


It looks like this is your first pull request to wordpress-develop. Here are a few things to be aware of that may help you out!


No one monitors this repository for new pull requests. Pull requests must be attached to a Trac ticket to be considered for inclusion in WordPress Core. To attach a pull request to a Trac ticket, please include the ticket's full URL in your pull request description.


Pull requests are never merged on GitHub. The WordPress codebase continues to be managed through the SVN repository that this GitHub repository mirrors. Please feel free to open pull requests to work on any contribution you are making.


More information about how GitHub pull requests can be used to contribute to WordPress can be found in this blog post.


Please include automated tests. Including tests in your pull request is one way to help your patch be considered faster. To learn about WordPress' test suites, visit the Automated Testing page in the handbook.


If you have not had a chance, please review the Contribute with Code page in the WordPress Core Handbook.


The Developer Hub also documents the various coding standards that are followed:


Thank you,
The WordPress Project

#5 @kadamwhite
6 months ago

  • Owner set to kadamwhite
  • Resolution set to fixed
  • Status changed from new to closed

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.

#7 @audrasjb
6 months ago

Thanks for opening a new ticket to handle this follow-up issue!

#8 @kadamwhite
6 months ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening to backport to 6.3.x based on the milestone

#9 @audrasjb
5 months ago

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

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.