Mailing List Archive: Wikipedia: Wikitech

r34541 and r34542

Index | Next | Previous | View Threaded

dan_the_man at telus

May 9, 2008, 6:13 PM

Post #1 of 16 (280 views)
Permalink

r34541 and r34542

http://svn.wikimedia.org/viewvc/mediawiki/?view=rev&revision=34541
http://svn.wikimedia.org/viewvc/mediawiki/?view=rev&revision=34542

Since brion had to go I'll post here and detail the magic quotes stuff.

Though, when it's reviewed and if it's ok, please don't un-revert it.
There were 3 accidental PHP syntax errors committed, I fixed all three,
but brion reverted for different reasons before I could commit the fix.
So if marked as ok, let me to the commit since I have the syntax
error-less version. But if you were wondering, Missing $ on 2374, and a
comma instead of a : on 2388 and 2389.

To make some improvements to the Title::getLocalURL and
Title::getFullURL functions I did some changes:

Firstly, the point. I'm trying to add in some functionality which we
nicely give out in things like $db->select, but is missing and would be
quite helpful in the two Title functions.
The idea is to allow an array or object as input into the query
parameter rather than restricting to a plain string.
In that way a developer can call something like:
$wgTitle->getLocalURL(array(
'foo' => $<Dangerous user supplied variable>,
'bar' => 'baz'
));
Rather than:
$wgTitle->getLocalURL("foo=" . addslashes($<Dangerous user supplied
variable>) . "&bar=baz");
It cleans up the input and allows developers to finally do the same
things done inside of the clean Database functions as well as the Xml
functions.

To do this I had to introduce a new function, similar to an existing
one. The new function named wfBuildQuery (because it is a substitute for
http_build_query, keeping the naming convention), which was to replace
the old wfArrayToCgi, which has two issues itself, the arguments are in
an unintuitive order (The reason it supports 2 arrays is so that you can
merge parameters into an array of defaults, however in wfArrayToCgi you
need to put the defaults first if you want to use them, so you can't
properly document the parameters since they change in definition
depending on how many params you pass), and the second issue is the
name... The definition here is that we can pass it a array, object, or
query string and it'll output a query string for us to use. But
ArrayToCgi doesn't fit that definition anymore.

Ok, so now how the train went...
We want to support both arrays and objects in inputs now, as well as
strings.
However, the hitch. wfArrayToCgi is under it's definition supposed to be
similar to wfBuildQuery but it supports having two arrays so you can
merge a defaults array with the query array.

First change... In wfArrayToCgi the order of the + operator ends up
making it so that you need to put the default first... So, if you pass
one parameter that is the query, if you pass two, then suddenly that
query parameter becomes the defaults parameter and the second becomes
the query parameter. That does not follow the convention of parameters
which we follow. So I reversed the order in wfBuildQuery and gave it
explicit $query and $default variables.

But now... remember we're now supporting arrays and objects. So, to fix
that, "$query = ((array)$defaults) + ((array)$query);" basically we're
typesetting the objects into arrays so we're making sure that we combine
two like types rather than is being possible to combine an array and an
object and get a potential error about trying to combine unlike types.
If you don't get what the dual braces are for, that's because if we
didn't wrap the typesets then they would apply to the whole thing and
not the individual variables, and we'd be back at square one trying to
combine two potentially unlike objects.

Ok, now onto our third method of input. Rather than forcing our code to
test the type of something before it passes it onto the function, we
should allow strings to passthrough rather than return fatal errors when
they try to.
So, we have a is_string down at the bottom, if it's a string it passes
through, if it isn't then we build a query string from it.

Ok, now we have one last valid input case which we have not handled.
What if we have a defaults array, and the query is a string? We can't
merge a strings the same way we can merge arrays.
So, the solution is we're going to have to take the string and parse it
into a query array so that it can be merged.
Ok, we've got two ways of doing this now...
A) Build some insane set of code to split up the query string which is
bound to fail on anything past basic data.
B) Use PHP's built in parse_str to turn the query string into a query
array with the exact same output as PHP. Quite a bit more reliable than
writing your own kludge by hand.

So, ok... We're using php's parse_str now to break apart the strings,
then we merge them back into an array. So we can turn the final result
into a good query string.
Two issues. Firstly, we don't want to split a string then merge it back
together all the time do we? No, that's stupid and inefficient. So, the
string parsing is contained inside the same if that the merge is inside
of. We only split the query string into an array if we have a default
query that we need to merge with. Just for sanity's sake we also make
sure that the defaults param is an array to and also split it if it's a
string. But if we only pass a single string and don't have a default,
then that is all skipped by and because of the is_string down below the
string simply passes in from the input, and out as the return value
sucessfully allowing any valid input and outputting a string all the
time, additionally that means it's safe so a query can be passed through
twice without anything being broken.

Ok, final issue, the one which caused brion to raise an eyebrow. There
is an issue with parse_str, it's even documented on it's php doc page.
"*Note:* The magic_quotes_gpc setting affects the output of this
function, as parse_str() uses the same mechanism that PHP uses to
populate the $_GET, $_POST, etc. variables."
Eeew... parse_str has the same issues we have to deal with inside of
$wgRequest... So, what do we do?
We test for get_magic_quotes_gpc just like $wgRequest does. And then if
it returns true we use the WebRequest's public fix_magic_quotes function
to clean up the evilness that parse_str gave us.

And voila, we have a function which does the best it can to do things
with clean code, and is easy to use inside of code.

Though on the note of that, before someone decides to unrevert. I think
it would be a good idea to create a wfParseQuery function as a safe
wrapper for parse_str in all it's evil gpc horror in case someone needs
to break up a query string again.

If you're wondering about the other changes:
We have the pair of wfMemcKey and wfForeignMemcKey, however we don't
have a pair for wfWikiID.
So I created the wfForeignWikiID. It's not apparent now, but with the
new ability to share specific tables people are going to find useful
ways to make advantage of that, and it's going to be a good idea to have
a way to get a key for the shared data or data of another wiki.
Helpfully the wfForeignWikiID function takes input for you to specify
the database and prefix, if you don't pass data to it then it will
fallback to the shared database's data. Which 85% of the time, is what
we want, something is shared and we want the shared ID to go along with
it. And finally, if you have no shared database set, we fallback to the
local database which will be what we want.

The MemcKey functions were also duplicating the functionality of the
WikiID functions. They had the same conditional expression inside of
them that the WikiID functions had. I made them make use of those and
cut them down to 3 liners.
Technically, the Local functions could actually be made to use the
Foreign functions. That would eliminate more code duplication. I could
do that to.

