add_query_arg() should encode values

[Viper007Bond]

One (or me at least) would expect that the result of

add_query_arg( 'foobar', 'this&that', 'index.php' )

would be

index.php?foobar=this%26that

since the whole purpose of the function is to build a URL. However the actual result is

index.php?foobar=this&that

You're asking to the function to create a URL in which foobar is this&that but instead it creates a URL in which foobar is set to only this. You shouldn't have to pre-encode values -- the function should take care of it for you.

The function to "blame" for this is our build_query() which for some reason does not encode by default.

Semi-related: #16943

[nacin]

I imagine this might break some things. Probably a lot of things.

[Viper007Bond]

Replying to nacin:

I imagine this might break some things. Probably a lot of things.

Specifically where things are already being pre-encoded using (raw)urlencode()? I thought of that -- we need a rawurlencode() that doesn't re-encode or something.

[azaozz]

Perhaps better to leave the urlencoding to the function that uses add_query_arg() as having & or = in the name or value is uncommon. Example:

add_query_arg( '_wp_http_referer', urlencode( stripslashes($_SERVER['REQUEST_URI']) ) )

[dd32]

I've run into this many times, caused a few core bugs due to me expecting it to encode too.
Unfortunately, as pointed out, changing this would lead to pretty much every case of people who are using it correctly breaking..

[Viper007Bond]

Then perhaps a new arg for the function could be added? I end up doing this before I call it usually and it's annoying to have to do.

$args = array_map( 'rawurlencode', $args );

[SergeyBiryukov]

Related: #18086

[kurtpayne]

Optionally encode parameters to add_query_arg

[kurtpayne]

Unit test for add_query_arg and remove_query_arg

[kurtpayne]

17923.patch​ doesn't affect current unit tests. The same tests pass / fail before and after the patch. Light testing of the admin / front-end show no change in behavior.

[Viper007Bond]

Sweet. :)

Rather than doing a foreach() though, array_map() is easier (see comment 6).

[kurtpayne]

s/foreach/array_map/

[kurtpayne]

Replying to Viper007Bond:

Sweet. :)

Rather than doing a foreach() though, array_map() is easier (see comment 6).

TIL that array_map preserves key/value relationships.

[Viper007Bond]

Just noticed something else -- why the change in methods between the array vs strings?

For arrays, you leave it where the value gets assigned to a variable and then check to see if encoding is enabled and if so, you then encode the value.

However for strings, you do both at once.

In short, wouldn't this be better?

(first line already exists in core, second and third lines would be new)

$qs[func_get_arg( 0 )] = func_get_arg( 1 );
if ( true === @func_get_arg( 3 ) )
	$qs[func_get_arg( 0 )] = rawurlencode( $qs[func_get_arg( 0 )] );

Wow, I am sure being nitpicky today. Sorry. lol

[kurtpayne]

Replying to Viper007Bond:

Just noticed something else -- why the change in methods between the array vs strings?

For arrays, you leave it where the value gets assigned to a variable and then check to see if encoding is enabled and if so, you then encode the value.

However for strings, you do both at once.

In short, wouldn't this be better?

(first line already exists in core, second and third lines would be new)

$qs[func_get_arg( 0 )] = func_get_arg( 1 );
if ( true === @func_get_arg( 3 ) )
	$qs[func_get_arg( 0 )] = rawurlencode( $qs[func_get_arg( 0 )] );

Wow, I am sure being nitpicky today. Sorry. lol

Very valid question, actually. The function should only encode new arguments. Anything that's currently in the URL may already be encoded and should be left alone. If the $qs array is encoded after it's split, things may be re-encoded and yield funny results. Example:

$url = '/wordpress/';
$url = add_query_arg('a', 'b&c', $url, true);  # /wordpress/?a=b%26c
$url = add_query_arg('a', 'b&c', $url, true);  # /wordpress/?a=b%2526c

As you may suspect, I found this out the hard way.

[Viper007Bond]

Check out my code again. ;) I wasn't suggesting running the whole $qs array through rawurlencode(), just the new arg.

I was suggesting a code simplification by setting the element in the array regardless and then if encoding is enabled, going back and encoding it, much like is done with the array format.

[Viper007Bond]

This is what I meant, although whether it's better or not is debatable

[kurtpayne]

Viper007Bond, your patch looks good. I understand now. Sorry for the miscommunication. Thanks for the attention to detail!

[bpetty]

Patch has been attached, just removing "needs-patch" keyword.

[kurtpayne]

Updated for 3.6

[kurtpayne]

Updated for new phpunit framework

[Viper007Bond]

Silencing errors via @ should be avoided if possible. Since it's a variable now, instead of this:

if ( true === @$args[3] ) 

We can just do this:

if ( ! empty( $args[3] ) )

[kurtpayne]

Use 'empty' instead of a silence operator

[kurtpayne]

Updated patch to use empty instead of silence operator.

[DeanMarkTaylor]

Both keys and values should be encoded, perhaps something like:

$kayvees = array_combine(
	array_map( 'rawurlencode', array_keys( $kayvees ) ),
	array_map( 'rawurlencode', array_values( $kayvees ) )
);

Perhaps WP::build_query_string needs reviewing too regarding encoding keys?

[Viper007Bond]

I keep running into this issue, especially when trying to improve code security. You have to be really careful with this function if you don't want to create bad stuff.

Maybe a better solution is to just introduce a new function, such as add_encoded_query_arg() that encodes and then calls add_query_arg(). That way we don't have to worry about passing yet another argument, especially when we would otherwise only pass 2 arguments. This also has the advantage of being able to deprecate add_query_arg() down the road if we so wish.

[chriscct7]

Per ​https://make.wordpress.org/plugins/2015/04/20/fixing-add_query_arg-and-remove_query_arg-usage/ permalinks should be run through esc_url() before being printed to the page. The new function isn't a bad idea but should be discussed in a new ticket. Closing as wontfix.

[Viper007Bond]

Escaping the output of add_query_arg() is completely unrelated to this issue. :)

Here's a demo:

echo '<a href="' . esc_url( add_query_arg( 'message', 'Make sure you do A & B!' ) ) . '">clicky</a>';
var_dump( $_GET );

Load that up and click the link. It doesn't work like you'd expect:

array (size=2)
  'message' => string 'MakesureyoudoA' (length=14)
  'B!' => string '' (length=0)

[johnbillion]

I'd be interested to know what the following code in add_query_arg() does. It appears to encode the URL's query arguments, but evidently doesn't:

wp_parse_str( $query, $qs );
$qs = urlencode_deep( $qs ); // this re-URL-encodes things that were already in the query string

[SergeyBiryukov]

Related: #46966