Make WordPress Core

Opened 18 years ago

Closed 18 years ago

Last modified 17 years ago

#4084 closed defect (bug) (fixed)

add_query_arg urldecodes passed values but doesn't re-encode them.

Reported by: mdawaffe's profile mdawaffe Owned by: rob1n's profile rob1n
Milestone: 2.2 Priority: low
Severity: normal Version: 2.2
Component: General Keywords: has-patch
Focuses: Cc:

Description

It also needs a small tweak regarding the removal of extraneous question marks.

Attachments (2)

4084.diff (732 bytes) - added by mdawaffe 18 years ago.
4084b.diff (1.3 KB) - added by mdawaffe 18 years ago.
idea

Download all attachments as: .zip

Change History (16)

@mdawaffe
18 years ago

#1 @rob1n
18 years ago

  • Owner changed from anonymous to rob1n

#2 @rob1n
18 years ago

It looks fine. I think we should test this, though. Just to make sure "double encoding" works as we want it to.

#3 @rob1n
18 years ago

  • Component changed from Administration to General
  • Keywords has-patch added

#4 @rob1n
18 years ago

  • Resolution set to fixed
  • Status changed from new to closed

(In [5193]) Re-encode query values after passed to add_query_arg(). Props mdawaffe. fixes #4084

#5 @mdawaffe
18 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

This ticket urlencodes every argument. Perhaps we should only urlencode those arguments that have passed through parse_str? That would keep things as backward compatible as possible.

What we do now (urlencoding everything, even new arguments) makes the most since but is not backward compatible.

For example, there are a few places in core (and surely plugins too) that do:

add_query_arg( 'foo', urlencode($bar), $url );

That is, the arg is manually urlencoded. Those args will probably now get double encoded with the current code in trunk.

So do we go for a function that works like (I think) it seems like it should, or do we go with a less convenient function that preserves backward compatibility?

Related to #4105 (slash behavior)

#6 @rob1n
18 years ago

I think preserving backwards compatibility is important. Whenever we try to switch something up on plugin authors (even with tons of warning -- *cough* $table* *cough*) isn't a good idea. Considering history.

As long as it works (double encoding/decoding), then I see no reason to fix it further.

#7 @rob1n
18 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

#8 @rob1n
18 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#9 @rob1n
18 years ago

(In [5240]) Rollback wonky part of [5193]. see #4084

@mdawaffe
18 years ago

idea

#10 @mdawaffe
18 years ago

4084b.diff

Only urlencode()s things that have gone through parse_str().

#11 @rob1n
18 years ago

Hmm. Looks interesting. I'll look at this when I get back later.

#12 @rob1n
18 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [5261]) Get add_query_arg() to urlencode all values of parse_str array. Props mdawaffe. fixes #4084

#13 @majelbstoat
18 years ago

Just so you're aware, this did break another useful function of add_query_arg, which was in easily adding arguments to param style arguments, like for wp_list_pages(). I was using it, for example, to quickly append rewrite rule query entries. So, for those who were doing a similar thing and specifically didn't want things urlencoding, the workaround is to rawurlencode the argument you pass to add_query_arg and then urldecode the whole result.

#14 @markjaquith
17 years ago

(In [6064]) Only urlencode previously existing values in add_query_arg() (more backwards compatible). fixes #4935. see #4084. see #4878

Note: See TracTickets for help on using tickets.