morebits: remove Morebits.queryString #725

siddharthvp · 2019-10-12T14:44:18Z

Morebits.queryString is redundant to mw.util.getParamValue(), mw.util.getUrl() and jQuery.param(). All existing usages are easily replaced.
Specifically,

Morebits.queryString.get() and Morebits.queryString.exists() are redundant to mw.util.getParamValue()
Morebits.queryString.create() is redundant to mw.util.getUrl() and jQuery.param()

While queryString can be also used for constructing query string objects and getting/setting param values in them, this functionality is not used anywhere, nor are there any reasonable potential usecases.

This is untested, for now.

siddharthvp · 2019-10-17T07:45:22Z

It's also worth noting that the whole Morebits.queryString class is redundant to the native JS URLSearchParams interface (not available in IE, though).

Amorymeltzer · 2019-11-03T20:33:13Z

Haven't taken a look at the code yet, but wholeheartedly agree with this. I think it was the fluff stuff I was doing earlier this year that made me really wonder why we still bothered with queryString.

siddharthvp · 2019-11-08T17:49:37Z

morebits.js

+				.replace(/^(.*?)(&token=[^&]*)(.*)/, '$1$3$2') // shunt token from middle of query string to the end
+				.replace(/^(token=[^&]*)&(.*)/, '$2&$1'); // shunt token from beginning of query string to the end
+		}
+


I wrote this initially as

if (this.query.token) { var token = this.query.token; delete this.query.token; this.query.token = token; }

followed by $.param(this.query.token), but the delete operator is purported to cause performance impacts in V8, and isn't used anywhere else in Twinkle. Anyway, the two lines of ugly regex is much better than the 25 lines of ugly code that was there originally in Morebits.queryString.create!

Much nicer! The original is certainly over-complicated, but I think casting into an array is indeed the generic way to do it (sans jQuery and morebits of course). Either way, this should work for our specific purposes, so w00t!

Could we make it even tighter, though, by relying on the fact that tokens end in +\? Would something like the following be sufficient?

queryString = $.param(this.query).replace(/^(.*?)(token=[^%]+%2B%5C)&(.*)/, '$1$3&$2')

It:

matches until %

