Page MenuHomePhabricator

"new mw.Uri()" ignores options when using an empty first argument
Closed, ResolvedPublic

Description

When using new mw.Uri() with an empty first argument, the overrideKeys option is ignored.

For example on the page https://en.wikipedia.org/w/index.php?title=Main_Page&action=foobar&action=history :

// incorrect, getting an array
(new mw.Uri(null, {overrideKeys: true})).query;
Object { title: "Main_Page", action: Array[2] }

// correct
(new mw.Uri(location.href, {overrideKeys: true})).query;
Object { title: "Main_Page", action: "history" }

This is because when the first argument is empty, the constructor just returns a clone of the cached "defaultUri" instance, ignoring the options.

Code: mediawiki.Uri.js

Event Timeline

Od1n created this task.Feb 2 2017, 2:54 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFeb 2 2017, 2:54 PM
Od1n triaged this task as High priority.Feb 2 2017, 2:55 PM

@Od1n: Thanks for taking a look at the code! As you set the priority of this task, do you plan to work on a patch?

Od1n added a comment.EditedFeb 4 2017, 9:12 PM

Here is the simplest patch I could figure out:

@@ -179,7 +179,8 @@
 		 *  override each other (`true`) or automagically convert them to an array (`false`).
 		 */
 		function Uri( uri, options ) {
-			var prop,
+			var prop, hrefCur,
+				hasOptions = ( options !== undefined ),
 				defaultUri = getDefaultUri();

 			options = typeof options === 'object' ? options : { strictMode: !!options };
@@ -208,8 +209,12 @@
 						this.query = {};
 					}
 				}
+			} else if ( hasOptions ) {
+				// We didn't get a URI in the constructor, but we got options.
+				hrefCur = typeof documentLocation === 'string' ? documentLocation : documentLocation();
+				this.parse( hrefCur, options );
 			} else {
-				// If we didn't get a URI in the constructor, use the default one.
+				// We didn't get a URI or options in the constructor, use the default instance.
 				return defaultUri.clone();
 			}

<del>Though, that location.href may not be suitable. Refs bece0d14c125 and T74334.</del>

edit: updated patch to account for dynamic locations.

Od1n added a comment.Feb 7 2017, 2:19 PM

I think the patch should be fine as it is.

If empty options are specified (e.g. empty object {}), it also avoids the cache, while we could theoretically use the cache.
But it is an edge case and avoiding the cache is harmless. Changing this would significantly increase code complexity, not worth it.
A simple test against undefined to check if the argument is defined is just fine.

@Od1n: Whou, thank you a lot for giving this a shot!
You are very welcome to use developer access to submit the proposed code changes as a Git branch directly into Gerrit which makes it easier to review them quickly and provide feedback. If you don't want to set up Git/Gerrit, you can also use the Gerrit Patch Uploader. Thanks again!

Od1n added a comment.Feb 7 2017, 5:44 PM

Thanks for pointing me to the Gerrit Patch Uploader. Last time I used Gerrit, it was a real PITA and I don't want to touch it again, ever.

So, here are the submitted changes using the tool: https://gerrit.wikimedia.org/r/#/c/336443/

Od1n added a comment.Feb 7 2017, 6:01 PM

If the changes are approved, a test should be added in mediawiki.Uri.test.js. But I'm not enough at ease there…

Od1n added a subscriber: matmarex.Feb 7 2017, 6:29 PM

@matmarex, many thanks for your review on Gerrit :)

  • Good catch about the missing comma.
  • As I wrote above, I'm not at ease for writing the test…

You are welcome to resubmit it properly. I don't care about the authorship, as long as the bug is fixed :)

Thanks! If you could change the line "Phabricator: T157035" to "Bug: T157035" there will be automated linking, see https://www.mediawiki.org/wiki/Gerrit/Commit_message_guidelines :)

Od1n added a comment.Feb 8 2017, 2:10 PM

I dropped my Gerrit account after my last irksome experience with it, and don't plan to create a new account.

So I can't edit the patch I have uploaded using the tool above. It's all in your hands now.

Change 336443 had a related patch set uploaded (by Bartosz Dziewoński):
mw.Uri: Fix ignored options when using "new mw.Uri()" with first argument empty

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

So I can't edit the patch I have uploaded using the tool above. It's all in your hands now.

You can actually edit it the same way you uploaded it, as long as you keep the "Change-Id" identical in the commit message.

I tweaked it, though.

Od1n added a comment.Feb 8 2017, 8:13 PM

Many thanks!

(btw I fixed the test, using your "Change-Id" hint)

Change 336443 merged by jenkins-bot:
mediawiki.Uri: Don't ignore options param when using default uri

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

matmarex closed this task as Resolved.Feb 9 2017, 3:24 PM
matmarex removed a project: Patch-For-Review.
matmarex assigned this task to Od1n.

The change will be deployed to Wikimedia wikis next week, 14-16 February, per https://www.mediawiki.org/wiki/MediaWiki_1.29/Roadmap.

Change 337229 had a related patch set uploaded (by Gerrit Patch Uploader):
Add release note for Iae5edf99

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

Change 337229 merged by jenkins-bot:
Add release note for Iae5edf99

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