Make WordPress Core

Opened 8 years ago

Last modified 3 years ago

#38073 assigned enhancement

Deprecate and replace wp_reset_vars()

Reported by: swissspidy's profile swissspidy Owned by: davideferre's profile davideferre
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 4.9
Component: General Keywords: good-first-bug dev-feedback has-patch needs-refresh
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 (5)

38073.patch (490.0 KB) - added by davideferre 7 years ago.
38073.2.patch (12.7 KB) - added by davideferre 7 years ago.
Fixes previous submitted patch.
38073.3.patch (13.1 KB) - added by killua99 7 years ago.
wp_reset_vars_results.txt (5.5 KB) - added by voldemortensen 7 years ago.
38073.4.diff (15.5 KB) - added by killua99 5 years ago.
Refresh latest trunk and moving deprecated function to the right place.

Download all attachments as: .zip

Change History (30)

#1 @swissspidy
8 years ago

  • Description modified (diff)

#2 follow-ups: @MattyRob
8 years ago

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

#3 in reply to: ↑ 2 @swissspidy
8 years 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
8 years 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
7 years ago

  • Keywords needs-patch good-first-bug added

See also #22400.

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


7 years ago

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

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

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

Fixes previous submitted patch.

#17 @swissspidy
7 years 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
7 years 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 years ago

@killua99
7 years ago

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

#22 @danieltj
7 years 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
6 years ago

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

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

@killua99
5 years ago

Refresh latest trunk and moving deprecated function to the right place.

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


5 years ago

#25 @swissspidy
3 years ago

  • Keywords needs-refresh added; reporter-feedback removed
Note: See TracTickets for help on using tickets.