-
Notifications
You must be signed in to change notification settings - Fork 6
Mediawiki backend routing #20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| private static $callbackUrlTail; | ||
|
|
||
| public static function getVersionedMediawikiBackendHost() { | ||
| $host = getenv('PLATFORM_MW_BACKEND_HOST'); // default fallback |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like having a fallback. I wonder if we actually want to introduce a "we're mid updates flag" that we turn on and only in that case we do we do the API lookup.
Not for this PR though!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, as these requests probably won't get cached, as nginx does. But also shouldnt be a problem, as OAuth requests shouldn't happen too often I suppose
| 'Host: ' . $_SERVER['SERVER_NAME'], | ||
| ]; | ||
|
|
||
| $client = curl_init('http://' . self::platformApiBackendHost . '/backend/ingress/getWikiVersionForDomain?domain=' . $_SERVER['SERVER_NAME']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was looking at this whole curl thing and then trying to parse the headers of response and tried to see if there was a less complicated way. I think an alternative could be:
https://www.php.net/manual/en/function.get-headers.php
Does this look like a suitable alternative to you? I think we'd set url parameter to 'http://' . self::platformApiBackendHost . '/backend/ingress/getWikiVersionForDomain?domain=' . $_SERVER['SERVER_NAME'] and probably associative to true.
I think we'd then be able to directly access X-Version: header from the array. Does that sound right? I didn't go and try this.
Not blocking if you don't like this approach for some reason
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
incredible, how did i not know about this function? seems much more solid to me, thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hahah! I didn't know about it either; I went hunting because I was scared to click approve on the string parsing of headers and all the curl setup I didn't really understand quickly without a ton of tests :P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah it wasnt very readable. Could you have a look now again pls?
Co-authored-by: Thomas Arrow <tarrow@users.noreply.github.com>
tarrow
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me; in the absence of a test system let see if this works on staging 👀
https://phabricator.wikimedia.org/T407504
An example of how the routing to the right mediawiki version backend could happen via the API in magnustools.
You can test this via wbstack/quickstatements#187