Our wfArrayToCGI and wfCgiToArray conversion functions (they transform data between 'foo=bar&hello=world' and array( 'foo' => 'bar', 'hello' => 'world' ) formats) seam to be fairly messed up.
In a discussion over r104518 on the way it did query parameters to pass to getLocalURL I noticed our lack of sane null handling in wfArrayToCGI. However after examining it even more I found that the output of this area of code seams completely messed up.
Some examples of how these behave:
echo wfArrayToCGI(array('foo' => 'bar', 'baz' => '', 'asdf' => null, 'qwerty' => false));
foo=bar&asdf=&qwerty= # In array -> cgi conversion we will omit a key for an empty string, but yield an empty value for null and false # Frankly it should be the other way around. An empty string is reasonable input to expect an empty but present cgi key.
# By the way, this is our treatment of an empty value in our method that goes in the opposite direction
var_dump(wfCgiToArray('foo=bar&asdf='));
array(2) { ["foo"]=> string(3) "bar" ["asdf"]=> string(0) "" }
# Naturally you can expect how a round trip is screwed up
var_dump(wfArrayToCGI(wfCgiToArray('foo=bar&asdf=')));
string(7) "foo=bar" # Because we treat an empty string as an omission instead of a value the round trip omits something we had in the first place
# cgi to array conversion could use some proper handling of an edge case too
var_dump(wfCgiToArray('foo=bar&asdf=&qwerty'));
PHP Notice: Undefined offset: 1 in /Users/daniel/Workspace/mediawiki/trunk/phase3/includes/GlobalFunctions.php on line 388 array(3) { ["foo"]=> string(3) "bar" ["asdf"]=> string(0) "" ["qwerty"]=> string(0) "" } # Personally I think that 'foo=bar&asdf=&qwerty' should yield an array like # array( 'foo' => 'bar', 'asdf' => '', 'qwerty );
Does anyone think anything would break if I re-coded these two deep core methods to work in a seemingly more sane way.
[r104518] https://www.mediawiki.org/wiki/Special:Code/MediaWiki/104518
On Sat, Dec 10, 2011 at 9:50 PM, Daniel Friesen lists@nadir-seen-fire.comwrote:
Does anyone think anything would break if I re-coded these two deep core methods to work in a seemingly more sane way.
Things *shouldn't* break, but we should watch out in case something does.
I'd noticed this same issue recently and didn't quite want to mess with it in the middle of another fix, but fixing it's the right thing to do. :)
-- brion
On Sun, 11 Dec 2011 00:08:33 -0800, Brion Vibber bvibber@wikimedia.org wrote:
On Sat, Dec 10, 2011 at 9:50 PM, Daniel Friesen lists@nadir-seen-fire.comwrote:
Does anyone think anything would break if I re-coded these two deep core methods to work in a seemingly more sane way.
Things *shouldn't* break, but we should watch out in case something does.
I'd noticed this same issue recently and didn't quite want to mess with it in the middle of another fix, but fixing it's the right thing to do. :)
-- brion
https://www.mediawiki.org/wiki/Special:Code/MediaWiki/105809
wikitech-l@lists.wikimedia.org