--
~Daniel Friesen(Dantman) of:
-The Gaiapedia (http://gaia.wikia.com)
-Wikia ACG on Wikia.com (http://wikia.com/wiki/Wikia_ACG)
-and Wiki-Tools.com (http://wiki-tools.com)

_______________________________________________
Wikitech-l mailing list
Wikitech-l[at]lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

brion at wikimedia

May 10, 2008, 9:34 AM

Post #2 of 16 (257 views)
Permalink

Re: r34541 and r34542 [In reply to]

A) there's no need for addslashes() on URL-encoded output

B) if parse_str is that broken, please avoid it.

-- brion vibber (brion @ wikimedia.org)

On May 9, 2008, at 18:13, DanTMan <dan_the_man[at]telus.net> wrote:

> http://svn.wikimedia.org/viewvc/mediawiki/?view=rev&revision=34541
> http://svn.wikimedia.org/viewvc/mediawiki/?view=rev&revision=34542
>
> Since brion had to go I'll post here and detail the magic quotes
> stuff.
>
> Though, when it's reviewed and if it's ok, please don't un-revert it.
> There were 3 accidental PHP syntax errors committed, I fixed all
> three,
> but brion reverted for different reasons before I could commit the
> fix.
> So if marked as ok, let me to the commit since I have the syntax
> error-less version. But if you were wondering, Missing $ on 2374,
> and a
> comma instead of a : on 2388 and 2389.
>
> To make some improvements to the Title::getLocalURL and
> Title::getFullURL functions I did some changes:
>
> Firstly, the point. I'm trying to add in some functionality which we
> nicely give out in things like $db->select, but is missing and would
> be
> quite helpful in the two Title functions.
> The idea is to allow an array or object as input into the query
> parameter rather than restricting to a plain string.
> In that way a developer can call something like:
> $wgTitle->getLocalURL(array(
> 'foo' => $<Dangerous user supplied variable>,
> 'bar' => 'baz'
> ));
> Rather than:
> $wgTitle->getLocalURL("foo=" . addslashes($<Dangerous user supplied
> variable>) . "&bar=baz");
> It cleans up the input and allows developers to finally do the same
> things done inside of the clean Database functions as well as the Xml
> functions.
>
> To do this I had to introduce a new function, similar to an existing
> one. The new function named wfBuildQuery (because it is a substitute
> for
> http_build_query, keeping the naming convention), which was to replace
> the old wfArrayToCgi, which has two issues itself, the arguments are
> in
> an unintuitive order (The reason it supports 2 arrays is so that you
> can
> merge parameters into an array of defaults, however in wfArrayToCgi
> you
> need to put the defaults first if you want to use them, so you can't
> properly document the parameters since they change in definition
> depending on how many params you pass), and the second issue is the
> name... The definition here is that we can pass it a array, object, or
> query string and it'll output a query string for us to use. But
> ArrayToCgi doesn't fit that definition anymore.
>
> Ok, so now how the train went...
> We want to support both arrays and objects in inputs now, as well as
> strings.
> However, the hitch. wfArrayToCgi is under it's definition supposed
> to be
> similar to wfBuildQuery but it supports having two arrays so you can
> merge a defaults array with the query array.
>
> First change... In wfArrayToCgi the order of the + operator ends up
> making it so that you need to put the default first... So, if you pass
> one parameter that is the query, if you pass two, then suddenly that
> query parameter becomes the defaults parameter and the second becomes
> the query parameter. That does not follow the convention of parameters
> which we follow. So I reversed the order in wfBuildQuery and gave it
> explicit $query and $default variables.
>
> But now... remember we're now supporting arrays and objects. So, to
> fix
> that, "$query = ((array)$defaults) + ((array)$query);" basically we're
> typesetting the objects into arrays so we're making sure that we
> combine
> two like types rather than is being possible to combine an array and
> an
> object and get a potential error about trying to combine unlike types.
> If you don't get what the dual braces are for, that's because if we
> didn't wrap the typesets then they would apply to the whole thing and
> not the individual variables, and we'd be back at square one trying to
> combine two potentially unlike objects.
>
> Ok, now onto our third method of input. Rather than forcing our code
> to
> test the type of something before it passes it onto the function, we
> should allow strings to passthrough rather than return fatal errors
> when
> they try to.
> So, we have a is_string down at the bottom, if it's a string it passes
> through, if it isn't then we build a query string from it.
>
> Ok, now we have one last valid input case which we have not handled.
> What if we have a defaults array, and the query is a string? We can't
> merge a strings the same way we can merge arrays.
> So, the solution is we're going to have to take the string and parse
> it
> into a query array so that it can be merged.
> Ok, we've got two ways of doing this now...
> A) Build some insane set of code to split up the query string which is
> bound to fail on anything past basic data.
> B) Use PHP's built in parse_str to turn the query string into a query
> array with the exact same output as PHP. Quite a bit more reliable
> than
> writing your own kludge by hand.
>
> So, ok... We're using php's parse_str now to break apart the strings,
> then we merge them back into an array. So we can turn the final result
> into a good query string.
> Two issues. Firstly, we don't want to split a string then merge it
> back
> together all the time do we? No, that's stupid and inefficient. So,
> the
> string parsing is contained inside the same if that the merge is
> inside
> of. We only split the query string into an array if we have a default
> query that we need to merge with. Just for sanity's sake we also make
> sure that the defaults param is an array to and also split it if
> it's a
> string. But if we only pass a single string and don't have a default,
> then that is all skipped by and because of the is_string down below
> the
> string simply passes in from the input, and out as the return value
> sucessfully allowing any valid input and outputting a string all the
> time, additionally that means it's safe so a query can be passed
> through
> twice without anything being broken.
>
> Ok, final issue, the one which caused brion to raise an eyebrow. There
> is an issue with parse_str, it's even documented on it's php doc page.
> "*Note:* The magic_quotes_gpc setting affects the output of this
> function, as parse_str() uses the same mechanism that PHP uses to
> populate the $_GET, $_POST, etc. variables."
> Eeew... parse_str has the same issues we have to deal with inside of
> $wgRequest... So, what do we do?
> We test for get_magic_quotes_gpc just like $wgRequest does. And then
> if
> it returns true we use the WebRequest's public fix_magic_quotes
> function
> to clean up the evilness that parse_str gave us.
>
> And voila, we have a function which does the best it can to do things
> with clean code, and is easy to use inside of code.
>
> Though on the note of that, before someone decides to unrevert. I
> think
> it would be a good idea to create a wfParseQuery function as a safe
> wrapper for parse_str in all it's evil gpc horror in case someone
> needs
> to break up a query string again.
>
> If you're wondering about the other changes:
> We have the pair of wfMemcKey and wfForeignMemcKey, however we don't
> have a pair for wfWikiID.
> So I created the wfForeignWikiID. It's not apparent now, but with the
> new ability to share specific tables people are going to find useful
> ways to make advantage of that, and it's going to be a good idea to
> have
> a way to get a key for the shared data or data of another wiki.
> Helpfully the wfForeignWikiID function takes input for you to specify
> the database and prefix, if you don't pass data to it then it will
> fallback to the shared database's data. Which 85% of the time, is what
> we want, something is shared and we want the shared ID to go along
> with
> it. And finally, if you have no shared database set, we fallback to
> the
> local database which will be what we want.
>
> The MemcKey functions were also duplicating the functionality of the
> WikiID functions. They had the same conditional expression inside of
> them that the WikiID functions had. I made them make use of those and
> cut them down to 3 liners.
> Technically, the Local functions could actually be made to use the
> Foreign functions. That would eliminate more code duplication. I could
> do that to.
>
> --
> ~Daniel Friesen(Dantman) of:
> -The Gaiapedia (http://gaia.wikia.com)
> -Wikia ACG on Wikia.com (http://wikia.com/wiki/Wikia_ACG)
> -and Wiki-Tools.com (http://wiki-tools.com)
>
> _______________________________________________
> Wikitech-l mailing list
> Wikitech-l[at]lists.wikimedia.org
> https://lists.wikimedia.org/mailman/listinfo/wikitech-l

_______________________________________________
Wikitech-l mailing list
Wikitech-l[at]lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

dan_the_man at telus

May 11, 2008, 2:25 AM

Post #3 of 16 (254 views)
Permalink

Re: r34541 and r34542 [In reply to]

parse_str isn't actually broken at all. It does exactly what it was
meant to do.
Duplicate what PHP does with $_SERVER['QUERY_STRING'] to create the
$_GET variable.

The issue is that PHP has gpc quotes on those. And because of the
definition of parse_str it does that to.
So we're merely handling parse_str in the same way as we handle PHP's
$_GET variable. By undoing that.

parse_str is avoided, the code never touches it unless it is absolutely
necessary. The only case where it may be
necessary is if we're given a string for the query, and the developer
asks us to merge that with the defaults.
And so the code makes sure to avoid it unless that is requested.

parse_str also can't be completely avoided if we want to break apart the
string.
Firstly we would likely mangle the query string by trying to do it
ourselves and suddenly encountering
complex data, which at some point would be attempted to be merged back
into a string.
Because we did not split apart the string in the same way that PHP did,
and thus have been passing around
data in a format that MediaWiki does not handle (invalid broken query
string) the end result will be a bad string
which is outputted into the page.
Incidentally, I also remember reading something about php coded
functions made to duplicate what parse_str
does to be more inefficient than it. In other words, it's more of a
performance issue than simply handling it the
same way we handle $_GET.

~Daniel Friesen(Dantman) of:
-The Gaiapedia (http://gaia.wikia.com)
-Wikia ACG on Wikia.com (http://wikia.com/wiki/Wikia_ACG)
-and Wiki-Tools.com (http://wiki-tools.com)

Brion Vibber wrote:
> A) there's no need for addslashes() on URL-encoded output
>
> B) if parse_str is that broken, please avoid it.
>
> -- brion vibber (brion @ wikimedia.org)
>
> On May 9, 2008, at 18:13, DanTMan <dan_the_man[at]telus.net> wrote:
>
>
>> http://svn.wikimedia.org/viewvc/mediawiki/?view=rev&revision=34541
>> http://svn.wikimedia.org/viewvc/mediawiki/?view=rev&revision=34542
>>
>> Since brion had to go I'll post here and detail the magic quotes
>> stuff.
>>
>> Though, when it's reviewed and if it's ok, please don't un-revert it.
>> There were 3 accidental PHP syntax errors committed, I fixed all
>> three,
>> but brion reverted for different reasons before I could commit the
>> fix.
>> So if marked as ok, let me to the commit since I have the syntax
>> error-less version. But if you were wondering, Missing $ on 2374,
>> and a
>> comma instead of a : on 2388 and 2389.
>>
>> To make some improvements to the Title::getLocalURL and
>> Title::getFullURL functions I did some changes:
>>
>> Firstly, the point. I'm trying to add in some functionality which we
>> nicely give out in things like $db->select, but is missing and would
>> be
>> quite helpful in the two Title functions.
>> The idea is to allow an array or object as input into the query
>> parameter rather than restricting to a plain string.
>> In that way a developer can call something like:
>> $wgTitle->getLocalURL(array(
>> 'foo' => $<Dangerous user supplied variable>,
>> 'bar' => 'baz'
>> ));
>> Rather than:
>> $wgTitle->getLocalURL("foo=" . addslashes($<Dangerous user supplied
>> variable>) . "&bar=baz");
>> It cleans up the input and allows developers to finally do the same
>> things done inside of the clean Database functions as well as the Xml
>> functions.
>>
>> To do this I had to introduce a new function, similar to an existing
>> one. The new function named wfBuildQuery (because it is a substitute
>> for
>> http_build_query, keeping the naming convention), which was to replace
>> the old wfArrayToCgi, which has two issues itself, the arguments are
>> in
>> an unintuitive order (The reason it supports 2 arrays is so that you
>> can
>> merge parameters into an array of defaults, however in wfArrayToCgi
>> you
>> need to put the defaults first if you want to use them, so you can't
>> properly document the parameters since they change in definition
>> depending on how many params you pass), and the second issue is the
>> name... The definition here is that we can pass it a array, object, or
>> query string and it'll output a query string for us to use. But
>> ArrayToCgi doesn't fit that definition anymore.
>>
>> Ok, so now how the train went...
>> We want to support both arrays and objects in inputs now, as well as
>> strings.
>> However, the hitch. wfArrayToCgi is under it's definition supposed
>> to be
>> similar to wfBuildQuery but it supports having two arrays so you can
>> merge a defaults array with the query array.
>>
>> First change... In wfArrayToCgi the order of the + operator ends up
>> making it so that you need to put the default first... So, if you pass
>> one parameter that is the query, if you pass two, then suddenly that
>> query parameter becomes the defaults parameter and the second becomes
>> the query parameter. That does not follow the convention of parameters
>> which we follow. So I reversed the order in wfBuildQuery and gave it
>> explicit $query and $default variables.
>>
>> But now... remember we're now supporting arrays and objects. So, to
>> fix
>> that, "$query = ((array)$defaults) + ((array)$query);" basically we're
>> typesetting the objects into arrays so we're making sure that we
>> combine
>> two like types rather than is being possible to combine an array and
>> an
>> object and get a potential error about trying to combine unlike types.
>> If you don't get what the dual braces are for, that's because if we
>> didn't wrap the typesets then they would apply to the whole thing and
>> not the individual variables, and we'd be back at square one trying to
>> combine two potentially unlike objects.
>>
>> Ok, now onto our third method of input. Rather than forcing our code
>> to
>> test the type of something before it passes it onto the function, we
>> should allow strings to passthrough rather than return fatal errors
>> when
>> they try to.
>> So, we have a is_string down at the bottom, if it's a string it passes
>> through, if it isn't then we build a query string from it.
>>
>> Ok, now we have one last valid input case which we have not handled.
>> What if we have a defaults array, and the query is a string? We can't
>> merge a strings the same way we can merge arrays.
>> So, the solution is we're going to have to take the string and parse
>> it
>> into a query array so that it can be merged.
>> Ok, we've got two ways of doing this now...
>> A) Build some insane set of code to split up the query string which is
>> bound to fail on anything past basic data.
>> B) Use PHP's built in parse_str to turn the query string into a query
>> array with the exact same output as PHP. Quite a bit more reliable
>> than
>> writing your own kludge by hand.
>>
>> So, ok... We're using php's parse_str now to break apart the strings,
>> then we merge them back into an array. So we can turn the final result
>> into a good query string.
>> Two issues. Firstly, we don't want to split a string then merge it
>> back
>> together all the time do we? No, that's stupid and inefficient. So,
>> the
>> string parsing is contained inside the same if that the merge is
>> inside
>> of. We only split the query string into an array if we have a default
>> query that we need to merge with. Just for sanity's sake we also make
>> sure that the defaults param is an array to and also split it if
>> it's a
>> string. But if we only pass a single string and don't have a default,
>> then that is all skipped by and because of the is_string down below
>> the
>> string simply passes in from the input, and out as the return value
>> sucessfully allowing any valid input and outputting a string all the
>> time, additionally that means it's safe so a query can be passed
>> through
>> twice without anything being broken.
>>
>> Ok, final issue, the one which caused brion to raise an eyebrow. There
>> is an issue with parse_str, it's even documented on it's php doc page.
>> "*Note:* The magic_quotes_gpc setting affects the output of this
>> function, as parse_str() uses the same mechanism that PHP uses to
>> populate the $_GET, $_POST, etc. variables."
>> Eeew... parse_str has the same issues we have to deal with inside of
>> $wgRequest... So, what do we do?
>> We test for get_magic_quotes_gpc just like $wgRequest does. And then
>> if
>> it returns true we use the WebRequest's public fix_magic_quotes
>> function
>> to clean up the evilness that parse_str gave us.
>>
>> And voila, we have a function which does the best it can to do things
>> with clean code, and is easy to use inside of code.
>>
>> Though on the note of that, before someone decides to unrevert. I
>> think
>> it would be a good idea to create a wfParseQuery function as a safe
>> wrapper for parse_str in all it's evil gpc horror in case someone
>> needs
>> to break up a query string again.
>>
>> If you're wondering about the other changes:
>> We have the pair of wfMemcKey and wfForeignMemcKey, however we don't
>> have a pair for wfWikiID.
>> So I created the wfForeignWikiID. It's not apparent now, but with the
>> new ability to share specific tables people are going to find useful
>> ways to make advantage of that, and it's going to be a good idea to
>> have
>> a way to get a key for the shared data or data of another wiki.
>> Helpfully the wfForeignWikiID function takes input for you to specify
>> the database and prefix, if you don't pass data to it then it will
>> fallback to the shared database's data. Which 85% of the time, is what
>> we want, something is shared and we want the shared ID to go along
>> with
>> it. And finally, if you have no shared database set, we fallback to
>> the
>> local database which will be what we want.
>>
>> The MemcKey functions were also duplicating the functionality of the
>> WikiID functions. They had the same conditional expression inside of
>> them that the WikiID functions had. I made them make use of those and
>> cut them down to 3 liners.
>> Technically, the Local functions could actually be made to use the
>> Foreign functions. That would eliminate more code duplication. I could
>> do that to.
>>
>> --
>> ~Daniel Friesen(Dantman) of:
>> -The Gaiapedia (http://gaia.wikia.com)
>> -Wikia ACG on Wikia.com (http://wikia.com/wiki/Wikia_ACG)
>> -and Wiki-Tools.com (http://wiki-tools.com)
>>
>> _______________________________________________
>> Wikitech-l mailing list
>> Wikitech-l[at]lists.wikimedia.org
>> https://lists.wikimedia.org/mailman/listinfo/wikitech-l
>>
>
> _______________________________________________
> Wikitech-l mailing list
> Wikitech-l[at]lists.wikimedia.org
> https://lists.wikimedia.org/mailman/listinfo/wikitech-l
>
>
_______________________________________________
Wikitech-l mailing list
Wikitech-l[at]lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

brion at wikimedia

May 12, 2008, 9:28 AM

Post #4 of 16 (250 views)
Permalink

Re: r34541 and r34542 [In reply to]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

DanTMan wrote:
> parse_str isn't actually broken at all. It does exactly what it was
> meant to do.
> Duplicate what PHP does with $_SERVER['QUERY_STRING'] to create the
> $_GET variable.
>
> The issue is that PHP has gpc quotes on those. And because of the
> definition of parse_str it does that to.
> So we're merely handling parse_str in the same way as we handle PHP's
> $_GET variable. By undoing that.
>
> parse_str is avoided, the code never touches it unless it is absolutely
> necessary. The only case where it may be
> necessary is if we're given a string for the query, and the developer
> asks us to merge that with the defaults.
> And so the code makes sure to avoid it unless that is requested.

Please don't invent unnecessary hoops to jump through... :)

http://c2.com/cgi/wiki?YouArentGonnaNeedIt

- -- brion
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.8 (Darwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkgocDsACgkQwRnhpk1wk46YGwCglD+9NRbE0fYxYGv4JzZn3FsF
U14AoIAnSAjAloA3SsK50XPp027jM3TT
=Pr46
-----END PGP SIGNATURE-----

_______________________________________________
Wikitech-l mailing list
Wikitech-l[at]lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

dan_the_man at telus

May 12, 2008, 9:58 AM

Post #5 of 16 (249 views)
Permalink

Re: r34541 and r34542 [In reply to]

So, if someone passes in wfBuildQuery( String, Array ); You just want
MediaWiki to die with a error?
I can shorten the code to do that if you want.
But I don't think a developer is going to be happy when they take a
query string passed to them from another part of MediaWiki and try to
merge that in with some other data they need in the query, and end up
with MediaWiki dieing. Even a silent failure wouldn't be that great as
the developer won't understand why their urls never show what query data
they told it to use.

~Daniel Friesen(Dantman) of:
-The Gaiapedia (http://gaia.wikia.com)
-Wikia ACG on Wikia.com (http://wikia.com/wiki/Wikia_ACG)
-and Wiki-Tools.com (http://wiki-tools.com)

Brion Vibber wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> DanTMan wrote:
>
>> parse_str isn't actually broken at all. It does exactly what it was
>> meant to do.
>> Duplicate what PHP does with $_SERVER['QUERY_STRING'] to create the
>> $_GET variable.
>>
>> The issue is that PHP has gpc quotes on those. And because of the
>> definition of parse_str it does that to.
>> So we're merely handling parse_str in the same way as we handle PHP's
>> $_GET variable. By undoing that.
>>
>> parse_str is avoided, the code never touches it unless it is absolutely
>> necessary. The only case where it may be
>> necessary is if we're given a string for the query, and the developer
>> asks us to merge that with the defaults.
>> And so the code makes sure to avoid it unless that is requested.
>>
>
> Please don't invent unnecessary hoops to jump through... :)
>
> http://c2.com/cgi/wiki?YouArentGonnaNeedIt
>
> - -- brion
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.8 (Darwin)
> Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
>
> iEYEARECAAYFAkgocDsACgkQwRnhpk1wk46YGwCglD+9NRbE0fYxYGv4JzZn3FsF
> U14AoIAnSAjAloA3SsK50XPp027jM3TT
> =Pr46
> -----END PGP SIGNATURE-----
>
> _______________________________________________
> Wikitech-l mailing list
> Wikitech-l[at]lists.wikimedia.org
> https://lists.wikimedia.org/mailman/listinfo/wikitech-l
>
>
_______________________________________________
Wikitech-l mailing list
Wikitech-l[at]lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

brion at wikimedia

May 12, 2008, 10:46 AM

Post #6 of 16 (247 views)
Permalink

Re: r34541 and r34542 [In reply to]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

DanTMan wrote:
> So, if someone passes in wfBuildQuery( String, Array ); You just want
> MediaWiki to die with a error?

I want them to not send *any old random data* to functions without any
reason to.

- -- brion
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.8 (Darwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkgogncACgkQwRnhpk1wk45xzgCfUYbMEMW/3pYFzTv1W1edToWr
2X8AnjCSYr4S3JaEuYg4uO4hWmcbXuPl
=V5YG
-----END PGP SIGNATURE-----

_______________________________________________
Wikitech-l mailing list
Wikitech-l[at]lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

dan_the_man at telus

May 12, 2008, 11:10 AM

Post #7 of 16 (248 views)
Permalink

Re: r34541 and r34542 [In reply to]

But it's not random data. It's query data. String, Arrays, and Objects
are all valid forms of Query Data. And wfBuildQuery is meant to convert
any of those valid forms into a string form.
The purpose of wfBuildQuery's second parameter is the same as the old
wfArrayToCgi. Taking the query data and merging it with a set of
defaults or extra query data needed in the url.
The expectation is that they can take valid query data, and merge it in
with more query data.

But the function is going to need some behavior if a string query is
passed and defaults are given:
A) Die with error saying defaults cannot be merged into a string.
B) Silently fail passing the original string along and wfDebug'ing that
query data can't be sent as strings when needed to be merged with more data.
C) Tack a merged version of the defaults onto the end of the query
string, hoping to god that data isn't being overridden in a way which
breaks it's meaning.
D) Current behavior inside the commit of parsing the string then merging
it, which works perfectly well.

~Daniel Friesen(Dantman) of:
-The Gaiapedia (http://gaia.wikia.com)
-Wikia ACG on Wikia.com (http://wikia.com/wiki/Wikia_ACG)
-and Wiki-Tools.com (http://wiki-tools.com)

Brion Vibber wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> DanTMan wrote:
>
>> So, if someone passes in wfBuildQuery( String, Array ); You just want
>> MediaWiki to die with a error?
>>
>
> I want them to not send *any old random data* to functions without any
> reason to.
>
> - -- brion
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.8 (Darwin)
> Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
>
> iEYEARECAAYFAkgogncACgkQwRnhpk1wk45xzgCfUYbMEMW/3pYFzTv1W1edToWr
> 2X8AnjCSYr4S3JaEuYg4uO4hWmcbXuPl
> =V5YG
> -----END PGP SIGNATURE-----
>
> _______________________________________________
> Wikitech-l mailing list
> Wikitech-l[at]lists.wikimedia.org
> https://lists.wikimedia.org/mailman/listinfo/wikitech-l
>
>
_______________________________________________
Wikitech-l mailing list
Wikitech-l[at]lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

brion at wikimedia

May 12, 2008, 11:19 AM

Post #8 of 16 (248 views)
Permalink

Re: r34541 and r34542 [In reply to]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

DanTMan wrote:
> But it's not random data. It's query data. String, Arrays, and Objects
> are all valid forms of Query Data.

I'd much prefer that we *not* explode the possible data formats by
inventing brand new ones without clear reason.

- -- brion
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.8 (Darwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkgoik4ACgkQwRnhpk1wk44uVACdFBmddRShqXU2lBhF1S99L+Zi
5EgAni/6J4/gb6xxr7Q2hw3tOl2/ITdA
=FQ9+
-----END PGP SIGNATURE-----

_______________________________________________
Wikitech-l mailing list
Wikitech-l[at]lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

dan_the_man at telus

May 12, 2008, 1:03 PM

Post #9 of 16 (247 views)
Permalink

Re: r34541 and r34542 [In reply to]

They're not brand new. Supporting Arrays as queries is just like
supporting sql conditions that way, and supplying arrays as attributes
in the Xml class. We're already doing this in other things, but the
Title class's two methods don't have any code meant to support that.
Which brings out wfBuildQuery because the current wfArrayToCgi is broken
because it does not handle data correctly like http_build_query does.
However http_build_query does not passthrough strings, which is what our
other cases where we convert arrays to strings allow, but moreover it
just needs a tweak to make sure the query separator is set right (we
either need to set an INI var or an argument; but that's a different
train of discussion).
The Object is just a side product of Array support.

The wfBuildQuery's second parameter was merely for compatibility with
what wfArrayToCgi was doing. If you really want, we could leave
wfArrayToCgi alone, or even just merge there. And keep merges out of
wfBuildQuery and make it a dedicated (Valid data) -to> (String)
function. Or by leaving wfArrayToCgi alone and depreciating it as a
forever broken function?

$title->getLocalURL(array( 'action' => $action ));
or even
$rcTitle->getFullURL(array(
'limit' => 250,
'days' => 3,
'from' => time(),
'hidebots' => $wgRequest->getBool('hidebots')
));

~Daniel Friesen(Dantman) of:
-The Gaiapedia (http://gaia.wikia.com)
-Wikia ACG on Wikia.com (http://wikia.com/wiki/Wikia_ACG)
-and Wiki-Tools.com (http://wiki-tools.com)

Brion Vibber wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> DanTMan wrote:
>
>> But it's not random data. It's query data. String, Arrays, and Objects
>> are all valid forms of Query Data.
>>
>
> I'd much prefer that we *not* explode the possible data formats by
> inventing brand new ones without clear reason.
>
> - -- brion
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.8 (Darwin)
> Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
>
> iEYEARECAAYFAkgoik4ACgkQwRnhpk1wk44uVACdFBmddRShqXU2lBhF1S99L+Zi
> 5EgAni/6J4/gb6xxr7Q2hw3tOl2/ITdA
> =FQ9+
> -----END PGP SIGNATURE-----
>
> _______________________________________________
> Wikitech-l mailing list
> Wikitech-l[at]lists.wikimedia.org
> https://lists.wikimedia.org/mailman/listinfo/wikitech-l
>
>
_______________________________________________
Wikitech-l mailing list
Wikitech-l[at]lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

brion at wikimedia

May 12, 2008, 2:32 PM

Post #10 of 16 (247 views)
Permalink

Re: r34541 and r34542 [In reply to]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

A general suggestion: slow down!

I've noticed a lot of your mails are just *packed* with thing after
thing after thing. This discourages people from reading them in whole
and actually addressing the points you raise.

Slow down and address specific issues, briefly and to the point.

Brevity is the sole of wit! :)

DanTMan wrote:
> They're not brand new. Supporting Arrays as queries is just like
> supporting sql conditions that way, and supplying arrays as attributes
> in the Xml class. We're already doing this in other things, but the
> Title class's two methods don't have any code meant to support that.

^^ This here should be the core of the issue!

Concentrate on the actual goal of simplifying code and encouraging
safe-by-default behavior by reducing manual query-string building.

> Which brings out wfBuildQuery because the current wfArrayToCgi is broken
> because it does not handle data correctly like http_build_query does.

Ok, let's address this:

1) wfArrayToCgi() doesn't happen to behave exactly as http_build_query()
does.

2) Since we don't use http_build_query(), there's nothing inherently
wrong with that.

3) Failing to support a *required* behavior, which as it happens some
other function X does, *might* in fact be considered broken. Remember to
distinguish these cases; to make your case, you must address the
specific behavior which is not being handled correctly.

> The Object is just a side product of Array support.

Tossing in objects is a totally different ballgame. This isn't a case
where arrays & objects sensibly can or should be treated the same way.

> The wfBuildQuery's second parameter was merely for compatibility with
> what wfArrayToCgi was doing.

There's no particular need for a wfBuildQuery() at all, or for changing
the acceptable inputs to wfArrayToCgi().

Go back to your core issue: allowing Title::get*Url() functions to
accept associated arrays of data to build query strings from, in
addition to raw query strings (for backwards compatibility).

Look at what that actually requires, and concentrate on that.

- -- brion vibber (brion @ wikimedia.org)
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.8 (Darwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkgot1AACgkQwRnhpk1wk47whQCgrtUZKc4946yp37otDov11z+5
/4UAn3TtSfTCgztKLusvbf+jYQAYqEDS
=a8TE
-----END PGP SIGNATURE-----

_______________________________________________
Wikitech-l mailing list
Wikitech-l[at]lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

dan_the_man at telus

May 12, 2008, 11:38 PM

Post #11 of 16 (242 views)
Permalink

Re: r34541 and r34542 [In reply to]

Brion Vibber wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> A general suggestion: slow down!
>
> I've noticed a lot of your mails are just *packed* with thing after
> thing after thing. This discourages people from reading them in whole
> and actually addressing the points you raise.
>
> Slow down and address specific issues, briefly and to the point.
>
> Brevity is the sole of wit! :)
>
>
> DanTMan wrote:
>
>> They're not brand new. Supporting Arrays as queries is just like
>> supporting sql conditions that way, and supplying arrays as attributes
>> in the Xml class. We're already doing this in other things, but the
>> Title class's two methods don't have any code meant to support that.
>>
>
> ^^ This here should be the core of the issue!
>
> Concentrate on the actual goal of simplifying code and encouraging
> safe-by-default behavior by reducing manual query-string building.
>
>
>> Which brings out wfBuildQuery because the current wfArrayToCgi is broken
>> because it does not handle data correctly like http_build_query does.
>>
>
> Ok, let's address this:
>
> 1) wfArrayToCgi() doesn't happen to behave exactly as http_build_query()
> does.
>
> 2) Since we don't use http_build_query(), there's nothing inherently
> wrong with that.
>
> 3) Failing to support a *required* behavior, which as it happens some
> other function X does, *might* in fact be considered broken. Remember to
> distinguish these cases; to make your case, you must address the
> specific behavior which is not being handled correctly.
>
Ok, in PHP keys in the form key[] are turned into arrays. And vice
versa. This is completely valid data to pass around, and MediaWiki does
support ($wgRequest->getArray(...);) and make use of this behavior
(Namely Special:Userrights). And additionally this kind of behavior is
something which would be beneficially used for extensions like
MultiUpload or UserRightsList which deal with a variable amount of data
input.

wfArrayToCgi does not support this behavior, but http_build_query does.
All input that wfArrayToCgi can handle correctly (Without printing
"Array" into the query string) http_build_query can handle, but the
reverse is not true.
>> The Object is just a side product of Array support.
>>
>
> Tossing in objects is a totally different ballgame. This isn't a case
> where arrays & objects sensibly can or should be treated the same way.
>
>
>> The wfBuildQuery's second parameter was merely for compatibility with
>> what wfArrayToCgi was doing.
>>
>
> There's no particular need for a wfBuildQuery() at all, or for changing
> the acceptable inputs to wfArrayToCgi().
>
>
> Go back to your core issue: allowing Title::get*Url() functions to
> accept associated arrays of data to build query strings from, in
> addition to raw query strings (for backwards compatibility).
>
> Look at what that actually requires, and concentrate on that.
>
Ok, I can make use of http_build_query to build the strings correctly,
but it's going to need a wrapper because of the & vs. &
I can't make use of or extend wfArrayToCgi without using parse_str
because of the multiple input. So that will probably just be excluded.
> - -- brion vibber (brion @ wikimedia.org)
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.8 (Darwin)
> Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
>
> iEYEARECAAYFAkgot1AACgkQwRnhpk1wk47whQCgrtUZKc4946yp37otDov11z+5
> /4UAn3TtSfTCgztKLusvbf+jYQAYqEDS
> =a8TE
> -----END PGP SIGNATURE-----
>
> _______________________________________________
> Wikitech-l mailing list
> Wikitech-l[at]lists.wikimedia.org
> https://lists.wikimedia.org/mailman/listinfo/wikitech-l
>
~Daniel Friesen(Dantman) of:
-The Gaiapedia (http://gaia.wikia.com)
-Wikia ACG on Wikia.com (http://wikia.com/wiki/Wikia_ACG)
-and Wiki-Tools.com (http://wiki-tools.com)

_______________________________________________
Wikitech-l mailing list
Wikitech-l[at]lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

brion at wikimedia

May 13, 2008, 9:45 AM

Post #12 of 16 (237 views)
Permalink

Re: r34541 and r34542 [In reply to]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

DanTMan wrote:
>> Ok, I can make use of http_build_query to build the strings correctly,
>> but it's going to need a wrapper because of the & vs. &

In cases where native PHP functions' behavior is unreliable, buggy, or
inconsistent, we tend to reimplement them ourselves. Sounds like this
might be such a case. Why build a wrapper when it's trivial to just
implement the function consistently?

>> I can't make use of or extend wfArrayToCgi without using parse_str
>> because of the multiple input. So that will probably just be excluded.

This is a non-sequitur -- parse_str() doesn't have any relevance to
wfArrayToCgi() at all, as it does something totally unrelated to our
requirement:

> Ok, in PHP keys in the form key[] are turned into arrays. And vice
> versa. This is completely valid data to pass around, and MediaWiki does
> support ($wgRequest->getArray(...);) and make use of this behavior
> (Namely Special:Userrights). And additionally this kind of behavior is
> something which would be beneficially used for extensions like
> MultiUpload or UserRightsList which deal with a variable amount of data
> input.

So, pay no attention to parse_str().

- -- brion vibber (brion @ wikimedia.org)
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.8 (Darwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkgpxZMACgkQwRnhpk1wk46XqgCcD/mjvwqIyrvTNfiSyukF1K6+
IT8AoLf7O2lntd1sH4QZ5pBEe5MGZwby
=I3S5
-----END PGP SIGNATURE-----

_______________________________________________
Wikitech-l mailing list
Wikitech-l[at]lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

dan_the_man at telus

May 14, 2008, 12:15 AM

Post #13 of 16 (235 views)
Permalink

Re: r34541 and r34542 [In reply to]

Brion Vibber wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> DanTMan wrote:
>
>>> Ok, I can make use of http_build_query to build the strings correctly,
>>> but it's going to need a wrapper because of the & vs. &
>>>
>
> In cases where native PHP functions' behavior is unreliable, buggy, or
> inconsistent, we tend to reimplement them ourselves. Sounds like this
> might be such a case. Why build a wrapper when it's trivial to just
> implement the function consistently?
>
Performance and reliability? php built functions with lots of repetition
and recursion do have a fair bit more load than core functions, I
thought I remembered someone noting the php driven attempts being
slower. And it doesn't always work out unless you grasp 100% what is
actually being done by the function, as a testament wfArrayToCgi was
basically an attempt at duplicating what PHP does, but when it was
created the developers around likely did not understand how PHP behaved
with Arrays and thus that was not handled in the function.

But ok, I can scrounge up some code from the comments on the PHP manual
pages and tweak them into a viable use wfBuildQuery function which does
not deal with ampersands the way PHP does and also handles strings.
(While I'm at it, I'll check out what type of urlencoding function we
actually use in MediaWiki and see if I should switch the one being used
to a cleaner one). I could even create a wfParseQuery to match the
wfBuildQuery, it's likely that extension developers may end up wanting
to do that, and it'll be good to avoid having anyone go and use the
broken PHP functions.
>>> I can't make use of or extend wfArrayToCgi without using parse_str
>>> because of the multiple input. So that will probably just be excluded.
>>>
>
> This is a non-sequitur -- parse_str() doesn't have any relevance to
> wfArrayToCgi() at all, as it does something totally unrelated to our
> requirement:
>
The issue was the multiple inputs. We want to allow string/aray input,
but that does not work with multiple input unless we parse any strings
that come along. So if we want to avoid parsing strings, we can't
upgrade an old function. That's just what I was meaning.
>> Ok, in PHP keys in the form key[] are turned into arrays. And vice
>> versa. This is completely valid data to pass around, and MediaWiki does
>> support ($wgRequest->getArray(...);) and make use of this behavior
>> (Namely Special:Userrights). And additionally this kind of behavior is
>> something which would be beneficially used for extensions like
>> MultiUpload or UserRightsList which deal with a variable amount of data
>> input.
>>
>
> So, pay no attention to parse_str().
>
> - -- brion vibber (brion @ wikimedia.org)
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.8 (Darwin)
> Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
>
> iEYEARECAAYFAkgpxZMACgkQwRnhpk1wk46XqgCcD/mjvwqIyrvTNfiSyukF1K6+
> IT8AoLf7O2lntd1sH4QZ5pBEe5MGZwby
> =I3S5
> -----END PGP SIGNATURE-----
>
> _______________________________________________
> Wikitech-l mailing list
> Wikitech-l[at]lists.wikimedia.org
> https://lists.wikimedia.org/mailman/listinfo/wikitech-l
>
~Daniel Friesen(Dantman) of:
-The Gaiapedia (http://gaia.wikia.com)
-Wikia ACG on Wikia.com (http://wikia.com/wiki/Wikia_ACG)
-and Wiki-Tools.com (http://wiki-tools.com)

_______________________________________________
Wikitech-l mailing list
Wikitech-l[at]lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

Simetrical+wikilist at gmail

May 14, 2008, 6:25 AM

Post #14 of 16 (234 views)
Permalink

Re: r34541 and r34542 [In reply to]

On Wed, May 14, 2008 at 3:15 AM, DanTMan <dan_the_man[at]telus.net> wrote:
> Performance and reliability? php built functions with lots of repetition
> and recursion do have a fair bit more load than core functions, I
> thought I remembered someone noting the php driven attempts being
> slower.

"Premature optimization is the root of all evil." There is absolutely
no reason to care about whether a trivial URL-building function is
fast or slow unless it demonstrably becomes a bottleneck. The issue
should not even be considered. We have profiling in place and are
able to focus our optimization efforts on actual bottlenecks.
Micro-optimizations in little-used parts of the code should not be
contemplated.

_______________________________________________
Wikitech-l mailing list
Wikitech-l[at]lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

brion at wikimedia

May 14, 2008, 8:24 AM

Post #15 of 16 (235 views)
Permalink

Re: r34541 and r34542 [In reply to]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

DanTMan wrote:
> Brion Vibber wrote:
> DanTMan wrote:
>
>>>>> Ok, I can make use of http_build_query to build the strings correctly,
>>>>> but it's going to need a wrapper because of the & vs. &
>>>>>
> In cases where native PHP functions' behavior is unreliable, buggy, or
> inconsistent, we tend to reimplement them ourselves. Sounds like this
> might be such a case. Why build a wrapper when it's trivial to just
> implement the function consistently?
>
>> Performance and reliability? php built functions with lots of repetition
>> and recursion do have a fair bit more load than core functions, I
>> thought I remembered someone noting the php driven attempts being
>> slower. And it doesn't always work out unless you grasp 100% what is
>> actually being done by the function, as a testament wfArrayToCgi was
>> basically an attempt at duplicating what PHP does, but when it was
>> created the developers around likely did not understand how PHP behaved
>> with Arrays and thus that was not handled in the function.

Ok, let's go over the basics here. :)

1) http://c2.com/cgi/wiki?YouArentGonnaNeedIt

wfArrayToCgi() *predates* http_build_query().

It was not meant to be a general-purpose replacement for a function that
didn't exist at the time, so has no need to replicate *every* feature
and behavior quirk of it.

wfArrayToCgi() was created to fill a specific need (easier construction
of paging links with options for MediaWiki's query pages), which at the
time did not include arbitrary array data (something we use only rarely,
and not in the forms it was originally created for).

As there was no requirement to implement an unneeded feature, it was not
included at the time.

If it becomes necessary, it can be easily added.

2) http://c2.com/cgi/wiki?PrematureOptimization

wfArrayToCgi() is not in a performance-critical code path.

Unless you can show by profiling that it becomes a performance problem,
there's no need to reflexively attempt to make it super-duper-mega-fast
at the expense of predictable behavior.

If using http_build_query() means unreliable behavior or jumping through
extra hoops to make it work as advertised, then it may be better to
avoid it.

Make your code work first, elegant second, and fast third.

- -- brion vibber (brion @ wikimedia.org)
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.8 (Darwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkgrBDoACgkQwRnhpk1wk44+lgCffKm8jVGUWVbWaGQFzZvlAswa
EXUAn2mqKLye+2vwJpEEBg7SHPVuCwQO
=nF1v
-----END PGP SIGNATURE-----

_______________________________________________
Wikitech-l mailing list
Wikitech-l[at]lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

dan_the_man at telus

May 14, 2008, 9:17 AM

Post #16 of 16 (234 views)
Permalink

Re: r34541 and r34542 [In reply to]

Brion Vibber wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> DanTMan wrote:
>
>> Brion Vibber wrote:
>> DanTMan wrote:
>>
>>
>>>>>> Ok, I can make use of http_build_query to build the strings correctly,
>>>>>> but it's going to need a wrapper because of the & vs. &
>>>>>>
>>>>>>
>> In cases where native PHP functions' behavior is unreliable, buggy, or
>> inconsistent, we tend to reimplement them ourselves. Sounds like this
>> might be such a case. Why build a wrapper when it's trivial to just
>> implement the function consistently?
>>
>>
>>> Performance and reliability? php built functions with lots of repetition
>>> and recursion do have a fair bit more load than core functions, I
>>> thought I remembered someone noting the php driven attempts being
>>> slower. And it doesn't always work out unless you grasp 100% what is
>>> actually being done by the function, as a testament wfArrayToCgi was
>>> basically an attempt at duplicating what PHP does, but when it was
>>> created the developers around likely did not understand how PHP behaved
>>> with Arrays and thus that was not handled in the function.
>>>
>
> Ok, let's go over the basics here. :)
>
>
> 1) http://c2.com/cgi/wiki?YouArentGonnaNeedIt
>
> wfArrayToCgi() *predates* http_build_query().
>
> It was not meant to be a general-purpose replacement for a function that
> didn't exist at the time, so has no need to replicate *every* feature
> and behavior quirk of it.
>
> wfArrayToCgi() was created to fill a specific need (easier construction
> of paging links with options for MediaWiki's query pages), which at the
> time did not include arbitrary array data (something we use only rarely,
> and not in the forms it was originally created for).
>
> As there was no requirement to implement an unneeded feature, it was not
> included at the time.
>
> If it becomes necessary, it can be easily added.
>
>
> 2) http://c2.com/cgi/wiki?PrematureOptimization
>
> wfArrayToCgi() is not in a performance-critical code path.
>
> Unless you can show by profiling that it becomes a performance problem,
> there's no need to reflexively attempt to make it super-duper-mega-fast
> at the expense of predictable behavior.
>
> If using http_build_query() means unreliable behavior or jumping through
> extra hoops to make it work as advertised, then it may be better to
> avoid it.
>
> Make your code work first, elegant second, and fast third.
>
wfArrayToCgi isn't, but throw in recursion over arrays ;) heh...
But at any rate, I already have a good solid wfBuildQuery, and
wfParseQuery functions build in PHP instead of as wrappers.
I'll commit them at some point with the code to let the Title functions
allow Array input.
> - -- brion vibber (brion @ wikimedia.org)
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.8 (Darwin)
> Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
>
> iEYEARECAAYFAkgrBDoACgkQwRnhpk1wk44+lgCffKm8jVGUWVbWaGQFzZvlAswa
> EXUAn2mqKLye+2vwJpEEBg7SHPVuCwQO
> =nF1v
> -----END PGP SIGNATURE-----
>
> _______________________________________________
> Wikitech-l mailing list
> Wikitech-l[at]lists.wikimedia.org
> https://lists.wikimedia.org/mailman/listinfo/wikitech-l
>
~Daniel Friesen(Dantman) of:
-The Gaiapedia (http://gaia.wikia.com)
-Wikia ACG on Wikia.com (http://wikia.com/wiki/Wikia_ACG)
-and Wiki-Tools.com (http://wiki-tools.com)

_______________________________________________
Wikitech-l mailing list
Wikitech-l[at]lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

Index | Next | Previous | View Threaded

Interested in having your list archived? Contact lists@gossamer-threads.com