WordPress.org

Make WordPress Core

Opened 21 months ago

Last modified 4 months ago

#38073 assigned enhancement

Deprecate and replace wp_reset_vars()

Reported by: swissspidy Owned by: davideferre
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 4.9
Component: General Keywords: good-first-bug dev-feedback has-patch reporter-feedback
Focuses: administration Cc:

Description (last modified by swissspidy)

wp_reset_vars() sets global variables based on $_POST and $_GET values. The function is used around 20 times in core and in my opinion this should be zero. Even better, the function should be deprecated.

Why?

First of all, it's easy to shoot yourself in the foot if you forget to properly sanitize the input value. Second, globals set by wp_reset_vars() aren't explicitly globalized in the files / functions using it. You might stumble upon code like this:

<?php
wp_reset_vars( array( 'foo', 'bar' ) );
// 100 lines further down…

// Where do these come from?!
echo $foo;
echo $bar;

And of course using globals is bad. It's not testable and should be avoided if possible. Sanitized $_GET / $_POST values should be used directly instead.

Related: #33837, #37699

Attachments (4)

38073.patch (490.0 KB) - added by davideferre 11 months ago.
38073.2.patch (12.7 KB) - added by davideferre 11 months ago.
Fixes previous submitted patch.
38073.3.patch (13.1 KB) - added by killua99 10 months ago.
wp_reset_vars_results.txt (5.5 KB) - added by voldemortensen 10 months ago.

Download all attachments as: .zip

Change History (27)

#1 @swissspidy
21 months ago

  • Description modified (diff)

#2 follow-ups: @MattyRob
21 months ago

Any estimate of how many plugins make a call in this function?

#3 in reply to: ↑ 2 @swissspidy
21 months ago

Replying to MattyRob:

Any estimate of how many plugins make a call in this function?

We could scan the plugin directory if necessary.

#4 @MattyRob
21 months ago

I suspect it's not many but maybe worth a check to avoid problems if it is widely used or used by very popular plugins.

#5 @swissspidy
16 months ago

  • Keywords needs-patch good-first-bug added

See also #22400.

This ticket was mentioned in Slack in #forums by lukecavanagh. View the logs.


16 months ago

#7 @patilswapnilv
14 months ago

Hello everyone,

I think this reference helps https://developer.wordpress.org/reference/functions/wp_reset_vars/ I don't know who updated this is, but it says this is used in 5 places, within the core.

I agree that since it works with global values, and carries a good amount of risk when used.

Are we sure we want to depreciate this? Is there anything else we want to consider/ check before we do this?

Thanks,

#8 @swissspidy
14 months ago

The function reference is created automatically by parsing the source code.

Note that wp_reset_vars() doesn't need to be deprecated. First and foremost, I want to eliminate its usage in core.

#9 @jfabot
12 months ago

I would love to write a patch to solve this issue by rewriting the way to fetch the vars in a save way. Doing it this way won't remove the wp_reset_vars function, but will make it obsolete and improve the code. Is this an acceptable approach?

#10 @swissspidy
12 months ago

We don't need to remove the wp_reset_vars() function itself. We just need to remove its usage. See [38745] for an example.

#11 @jfabot
12 months ago

I lookup the ticket and read the ideas that have passed. Furthermore, I will try to write a fitting patch for this to share.

#12 follow-up: @weijland
11 months ago

Since all wp_reset_vars does is (re)assigning the value of a POST or GET request to a variable if it exists, isn't it an idea to write a new function which does exactly the same, but without the use of globals? Something like this:

$action = wp_assign_request_var( 'action' );
function wp_assign_request_var( $var ) {
   if ( empty( $_POST[ $var ] ) ) {
        if ( empty( $_GET[ $var ] ) ) {
             return '';
        } else {
             return $_GET[ $var ];
        }
   } else {
        return $_POST[ $var ];
   }
}

