WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#4084 closed defect (bug) (fixed)

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

Reported by: mdawaffe Owned by: 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 7 years ago.
4084b.diff (1.3 KB) - added by mdawaffe 7 years ago.
idea

Download all attachments as: .zip

Change History (16)

mdawaffe7 years ago

comment:1 rob1n7 years ago

  • Owner changed from anonymous to rob1n

comment:2 rob1n7 years ago

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

comment:3 rob1n7 years ago

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

comment:4 rob1n7 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

comment:5 mdawaffe7 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)

comment:6 rob1n7 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.

comment:7 rob1n7 years ago

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

comment:8 rob1n7 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:9 rob1n7 years ago

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

mdawaffe7 years ago

idea

comment:10 mdawaffe7 years ago

4084b.diff

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

comment:11 rob1n7 years ago

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

comment:12 rob1n7 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

comment:13 majelbstoat7 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.

comment:14 markjaquith7 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.