Last modified: 2011-12-15 18:01:49 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T30816, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 28816 - Review and deploy ApiSandbox extension on Wikimedia Cluster
Review and deploy ApiSandbox extension on Wikimedia Cluster
Status: RESOLVED FIXED
Product: Wikimedia
Classification: Unclassified
Extension setup (Other open bugs)
unspecified
All All
: Normal enhancement (vote)
: ---
Assigned To: Brion Vibber
: patch-need-review
Depends on:
Blocks: 31235
  Show dependency treegraph
 
Reported: 2011-05-04 19:58 UTC by Sam Reed (reedy)
Modified: 2011-12-15 18:01 UTC (History)
4 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments
Screenshot showing JSON result wider than page (267.94 KB, image/png)
2011-11-28 15:36 UTC, Brion Vibber
Details

Description Sam Reed (reedy) 2011-05-04 19:58:57 UTC
Review and deploy ApiSandbox extension on mediawikiwiki!
Comment 1 Max Semenik 2011-05-04 20:07:04 UTC
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.
Comment 2 Sam Reed (reedy) 2011-05-04 20:09:42 UTC
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 ;)
Comment 3 Sam Reed (reedy) 2011-09-21 00:15:24 UTC
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...
Comment 4 Max Semenik 2011-09-21 01:23:44 UTC
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.
Comment 5 Max Semenik 2011-10-15 20:02:07 UTC
All the problems are resolved, ready for review.
Comment 6 Brion Vibber 2011-11-28 15:02:02 UTC
Assigning to self for review.
Comment 7 Brion Vibber 2011-11-28 15:36:50 UTC
Created attachment 9571 [details]
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. :)
Comment 8 Brion Vibber 2011-11-28 15:53:24 UTC
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
   }
});
Comment 9 Brion Vibber 2011-11-28 16:15:58 UTC
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.
Comment 10 Roan Kattouw 2011-11-30 12:45:58 UTC
(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.
Comment 11 Sam Reed (reedy) 2011-11-30 20:06:16 UTC
All done and live!
Comment 12 Max Semenik 2011-12-15 18:01:49 UTC
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.

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links