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

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

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.

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']) ) )

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..

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 );

Related: #18086

Optionally encode parameters to add_query_arg

Unit test for add_query_arg and remove_query_arg

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.

Sweet. :)

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

s/foreach/array_map/

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.

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

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.

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.

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

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

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

Updated for 3.6

Updated for new phpunit framework

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] ) )

Use 'empty' instead of a silence operator

Updated patch to use empty instead of silence operator.