1.3 Router urlencode fix #214

Closed
wants to merge 1 commit into
from

4 participants

@basuke

Router's named arguments doesn't encode and decode url values, but it should be. fixed.

For example, CakeDC's search plugin will force redirected to the url
with the POST values encoded as named args. It is safe to encode them.

Yosuke Basuke Suzuki Router's named arguments doesn't encode and decode url values, but it…
… should be. fixed.

For example, CakeDC's search plugin will force redirected to the url
with the POST values encoded as named args. It is safe to encode them.
b0dec45
@renan
CakePHP member

I think forcing the automatically url encoding can cause unwanted behavior.
All other methods that has encoding has a option to disable it.

@basuke

I think named argument is replacement of query parameters in CakePHP. For example, php's http_build_query accept raw values as input and output encoded url. So does Router.

@ADmad
CakePHP member

We can't do this change for 1.3 as it can affect existing apps and cause issues like double encoding. Maybe we can consider it for 2.0.

@basuke

Thanks for taking this patch on the table of 2.0.

I'll send a patch for 2.0. But please re-consider with 1.3. How about adding some Configure option, such as Routing.encodeNamedArgs or Routing.encode.namedArgs.

Configure::write('Routing.encodeNamedArgs', true);

You know this is huge. For example, CakeDC's search plugin does nothing care about named args and Prg component redirects by adding POST vars to named args without url encoding. I can easily image this kind of things happens everywhere. If you say you can consider this in 2.0, let's consider with 1.3. I think CakePHP 1.3 will live for next few years. This will ease the pane of Cake programmers.

How about adding configuration in 1.3, default to false, and 2.0 maybe default to true?

@ADmad
CakePHP member

There aren't going to be any more feature additions in 1.3. What you are asking is an rfc/enhancement not a bug and 1.3 only has bug fix releases.

As for CakeDC's search plugin you should probably send a patch for the issue you mention.

@ADmad ADmad closed this Sep 27, 2011
@markstory
CakePHP member

No more config options please. Deciding on the proper way to handle encoding is a better way. Since they are part of the URL they should be urlencoded. Not encoding them will make URI's with non-alphanumeric characters invalid. There are some problems with urlencoding though, as apache has the horrible behavior of urldecoding, but not re-encoding when running URL's through mod_rewrite.

Named parameters are not a replacement for querystring arguments. They are an aberration that only exists in CakePHP. I'd rather it be removed than improved. The rest of the internet doesn't understand named parameters, and they are not the 'convention' of the internet.

@basuke

I'd rather it be removed than improved.

I totally agree with that. Actually I don't want to use that, but pagination and some plugin depends on that. So I make my mind if I use that, I use that collect way. This patch was born as such.

Anyway, if it will be removed or at least no dependency with core functions, I'm happy to decline this patch, because my next project must be 2.0 :)

@markstory
CakePHP member

Pagination in 2.0 has the option of using querystring args for all pagination parameters at least :)

@basuke

Wow. Sounds great.

I've just checked the controller 2.0 code this afternoon, and found many improvements with its structure. Especially paginator is separated well, and also scaffold handling is moved from dispatcher into controller so I have chance to customize the scaffold! Of source, request objects did eliminate the $params, this is huge.

Great job.

@markstory markstory added a commit that referenced this pull request Oct 30, 2011
@markstory markstory Fix missing urlencod/urldecode in routing.
Named params and passed args should be urlencoded, as they
may contain non-ascii characters.

Refs GH-214
92f9a6c
@m1nd53t m1nd53t pushed a commit that referenced this pull request Mar 10, 2012
@markstory markstory Fix missing urlencod/urldecode in routing.
Named params and passed args should be urlencoded, as they
may contain non-ascii characters.

Refs GH-214
34041d1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment