Make WordPress Core

Opened 3 years ago

Closed 22 months ago

#53946 closed enhancement (fixed)

Improve return types for sanitizing/escaping functions

Reported by: malthert's profile malthert Owned by: audrasjb's profile audrasjb
Milestone: 6.1 Priority: normal
Severity: normal Version:
Component: General Keywords: good-first-bug has-patch commit
Focuses: docs Cc:

Description

The return type of functions where param is string|array, specificall escaping and sanitizing functions, should be set more specific (= that the function returns the same type the passed $arg has).

This means changing:

<?php
     * @param string|array $value String or array of data to unslash.
     * @return string|array Unslashed $value.
     */
    function wp_unslash($value)

to this:

     * @param string|array $value String or array of data to unslash.
     * @return ($value is array ? array : string) Unslashed $value.
     */
    function wp_unslash($value)

This ensures that static analysis tools correctly infer the type, as well as make it clear to developers that the variable type is not changed by this function.

This is filed as bug, since it is a (more and more) deprecated way of declaring phpdocs, thus should be updated.

Attachments (1)

53946.diff (1.2 KB) - added by byohann6 23 months ago.
Improve description for return types

Download all attachments as: .zip

Change History (17)

#1 in reply to: ↑ description @malthert
3 years ago

Specifically, this is for:
public function _escape
public function escape
function esc_sql
function wp_slash
function wp_unslash

#2 @SergeyBiryukov
3 years ago

  • Component changed from General to Security
  • Focuses docs added

#3 @johnbillion
3 years ago

  • Keywords reporter-feedback added
  • Type changed from defect (bug) to enhancement
  • Version trunk deleted

Thanks for the report @malthert . Can you point us to the documentation for this syntax please?

#4 @malthert
3 years ago

The documentation for phpdocs is extremely fragmented, so I am not really sure where this is from.

I just found https://github.com/vimeo/psalm/issues/4629#issuecomment-730570590 and this seems to work for psalm and phpstan (besides the fact, that this is easily clearer than what is there now)

#5 follow-up: @johnbillion
3 years ago

  • Keywords reporter-feedback removed

Thanks for the link!

This syntax appears to be specific to Psalm and it isn't supported by PHPStan (although it's on the roadmap), nor is it supported by phpDocumentor, so I'm inclined to say that as much as it would be great to have machine-readable conditional return types I don't think we should introduce a syntax which breaks existing tooling that isn't Psalm.

We could use the Psalm-specific @psalm-return tag but that would imply some level of support for Psalm in core which there isn't. There's some discussion on whether we should introduce PHPStan-specific syntax and configuration in #52217 and the same would apply to Psalm.

I think this is a wontfix, unfortunately, but I would welcome a patch which improves the descriptions of these functions in order to better explain the possible return types.

#6 in reply to: ↑ 5 @SergeyBiryukov
3 years ago

Replying to johnbillion:

I think this is a wontfix, unfortunately, but I would welcome a patch which improves the descriptions of these functions in order to better explain the possible return types.

I've found some similar instances mentioning "same type":

  • sanitize_bookmark():
     * @param stdClass|array $bookmark Bookmark row.
     * @param string         $context  Optional. How to filter the fields. Default 'display'.
     * @return stdClass|array Same type as $bookmark but with fields sanitized.
     */
    function sanitize_bookmark( $bookmark, $context = 'display' ) {
    
  • sanitize_category():
     * @param object|array $category Category data.
     * @param string       $context  Optional. Default 'display'.
     * @return object|array Same type as $category with sanitized data for safe use.
     */
    function sanitize_category( $category, $context = 'display' ) {
    
  • sanitize_category_field()
     * @param string $field   Category key to sanitize.
     * @param mixed  $value   Category value to sanitize.
     * @param int    $cat_id  Category ID.
     * @param string $context What filter to use, 'raw', 'display', etc.
     * @return mixed Same type as $value after $value has been sanitized.
     */
    function sanitize_category_field( $field, $value, $cat_id, $context ) {
    
  • sanitize_user_object():
     * @param object|array $user    The user object or array.
     * @param string       $context Optional. How to sanitize user fields. Default 'display'.
     * @return object|array The now sanitized user object or array (will be the same type as $user).
     */
    function sanitize_user_object($user, $context = 'display') {
    
  • sanitize_post():
     * @param object|WP_Post|array $post    The post object or array
     * @param string               $context Optional. How to sanitize post fields.
     *                                      Accepts 'raw', 'edit', 'db', 'display',
     *                                      'attribute', or 'js'. Default 'display'.
     * @return object|WP_Post|array The now sanitized post object or array (will be the
     *                              same type as `$post`).
     */
    function sanitize_post( $post, $context = 'display' ) {
    
  • wpdb::_escape()
     * @param string|array $data Data to escape.
     * @return string|array Escaped data, in the same type as supplied.
     */
    public function _escape( $data ) {
    

So I think something like this would work here as well:

 * @param string|array $data Unescaped data.
 * @return string|array Escaped data, in the same type as supplied.
 */
function esc_sql( $data ) {

...
 * @param string|array $value String or array of data to slash.
 * @return string|array Slashed $value, in the same type as supplied.
 */
function wp_slash( $value ) {

...
* @param string|array $value String or array of data to unslash.
* @return string|array Unslashed $value, in the same type as supplied.
*/
function wp_unslash( $value )

#7 @malthert
2 years ago

  • Resolution set to wontfix
  • Status changed from new to closed

#8 @desrosj
2 years ago

  • Component changed from Security to General
  • Milestone changed from Awaiting Review to 6.1

Reopening to address the findings in ticket:53946#comment:5.

#9 @desrosj
23 months ago

  • Resolution wontfix deleted
  • Status changed from closed to reopened

#10 @desrosj
23 months ago

  • Keywords needs-patch good-first-bug added

Marking as good-first-bug.

#11 follow-up: @malthert
23 months ago

@desrosj can you explain me why you reopened? There is no way to fix this without using (currently) psalm-specific syntax as explained by john. This means this is wontfix, until this syntax has more widespread support.
Or am I missing something?

#12 in reply to: ↑ 11 @SergeyBiryukov
23 months ago

Replying to malthert:

can you explain me why you reopened?

We could improve the descriptions to better explain the possible return types, as suggested in comment:6.

@byohann6
23 months ago

Improve description for return types

#13 @byohann6
23 months ago

I improved the descriptions for the possible return types as @SergeyBiryukov suggested in comment:6

#14 @byohann6
23 months ago

  • Keywords has-patch added; needs-patch removed

#15 @audrasjb
22 months ago

  • Keywords commit added

We need a period at the end of the @return declaration of esc_sql but otherwise, the patch looks good to me. I'll fix it during commit.

#16 @audrasjb
22 months ago

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

In 53775:

Docs: Refine @return docblock mentions for esc_sql(), wp_slash() and wp_unslash().

Props malthert, SergeyBiryukov, johnbillion, desrosj, byohann6.
Fixes #53946.
See #55646.

Note: See TracTickets for help on using tickets.