Page MenuHomePhabricator

mw.uri.new behaves differently from mw.uri.fullUrl and related functions for query table keys with multiple values
Open, MediumPublic

Description

In the debug console, this code:

=tostring( mw.uri.fullUrl( "Test", { foo = { "bar", "baz" } } ) )

gives me the following output:

http://127.0.0.1:8080/w/index.php?title=Test&foo%5B1%5D=bar&foo%5B2%5D=baz

Instead, I would expect this:

http://127.0.0.1:8080/w/index.php?title=Test&foo=bar&foo=baz

I see the same effect with mw.uri.localUrl and mw.uri.canonicalUrl as well.

However, I see it works fine with mw.uri.new:

=tostring( mw.uri.new{ host = "www.mediawiki.org", path = "/w/index.php", query = { foo = { "bar", "baz" } } } )
//www.mediawiki.org/w/index.php?foo=bar&foo=baz

It looks like something odd is happening when the query array is passed through to wfArrayToCgi via Title.php. I'm not sure where the best place is to make the fix though.

Event Timeline

MrStradivarius raised the priority of this task from to Medium.
MrStradivarius updated the task description. (Show Details)
MrStradivarius added a subscriber: MrStradivarius.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 18 2015, 12:36 PM
Anomie added a subscriber: Anomie.Mar 18 2015, 2:53 PM

There should be consistency, yes. But if this is going to be fixed in Scribunto, it'll probably be fixed by making mw.uri.buildQueryString behave like MediaWiki's PHP methods rather than by reimplementing all the PHP methods.

Ah, now I see. My use case was in trying to construct a URL containing preload text parameters, which use URLs that look like https://www.mediawiki.org/w/index.php?...&preloadparams[]=foo&preloadparams[]=bar.

I made the mistake of passing in the key preloadparams[] instead of just preloadparams, which caused me to output URL keys like preloadparams%5B%5D%5B1%5D. I now see that the [] marks the parameter as a duplicate instead of being part of the actual parameter name. It also seems that URLs with parameters like &preloadparams%5B1%5D=foo&preloadparams%5B2%5D=bar work just fine with preload text, so that solves my problem.

As for how to resolve the discrepancy, now that I see how the parameters actually work I agree that we should make Scribunto act like the PHP methods. As well as making mw.uri.new behave like mw.uri.fullUrl etc., we will also need to normalise the array indexes. At the moment duplicate parameters passed with mw.uri.fullUrl start counting from 1 (foo%5B1%5D=bar&foo%5B2%5D=baz), but using wfArrayToCgi with a PHP array will make parameters that start counting from 0 (foo%5B0%5D=bar&foo%5B1%5D=baz). Or maybe this detail isn't important? On testing the preload parameter link, the text between the square brackets was ignored altogether, and only the position of preloadparams[] in the URL was used to generate the parameter number.

MrStradivarius renamed this task from mw.uri.fullUrl and related functions garble query table keys that have multiple values to mw.uri.new behaves differently from mw.uri.fullUrl and related functions for query table keys with multiple values.Mar 21 2015, 5:03 AM
MrStradivarius set Security to None.

I now see that the [] marks the parameter as a duplicate instead of being part of the actual parameter name.

I don't know whether this syntax for query strings was actually "invented" by PHP, but it works just like PHP assignments: A query string with "&foo[]=bar" is roughly equivalent to $foo[] = 'bar'.

Or maybe this detail isn't important?

It's probably important in the general case, since receivers would be expecting array( 0 => 'bar', 1 => 'baz' ) rather than array( 1 => 'bar', 2 => 'baz' ) that foo%5B1%5D=bar&foo%5B2%5D=baz provides if they're not ignoring the keys entirely.

Change 199911 had a related patch set uploaded (by Mr. Stradivarius):
Standardise URI query fields with multiple values

https://gerrit.wikimedia.org/r/199911

Aklapper removed MrStradivarius as the assignee of this task.Jun 19 2020, 4:25 PM

This task has been assigned to the same task owner for more than two years. Resetting task assignee due to inactivity, to decrease task cookie-licking and to get a slightly more realistic overview of plans. Please feel free to assign this task to yourself again if you still realistically work or plan to work on this task - it would be welcome!

For tips how to manage individual work in Phabricator (noisy notifications, lists of task, etc.), see https://phabricator.wikimedia.org/T228575#6237124 for available options.
(For the records, two emails were sent to assignee addresses before resetting assignees. See T228575 for more info and for potential feedback. Thanks!)

Aklapper removed a subscriber: Anomie.Oct 16 2020, 5:02 PM