WordPress.org

Make WordPress Core

Opened 10 months ago

Last modified 3 months ago

#45486 reviewing task (blessed)

Docs: Correct the inline documentation for rest_sanitize_value_from_schema()

Reported by: birgire Owned by: SergeyBiryukov
Milestone: Future Release Priority: normal
Severity: normal Version: 4.7
Component: REST API Keywords: good-first-bug has-patch
Focuses: docs Cc:

Description

According to the inline documentation for rest_sanitize_value_from_schema() it can return true or WP_Error:

* @return true|WP_Error

See here:

https://core.trac.wordpress.org/browser/tags/4.9.8/src/wp-includes/rest-api.php#L1214

But it looks like it can return an array, boolean, string, int and float.

See e.g.

https://core.trac.wordpress.org/browser/tags/4.9.8/src/wp-includes/rest-api.php#L1236
https://core.trac.wordpress.org/browser/tags/4.9.8/src/wp-includes/rest-api.php#L1268
https://core.trac.wordpress.org/browser/tags/4.9.8/src/wp-includes/rest-api.php#L1291
https://core.trac.wordpress.org/browser/tags/4.9.8/src/wp-includes/rest-api.php#L1260
https://core.trac.wordpress.org/browser/tags/4.9.8/src/wp-includes/rest-api.php#L1264

It would also be informative to check if WP_Error can really be returned. It's not obvious from the function's code, but could be implicitly returned from the other function used there. But I haven't checked them all, so I'm not convinced yet :-) Looking at the unit tests in WP_Test_REST_Schema_Sanitization there are no tests for WP_Error.

So it seems that the inline documentation for @return is accidentally the same for rest_sanitize_value_from_schema() and rest_validate_value_from_schema():

https://core.trac.wordpress.org/browser/tags/4.9.8/src/wp-includes/rest-api.php#L1072

So the inline documentation needs to be corrected for rest_sanitize_value_from_schema().

Attachments (4)

patch-45486.diff (509 bytes) - added by Jean-David 10 months ago.
Fix inline documentation
45486.patch (539 bytes) - added by mukesh27 9 months ago.
Updated patch.
patch-45486.2.diff (1.1 KB) - added by Jean-David 9 months ago.
Not sure of the returns formatting.
454836.3.diff (946 bytes) - added by priyankkpatel 4 months ago.
Patch for wp-includes/rest-api.php

Download all attachments as: .zip

Change History (15)

@Jean-David
10 months ago

Fix inline documentation

#1 @Jean-David
10 months ago

First timer here, I hope I did everything right.

Actually, none of the code in the function returns a WP_Error; the rest_sanitize_boolean and sanitize_text_field don't.

@mukesh27
9 months ago

Updated patch.

#2 @birgire
9 months ago

Thanks for the patches @Jean-David and @mukesh27

I've also seen @return mixed ... being used in core.

According to

http://docs.phpdoc.org/guides/types.html

the primitive types in PHPDoc are: string, int, float, bool, array, resource, null, callable.

And regarding the mixed keyword it says:

The PHPDoc Standard also describes several keywords that are not native to PHP but are found to be often used or are representations of situations that are convenient to describe.

mixed
A value with this type can be literally anything; the author of the documentation is unable to predict which type it will be.

I'm not sure how strict the definition of mixed is in core, if it means that it must be "literally anything", i.e. all the primitive types and keywords, and that the author must be unable to predict the type?

We have the following on @return in the Core Handbook:

https://make.wordpress.org/core/handbook/best-practices/inline-documentation-standards/php

@return: Should contain all possible return types, and a description for each. Use a period at the end. 
Note: @return void should not be used outside of the default bundled themes.

So according to this it seems we are on the right path regarding the patches, but it seems we need to update it with a description for each type.

@mukesh27, @Jean-David What do you think?

#3 @SergeyBiryukov
9 months ago

  • Component changed from General to REST API
  • Milestone changed from Awaiting Review to 5.1
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

@Jean-David
9 months ago

Not sure of the returns formatting.

#4 @Jean-David
9 months ago

Actually, the last line of this function returns the original $value passed in first parameters.
@birgire In this case, is it considered mixed type as we don't know the value passed in parameters ?

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


8 months ago

#6 @pento
8 months ago

  • Type changed from defect (bug) to task (blessed)

Converting to task, as it's only a docs change.

#7 @pento
7 months ago

  • Milestone changed from 5.1 to 5.2

There's still some discussion on the right way to do this. Moving to 5.2.

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


5 months ago

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


5 months ago

#10 @JeffPaul
5 months ago

  • Milestone changed from 5.2 to Future Release

Per today's bugscrub, we're punting this as not enough time remains to get this into 5.2. Given the minimal movement during the 5.2 release cycle, I'm punting this to Future Release so that we consciously milestone this for a numbered release when we're ready to take it on.

@priyankkpatel
4 months ago

Patch for wp-includes/rest-api.php

#11 @splitti
3 months ago

  • Keywords has-patch added; needs-patch removed
Note: See TracTickets for help on using tickets.