Unhardcode "/w" from the path to api.php (for non-Wikimedia wikis) #4
Conversation
See https://phabricator.wikimedia.org/T297337 /w/api.php is only correct if $wgScriptPath = "/w". Non-Wikimedia wikis can have a different value here. Extension:Graph will be modified to provide correct "scriptPath" to this library via the options.
|
@nyurik : Any chance you could take a look? |
| @@ -116,6 +116,9 @@ class VegaWrapper2 { | |||
| const host = urlObj.wiki ? urlObj.wiki : options.domain; | |||
| const sanitizedHost = this.sanitizeHost(host); | |||
|
|
|||
| // Path to api.php, e.g. "/w" for Wikimedia projects. | |||
| const scriptPath = options.scriptPath !== undefined ? options.scriptPath : '/w' | |||
There was a problem hiding this comment.
I am not certain, but wouldn't this be simpler? I might be a bit rusty on my javascript though. Also, needs a semicolon.
| const scriptPath = options.scriptPath !== undefined ? options.scriptPath : '/w' | |
| const scriptPath = options.scriptPath ?? '/w'; |
There was a problem hiding this comment.
?? would also allow for null (which I wouldn't think would be a problem here), but is indeed otherwise equivalent; however, depending on your versions of JavaScript support, this "nullish coalescing operator" was added more recently in JavaScript. Still, is now pretty good: https://caniuse.com/mdn-javascript_operators_nullish_coalescing .
There was a problem hiding this comment.
You could also do options.scriptPath || '/w'; i think
There was a problem hiding this comment.
That would unfortunately choose the default when the script path was the empty string, which I imagine could be a problem here.
There was a problem hiding this comment.
I don't think an empty string is valid - it must start with a /, or else it won't work
There was a problem hiding this comment.
oh, oops, you are right.... I think the better path would be to introduce a full API endpoint url rather than a part of it. E.g. apiEndpoint = "/w/api.php" ? This would allow the proper override, wouldn't it? I just feel string concat magic is a bit more ... unpredictable
There was a problem hiding this comment.
I don't have any strong opinions about it myself, but I can see your point. Not sure if the submitter, @edwardspec , has any opinions about it...
There was a problem hiding this comment.
Doesn't matter to me. Feel free to change the syntax if needed.
|
Thanks very much! Do you think you could put out a release too, so the graph extension can offer this fix to users? |
|
ok, did a minor update, pushed and released as 0.6.0 |
See https://phabricator.wikimedia.org/T297337
/w/api.php is only correct if
$wgScriptPath == "/w". Non-Wikimedia wikis can have a different value here.Extension:Graph will be modified to provide correct "scriptPath" to this library via the options.