Page MenuHomePhabricator

Review and deploy ApiSandbox extension on Wikimedia Cluster
Closed, ResolvedPublic

Description

Review and deploy ApiSandbox extension on mediawikiwiki!


Version: unspecified
Severity: enhancement

Details

Reference
bz28816

Event Timeline

bzimport raised the priority of this task from to Normal.Nov 21 2014, 11:33 PM
bzimport set Reference to bz28816.
Reedy created this task.May 4 2011, 7:58 PM
MaxSem added a comment.May 4 2011, 8:07 PM

Thanks, Sam.
One clarification: this should be just first step, ApiSandbox makes most sense when used on target wiki, where it takes into account all installed extensions and configuration options. Script writers should be able to test stuff on their home wikis.

Reedy added a comment.May 4 2011, 8:09 PM

Didn't really think of that...

I'd guess deploying it everywhere isn't going to be a major issue, you're not going to be bringing any performance problems, and as long as your JS is halfway reasonable ;)

Max, what is still outstanding to be done to the ApiSandbox? I remember you saying there was something, but I can't find it anywhere...

The only big problem with it is that it currently supports only selected module's parameters, so you can't use titles=foo or generator=bar. It requires some refactoring to do that. Otherwise, it works and already can edit pages with ot, for example.

All the problems are resolved, ready for review.

brion added a comment.Nov 28 2011, 3:02 PM

Assigning to self for review.

brion added a comment.Nov 28 2011, 3:36 PM

Created attachment 9571
Screenshot showing JSON result wider than page

It's easy to end up with long/wide results; this list query pushed beyond the size of the page and requires horizontal scrolling to find the vertical scrollbar of the result area.

May be best to limit the width of the result area and make sure it does its own horizontal scrollbar if it needs one.

Also, if the result area could expand *vertically* that would eliminate another inner vertical scrollbar. :)

Attached:

brion added a comment.Nov 28 2011, 3:53 PM

A few more things found during review testing:

Consider pushing 'query' up to the start of the action= box; all the easiest stuff to explore is in there, and it's probably going to be most peoples' desired starting point.

See if you can make hitting 'enter' while in various text fields trigger the query. This can probably be done by making sure everthing's wrapped in a <form> and hooking its 'submit' event. Prevent default behavior to make sure it doesn't do anything else. :)

The request URL is nice for copy-pasting to a simple templatized URL, but I find a lot of my API usage is actually from calls to $.ajax or some other HTTP client library that accepts data parameters as a hashmap. Being able to show the selected parameters as JavaScript-style object literals would be a help when developing gadgets and extensions:

/trunk/api.php?action=query&list=search&format=json&srsearch=main&srlimit=10

$.ajax('/trunk/api.php', {

data: {
  action: "query",
  list: "search",
  format: "json",
  srsearch: "main",
  srlimit" 10
}

});

brion added a comment.Nov 28 2011, 4:15 PM

A few more little notes:

UiBuilder.createInputs doesn't escape the field names when building HTML. While this shouldn't be a problem, it feels sketchy especially since the field names are received from the API and probably aren't being validated here.

UiBuilder.setHelp could fail interestingly if literal "$1" occurs in the help URL. Should escape input strings for the regex replacement here?

UiBuilder.input doesn't escape names etc. There's also a bit that doesn't escape a value var, which here is fixed but could change in the future to a parameter; safer to make sure it's escaped.

getRequestData() doesn't %-escape field names. Shouldn't matter, but you never know.

smartEscape() isn't very clear about what structures it's pretty-printing (appears to be 'areas of indentation' and 'indented lines' but the regexes are hard to read, and it wouldn't hurt to add a comment to that effect).

updateQueryInfo() is missing a 'var' local var definition for 'data', and sets a global variable instead.

There are a bunch of 'for (var x in obj)' loops:

for ( var prop in data.paraminfo ) {

this is unsafe if object prototypes have been modified by some JS libraries. Recommend using $.map or $.each() here, which already covers the necessary logic. (otherwise use if(data.paraminfo.hasOwnProperty(prop)) on each iteration).

merge() appears to be unused; it also looks like $.extend() can accept multiple arguments, making it unnecessary.

(In reply to comment #9)

There are a bunch of 'for (var x in obj)' loops:

for ( var prop in data.paraminfo ) {

this is unsafe if object prototypes have been modified by some JS libraries.
Recommend using $.map or $.each() here, which already covers the necessary
logic. (otherwise use if(data.paraminfo.hasOwnProperty(prop)) on each
iteration).

Our coding conventions generally consider for..in loops on objects safe. If a library modifies Object.prototype, then that library is profoundly evil and we shouldn't use it. for..in loops on *arrays*, however, are not acceptable.

Reedy added a comment.Nov 30 2011, 8:06 PM

All done and live!

Form submit on enter works now too. The problem with result box growing with its content vertically is that you'll have to scroll all the way to the bottom to get to the horizontal scrollbox if the content overflows horizontally.