WordPress.org

Make WordPress Core

Opened 18 months ago

Last modified 2 weeks 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 8 months ago.
38073.2.patch (12.7 KB) - added by davideferre 8 months ago.
Fixes previous submitted patch.
38073.3.patch (13.1 KB) - added by killua99 7 months ago.
wp_reset_vars_results.txt (5.5 KB) - added by voldemortensen 7 months ago.

Download all attachments as: .zip

Change History (27)

#1 @swissspidy
18 months ago

  • Description modified (diff)

#2 follow-ups: @MattyRob
18 months ago

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

#3 in reply to: ↑ 2 @swissspidy
17 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
17 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
13 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.


13 months ago

#7 @patilswapnilv
11 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
11 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
8 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
8 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
8 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
8 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 8 months ago by weijland (previous) (diff)

#13 in reply to: ↑ 2 @weijland
8 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
8 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
8 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
8 months ago

#16 @davideferre
8 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
8 months ago

Fixes previous submitted patch.

#17 @swissspidy
8 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
8 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.


7 months ago

@killua99
7 months ago

#20 @killua99
7 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
7 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 7 months ago by voldemortensen (previous) (diff)

#22 @danieltj
6 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
2 weeks 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.