Improve return types for sanitizing/escaping functions

[malthert]

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.

[malthert]

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

[johnbillion]

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

[malthert]

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)

[johnbillion]

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.

[SergeyBiryukov]

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 )

[desrosj]

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

[desrosj]

Marking as good-first-bug.

[malthert]

@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?

[SergeyBiryukov]

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]

Improve description for return types

[byohann6]

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

[audrasjb]

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.

[audrasjb]

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.