only matches if something is present (not sure what happens if there's an empty token parameter isn't last, but that shouldn't happen)

removes the trailing & and places one before the terminal token:

if token is first, the ? remains for the new first parameter

if token is in the middle, token's & remains for the upcoming parameter

if token is last, there won't be a terminal & to match and there's nothing to do anyway!

Great work in condensing it down to a single regex replace!

(token=[^%]+%2B%5C)

not so sure whether this is an improvement. While a nice idea, it seems better to me to leave it as

(token=[^&]*)

$.param(this.query) encodes the URI components so & can't occur as part of the token itself. So encountering an & means that the token part has ended. So we aren't really tightening up anything right?

~~NOTE: The code here doesn't account for the other features of Morebits.queryString.create - omitting parameters with value undefined and converting array parameters to |-delimited strings.~~

~~This needs to be resolved before merge.~~ Now done.

So that'd be queryString = $.param(this.query).replace(/^(.*?)(token=[^&]+)&(.*)/, '$1$3&$2'), right? (+ rather than *)

meh, using * means an empty token= also gets moved to the end, which is better right?

Amorymeltzer · 2019-12-19T11:56:19Z

Not sure I'll have time before the holidays, but just some history as I start clearing the way:

@azatoth first added Jan 2007
Initially, seems like the motivation was for fluff
mw.util.getParamValue available in the initial version of mediawiki.util in Oct 2010, circa 1.17
mw.util.getUrl began life as mw.util.wikiGetLink soon after, and was renamed circa 1.23
jQuery added to mediawiki in 1.16/1.17 at v1.3.2/1.4.2, respectively

Amorymeltzer · 2019-12-19T12:16:49Z

Is it worth attempting to deprecate these first? Maybe swapping in their equivalents would be easiest too start, and catch any edge cases. bI haven't yet checked usages elsewhere, but if there are none as you say @siddharthvp, then maybe that's overkill/assigning too much importance.

siddharthvp · 2019-12-19T15:40:09Z

18 usages in userspace:

User:Gigs/delsort - no one uses it + still uses sajax for API calls (so there's no way this can work)
User:Livitup/SimpleVote2012-code.js (4 users) - can be fixed by changing

var query = new Morebits.queryString( anchors[i].href.split('?',2)[1].toString() );
var link = query.get('title');

to

var link = anchors[i].href.match(/\btitle=([^&]*)/)[1];

if at all it worked before.

User:This,_that_and_the_other/SimpleVote2012-code.js - fork of Livitup's version above. Unused.
Remaining are all unused copies of Twinkle code.

I don't think there's any usage worth giving deprecation warnings for.

Amorymeltzer · 2020-01-06T18:21:50Z

modules/twinklebatchprotect.js

-		query.gapprefix = Morebits.queryString.exists('from') ? Morebits.string.toUpperCaseFirstChar(Morebits.queryString.get('from').replace('+', ' ')) :
-			Morebits.string.toUpperCaseFirstChar($('input[name=prefix]').val());
+		query.gapnamespace = mw.util.getParamValue('namespace') || $('select[name=namespace]').val();
+		query.gapprefix = mw.util.getParamValue('prefix') || $('input[name=prefix]').val();


Wow! from is used to paginate, while prefix is the actual prefix. That is, prefix defines the list (e.g. prefix=A) while from defines what is shown on the user's screen (e.g. prefix=A&from=Amory_Lovins). batchprotect has been busted on non-first-page results for a decade, very lame!

Amorymeltzer

Looks pretty good to me, @siddharthvp! As above, I think this is good to do; we're not attempting to support old MW versions, and with with jQuery and mw.util a lot of this is unnecessary. Maybe there's places we don't want to remove things from the morebits library, but I'm fine with queryString going. cc @MusikAnimal, @atlight, and @azatoth in case any of you feel strongly or want to review.

The only thing I do think might be worth holding onto is the Morebits.queryString function itself, or at least something like it. I'm not familiar with an easy way using jQuery or mw.util to build an object with each parameter (e.g. new Morebits.queryString(string).params), am I wrong on that? It's not used by Twinkle, but that feels useful and fairly low overhead. See below

Amorymeltzer · 2020-01-06T19:04:43Z

morebits.js

+				.replace(/^(.*?)(&token=[^&]*)(.*)/, '$1$3$2') // shunt token from middle of query string to the end
+				.replace(/^(token=[^&]*)&(.*)/, '$2&$1'); // shunt token from beginning of query string to the end
+		}
+


Much nicer! The original is certainly over-complicated, but I think casting into an array is indeed the generic way to do it (sans jQuery and morebits of course). Either way, this should work for our specific purposes, so w00t!

Could we make it even tighter, though, by relying on the fact that tokens end in +\? Would something like the following be sufficient?

queryString = $.param(this.query).replace(/^(.*?)(token=[^%]+%2B%5C)&(.*)/, '$1$3&$2')

It:

matches until %

only matches if something is present (not sure what happens if there's an empty token parameter isn't last, but that shouldn't happen)

removes the trailing & and places one before the terminal token:

if token is first, the ? remains for the new first parameter

if token is in the middle, token's & remains for the upcoming parameter

if token is last, there won't be a terminal & to match and there's nothing to do anyway!

morebits.js

siddharthvp · 2020-01-07T04:39:43Z

The only thing I do think might be worth holding onto is the Morebits.queryString function itself, or at least something like it. I'm not familiar with an easy way using jQuery or mw.util to build an object with each parameter (e.g. new Morebits.queryString(string).params), am I wrong on that? It's not used by Twinkle, but that feels useful and fairly low overhead.

new mw.Uri(string).query does do that, and the mw.Uri dependency is pretty cheap. The only trouble with it is that it needs a full URL and won't accept just a querystring.

Do you still think that this is needed @Amorymeltzer?

Amorymeltzer · 2020-01-07T11:20:01Z

new mw.Uri(string).query does do that, and the mw.Uri dependency is pretty cheap.

💯 TIL! And what dependency, is it not in core?

The only trouble with it is that it needs a full URL and won't accept just a querystring.

That's not an issue for me. I was honestly more interested in the default behavior using the current location string, as I thought it could be handy to be able to easily iterate on each of the parameters. It should only make a difference if trying to parse a pseudo link on the page that had the parameter string but not the full url (i.e. someone wrote "You can just append ?query=action&titles=Macbeth to the en.wikipedia.org but use /w/index.php instead of /wiki/" which is, uh, not a usecase. Otherwise, it's a full url, the default from location.href, an object we already own (making it pointless), or we can just toss it behind mw.util.getUrl`. All easy.

Do you still think that this is needed @Amorymeltzer?

Nope! 👍

Amorymeltzer · 2020-01-15T20:13:26Z

@siddharthvp With https://github.com/azatoth/twinkle/compare/dde1faa4143ac42ce8b788034243186e89d1ae1a..5bd60477a2f829cedeaeaae741d628dce77cbc3b am I right that you're done working on this, and I can give it a final review? Looking forward to it!

siddharthvp · 2020-01-17T05:01:12Z

@Amorymeltzer yeah 👍 sorry if was being confusing above.

Amorymeltzer

I worked out all the merge conflicts locally, so I think this is ready to go with one minor change! Whoo!

morebits.js

Amorymeltzer

One more bug that needs fixing. The this.query issue I'm fairly confident about, but wouldn't mind your confirmation/testing that this is the best way to deal with this for now.

Amorymeltzer · 2020-01-23T11:20:33Z

morebits.js

+			} else if (val !== undefined) {
+				return encodeURIComponent(i) + '=' + encodeURIComponent(val);
+			}
+		}).join('&').replace(/^(.*?)(token=[^&]*)&(.*)/, '$1$3&$2');


Okay there's actually another issue here @siddharthvp — for queries with intoken (e.g. before editing) this leaves the in behind for another parameter. Not good. We're not going to overhaul the token system right now, so for now I think this should do it:

Suggested change

}).join('&').replace(/^(.*?)(token=[^&]*)&(.*)/, '$1$3&$2');

}).join('&').replace(/^(.*?)((?:in)?token=[^&]*)&(.*)/, '$1$3&$2');

But I'm not 100% it's good enough.

Actually, I think that's technically backwards; we don't want to move intoken (well, I suppose it doesn't really matter) so it'd be better to do something like either:

Suggested change

}).join('&').replace(/^(.*?)(token=[^&]*)&(.*)/, '$1$3&$2');

}).join('&').replace(/^(.*?)[&\?]?(token=[^&]*)&(.*)/, '$1$3&$2');

or

Suggested change

}).join('&').replace(/^(.*?)(token=[^&]*)&(.*)/, '$1$3&$2');

}).join('&').replace(/^(.*?)([^in]token=[^&]*)&(.*)/, '$1$3&$2');

How about using \b ? It has zero-length match, so I think that should be ideal.

Mmm, yes, I think that's probably right/best/parsimonious

appears to work fine, pushed.

Is there a difference between \b(token... and (\btoken...? Off hand I'm not thinking there should be here.

\b has a zero-length match, so no.

Yeah - I was wondering more about ZWSP and exotics like that, I curious how they "counted." Turns out they're \S and `\W!

Morebits.queryString is redundant to mw.util.getParamValue(), mw.util.getUrl() and jQuery.param(). All existing usages are easily replaced. Specifically, - Morebits.queryString.get() and Morebits.queryString.exists() are redundant to mw.util.getParamValue() - Morebits.queryString.create() is redundant to mw.util.getUrl() and jQuery.param() While queryString can be also used for constructing query string objects and getting/setting param values in them, this functionality is not used anywhere, nor are there any reasonable potential usecases.

PrefixIndex URLs use `prefix` parameter, not a `from` parameter. Also, mw.util.getParamValue takes care of converting `+` to space, (though queryString also does that since f0866be; before that commit this was a bug) Also, use of Morebits.toUpperCaseFirstChar is unnecessary, that is taken care of by the API.

Fixes made!

Amorymeltzer · 2020-01-23T11:58:38Z

I'll do some more testing later today, but I think this is good to go!

Amorymeltzer

Will merge locally, but 🎉

Morebits.queryString is redundant to mw.util.getParamValue(), mw.util.getUrl() and jQuery.param(). All existing usages are easily replaced. Specifically, - Morebits.queryString.get() and Morebits.queryString.exists() are redundant to mw.util.getParamValue() - Morebits.queryString.create() is redundant to mw.util.getUrl() and jQuery.param() While queryString can be also used for constructing query string objects and getting/setting param values in them, this functionality is not used anywhere, nor are there any reasonable potential usecases. Co-authored-by: Siddharth VP <siddharthvp@gmail.com> Co-authored-by: Amory Meltzer <Amorymeltzer@gmail.com>

Amorymeltzer · 2020-01-23T20:20:31Z

Merged as 37d6b67 and bf1919f

Fixes wikimedia-gadgets#913, same basic issue as wikimedia-gadgets#627/wikimedia-gadgets#628: if a user has "Do not show page content below diffs" selected in Special:Preferences, wgRevisionId is set to 0 on diffs. While wikimedia-gadgets#628 took care of that for the links on diff pages themselves, trying to rollback from a contribs page was left unfixed. Those links load the diff page then `auto`matically revert, so will run into the same issue. This essentialy reverts back to the check before 3e5ec5b (wikimedia-gadgets#561), albeit by using `mw.util.getParamValue` rather than the no-longer-existing `Morebits.queryString.get` (wikimedia-gadgets#725).

Lost in between adding JSON support (19435c4/wikimedia-gadgets#493) and removing `queryString` (37d6b67/wikimedia-gadgets#725)

Fixes wikimedia-gadgets#913, same basic issue as wikimedia-gadgets#627/wikimedia-gadgets#628: if a user has "Do not show page content below diffs" selected in Special:Preferences, wgRevisionId is set to 0 on diffs. While wikimedia-gadgets#628 took care of that for the links on diff pages themselves, trying to rollback from a contribs page was left unfixed. Those links load the diff page then `auto`matically revert, so will run into the same issue. This essentialy reverts back to the check before 3e5ec5b (wikimedia-gadgets#561), albeit by using `mw.util.getParamValue` rather than the no-longer-existing `Morebits.queryString.get` (wikimedia-gadgets#725).

Lost in between adding JSON support (19435c4/wikimedia-gadgets#493) and removing `queryString` (37d6b67/wikimedia-gadgets#725)

Amorymeltzer added the Module: morebits The morebits.js library label Oct 15, 2019

Amorymeltzer self-assigned this Nov 3, 2019

siddharthvp force-pushed the qs branch from fcc84a0 to dde1faa Compare November 8, 2019 17:42

siddharthvp commented Nov 8, 2019

View reviewed changes

Amorymeltzer added this to the January 2020 update milestone Nov 12, 2019

Amorymeltzer reviewed Jan 6, 2020

View reviewed changes

Amorymeltzer added the bug label Jan 6, 2020

Amorymeltzer added the Module: batch The three batch modules and deprod label Jan 7, 2020

siddharthvp force-pushed the qs branch from dde1faa to 5bd6047 Compare January 11, 2020 11:10

Amorymeltzer requested changes Jan 23, 2020

View reviewed changes

morebits.js Outdated Show resolved Hide resolved

Amorymeltzer previously requested changes Jan 23, 2020

View reviewed changes

siddharthvp added 2 commits January 23, 2020 17:09

siddharthvp force-pushed the qs branch from 5bd6047 to 8052750 Compare January 23, 2020 11:39

Amorymeltzer mentioned this pull request Jan 23, 2020

Add select2 library (v4.0.12) to better handle options in warn and xfd #812

Merged

Amorymeltzer approved these changes Jan 23, 2020

View reviewed changes

Amorymeltzer closed this Jan 23, 2020

Amorymeltzer mentioned this pull request Apr 7, 2020

fluff: Fix revert coming from contribs if diffonly is used #914

Merged

Amorymeltzer added a commit to Amorymeltzer/twinkle that referenced this pull request Jun 28, 2020

morebits: fix ability to query in JSON

a14f5a1

Lost in between adding JSON support (19435c4/wikimedia-gadgets#493) and removing `queryString` (37d6b67/wikimedia-gadgets#725)

Amorymeltzer mentioned this pull request Jun 28, 2020

morebits: fix ability to query in JSON #1007

Merged

wiki-ST47 pushed a commit to wiki-ST47/twinkle that referenced this pull request Sep 2, 2020

morebits: fix ability to query in JSON

b57e9e0

Lost in between adding JSON support (19435c4/wikimedia-gadgets#493) and removing `queryString` (37d6b67/wikimedia-gadgets#725)

siddharthvp deleted the qs branch October 22, 2020 20:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

morebits: remove Morebits.queryString #725

morebits: remove Morebits.queryString #725

siddharthvp commented Oct 12, 2019

siddharthvp commented Oct 17, 2019

Amorymeltzer commented Nov 3, 2019

siddharthvp Nov 8, 2019

Amorymeltzer Jan 6, 2020

siddharthvp Jan 10, 2020

siddharthvp Jan 10, 2020 •

edited

Loading

Amorymeltzer Jan 11, 2020

siddharthvp Jan 11, 2020

Amorymeltzer commented Dec 19, 2019 •

edited

Loading

Amorymeltzer commented Dec 19, 2019

siddharthvp commented Dec 19, 2019 •

edited

Loading

Amorymeltzer Jan 6, 2020 •

edited

Loading

Amorymeltzer left a comment •

edited

Loading

Amorymeltzer Jan 6, 2020

siddharthvp commented Jan 7, 2020

Amorymeltzer commented Jan 7, 2020

Amorymeltzer commented Jan 15, 2020

siddharthvp commented Jan 17, 2020

Amorymeltzer left a comment

Amorymeltzer left a comment

Amorymeltzer Jan 23, 2020 •

edited

Loading

Amorymeltzer Jan 23, 2020 •

edited

Loading

siddharthvp Jan 23, 2020

Amorymeltzer Jan 23, 2020

siddharthvp Jan 23, 2020

Amorymeltzer Jan 23, 2020 •

edited

Loading

siddharthvp Jan 23, 2020

Amorymeltzer Jan 23, 2020

Amorymeltzer commented Jan 23, 2020

Amorymeltzer left a comment

Amorymeltzer commented Jan 23, 2020

	}).join('&').replace(/^(.?)(token=[^&])&(.*)/, '$1$3&$2');
	}).join('&').replace(/^(.?)((?:in)?token=[^&])&(.*)/, '$1$3&$2');

	}).join('&').replace(/^(.?)(token=[^&])&(.*)/, '$1$3&$2');
	}).join('&').replace(/^(.?)[&\?]?(token=[^&])&(.*)/, '$1$3&$2');

	}).join('&').replace(/^(.?)(token=[^&])&(.*)/, '$1$3&$2');
	}).join('&').replace(/^(.?)([^in]token=[^&])&(.*)/, '$1$3&$2');