I figure that to fix this issue, a new function is needed, otherwise you have to apply the same fix to every file that makes use of wp_reset_vars(), which isn't DRY of course. I suggest placing the new function in wp-admin/includes/misc.php, since that is the place where the original function is located.

Last edited 11 months ago by weijland (previous) (diff)

#13 in reply to: ↑ 2 @weijland
11 months ago

Replying to MattyRob:

Any estimate of how many plugins make a call in this function?

I did a quick search and it seems that it is used 20 times by the core.

#14 in reply to: ↑ 12 @jfabot
11 months ago

That was indeed the thing I was thinking about.

Replying to weijland:

Since all wp_reset_vars does is (re)assigning the value of a POST or GET request to a variable if it exists, isn't it an idea to write a new function which does exactly the same, but without the use of globals? Something like this:

$action = wp_assign_request_var( 'action' );
function wp_assign_request_var( $var ) {
   if ( empty( $_POST[ $var ] ) ) {
        if ( empty( $_GET[ $var ] ) ) {
             return '';
        } else {
             return $_GET[ $var ];
        }
   } else {
        return $_POST[ $var ];
   }
}

I figure that to fix this issue, a new function is needed, otherwise you have to apply the same fix to every file that makes use of wp_reset_vars(), which isn't DRY of course. I suggest placing the new function in wp-admin/includes/misc.php, since that is the place where the original function is located.

#15 @davideferre
11 months ago

If it's ok, I can work on this ticket implementing @weijland proposal and replacing all occurences of wp_reset_vars in WordPress core.

@davideferre
11 months ago

#16 @davideferre
11 months ago

I've see now that my editor replaced tab with spaces, I will fix this and re-submit a new patch with tabs instead of spaces.

@davideferre
11 months ago

Fixes previous submitted patch.

#17 @swissspidy
11 months ago

  • Keywords dev-feedback has-patch added; needs-patch removed

Since all wp_reset_vars does is (re)assigning the value of a POST or GET request to a variable if it exists, isn't it an idea to write a new function which does exactly the same, but without the use of globals?

I don't really the see the value in wp_assign_request_var() because you could probably just as well use $_REQUEST.

However, in pretty much all places where wp_reset_vars() is used, the variables actually only come from a single source, either only $_GET or only $_POST. So I think in our code we should be specific enough to make this clear for developers to prevent confusion and bugs. Something like isset( $_GET['xy'] ) ? sanitization_function( $_GET['xy'] ) : 'default' is much clearer than having wp_assign_request_var() all over the place.

#18 @davideferre
11 months ago

I'm new in contributing to WordPress core and this is my very first ticket. That said, for what I've seen while replacing wp_reset_vars occurrencies, there's no references on where the variable came from ($_POST or $_GET) in the functions where is used and sometimes what can be his default value. Do you think it's easy to find this informations?

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


10 months ago

@killua99
10 months ago

#20 @killua99
10 months ago

  • Keywords reporter-feedback added
  • Version set to trunk

In this patch I follow @swissspidy suggestions about simple stop using wp_reset_vars() and refactoring for sanitize_text_field() instead. Notice I do not alter any CodeStandard to make easy to review the patch. Also I did add a message to the function with a @deprecated message, I really don't know which version that function could be removed so I guessed WordPress 5.

#21 @voldemortensen
10 months ago

For informational purposes, attached are the results of a scan of the plugin directory. 87 plugins are using wp_reset_vars at least once, several using it more than that. Only a handful of the plugins are popular.

Last edited 10 months ago by voldemortensen (previous) (diff)

#22 @danieltj
9 months ago

  • Summary changed from Goodbye wp_reset_vars() to Deprecate and replace wp_reset_vars()

Changed the title so it's more clear what the intent is.

#23 @DrewAPicture
4 months ago

  • Owner set to davideferre
  • Status changed from new to assigned

Assigning to mark the good-first-bug as "claimed".

Note: See TracTickets for help on using tickets.