Make WordPress Core

Opened 3 weeks ago

Closed 13 days ago

Last modified 13 days ago

#61580 closed defect (bug) (fixed)

WP_REST_Templates_Controller::get_wp_templates_author_text_field() doesn't always return value

Reported by: antonvlasenko's profile antonvlasenko Owned by: hellofromtonya's profile hellofromTonya
Milestone: 6.7 Priority: normal
Severity: minor Version: 6.6
Component: REST API Keywords: has-patch commit
Focuses: rest-api Cc:

Description

The WP_REST_Templates_Controller::get_wp_templates_author_text_field() method sometimes fails to return a value. Its @return type is specified as a string, so it should consistently return a string.

Possible solutions include returning an empty string or throwing an exception if the $original_source does not equal one of the expected values.

Change History (10)

#1 follow-up: @debarghyabanerjee
2 weeks ago

Replying to antonvlasenko:

I have checked the core codebase, and if you check this function, you will find that it will always return a string, which will match atleast one of the expected values/cases.

WP_REST_Templates_Controller::get_wp_templates_original_source_field()
Last edited 2 weeks ago by debarghyabanerjee (previous) (diff)

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


2 weeks ago
#2

  • Keywords has-patch added

#3 in reply to: ↑ 1 @antonvlasenko
2 weeks ago

Replying to debarghyabanerjee:

I have checked the core codebase, and if you check this function, you will find that it will always return a string, which will be one of the expected values/cases.

WP_REST_Templates_Controller::get_wp_templates_original_source_field()

Thank you for your feedback. You are right. The :: get_wp_templates_author_text_field() method will always return a string if the ::get_wp_templates_original_source_field() method returns one of the expected values: theme, plugin, site, or user.

However, if ::get_wp_templates_original_source_field() method is changed to return unexpected values, it poses a risk.

I suggest that ::get_wp_templates_author_text_field() should return a default value instead of relying on ::get_wp_templates_original_source_field() to return predefined values.
I explored that option in https://github.com/WordPress/wordpress-develop/pull/7000.

#4 @hellofromTonya
2 weeks ago

  • Keywords changes-requested added
  • Milestone changed from Awaiting Review to 6.7

Hmm, this is interesting.

WP_REST_Templates_Controller:: get_wp_templates_original_source_field() always returns a string with one of the values used within the switch() within WP_REST_Templates_Controller::get_wp_templates_author_text_field().

But as @antonvlasenko noted, what if either method gets changed? To make the code more future-proof to avoid changes in one method affecting the other, a default of an empty string could be the graceful fall-through, e.g. return '';' at the end of the method.

Moving this into 6.7 for visibility.

I'll also comment on the PR with a suggested change.

@debarghyabanerjee commented on PR #7000:


2 weeks ago
#5

Hi @anton-vlasenko,

Did you check this method,

WP_REST_Templates_Controller::get_wp_templates_original_source_field()

This will always return a string which will match atleast one of the cases. Thanks :-)

@debarghyabanerjee commented on PR #7000:


2 weeks ago
#6

Hi @anton-vlasenko,

Did you check this method,

WP_REST_Templates_Controller::get_wp_templates_original_source_field()

This will always return a string which will match atleast one of the cases. Thanks :-)

#7 @hellofromTonya
13 days ago

  • Keywords commit added; changes-requested removed
  • Owner set to hellofromTonya
  • Status changed from new to reviewing

Patch: https://github.com/WordPress/wordpress-develop/pull/7000

Marking this ready for commit and self-assigning for the commit.

#8 @hellofromTonya
13 days ago

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

In 58705:

REST API: Ensure string returned in WP_REST_Templates_Controller::get_wp_templates_author_text_field().

Adds a fail-safe to return an empty string should the switch ever fall through without returning.

Currently, WP_REST_Templates_Controller::get_wp_templates_author_text_field() is tightly coupled to WP_REST_Templates_Controller::get_wp_templates_original_source_field(). However, if the $original_source values change in either method, but not both, it is possible a void or null will be returned, rather than a string.

Follow-up to [57366].

Props antonvlasenko, hellofromTonya, debarghyabanerjee.
Fixes #61580.

@hellofromTonya commented on PR #7000:


13 days ago
#9

Committed via https://core.trac.wordpress.org/changeset/58705. Thanks everyone for your contributions

@antonvlasenko commented on PR #7000:


13 days ago
#10

Committed via https://core.trac.wordpress.org/changeset/58705. Thanks everyone for your contributions

Thank you!

Note: See TracTickets for help on using tickets.