morebits: remove Morebits.queryString #725

morebits: remove Morebits.queryString #725

Conversation

siddharthvp commented Oct 12, 2019

siddharthvp commented Oct 17, 2019

Amorymeltzer commented Nov 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siddharthvp Jan 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Amorymeltzer commented Dec 19, 2019 • edited Loading

Amorymeltzer commented Dec 19, 2019

siddharthvp commented Dec 19, 2019 • edited Loading

Amorymeltzer Jan 6, 2020 • edited Loading

Choose a reason for hiding this comment

Amorymeltzer left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siddharthvp commented Jan 7, 2020

Amorymeltzer commented Jan 7, 2020

Amorymeltzer commented Jan 15, 2020

siddharthvp commented Jan 17, 2020

Amorymeltzer left a comment

Choose a reason for hiding this comment

Amorymeltzer left a comment

Choose a reason for hiding this comment

Amorymeltzer Jan 23, 2020 • edited Loading

Choose a reason for hiding this comment

Amorymeltzer Jan 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Amorymeltzer Jan 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Amorymeltzer commented Jan 23, 2020

Amorymeltzer left a comment

Choose a reason for hiding this comment

Amorymeltzer commented Jan 23, 2020

siddharthvp Jan 10, 2020 •

edited

Loading

Amorymeltzer commented Dec 19, 2019 •

edited

Loading

siddharthvp commented Dec 19, 2019 •

edited

Loading

Amorymeltzer Jan 6, 2020 •

edited

Loading

Amorymeltzer left a comment •

edited

Loading

Amorymeltzer Jan 23, 2020 •

edited

Loading

Amorymeltzer Jan 23, 2020 •

edited

Loading

Amorymeltzer Jan 23, 2020 •

edited

Loading