-
Notifications
You must be signed in to change notification settings - Fork 4
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
fix: propagate status code from platform api when routing wiki requests #410
Changes from 1 commit
8ada66b
b7666ab
0b9240f
9e14373
0116f75
75c7bd9
41e6c57
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,26 +10,26 @@ class GlobalSet { | |
|
||
/** | ||
* @param string $requestDomain A request domain, or 'maint' Example: 'addshore-alpha.wiki.opencura.com' | ||
* @return bool was the global successfully set? | ||
* @return void | ||
*/ | ||
public static function forDomain( $requestDomain ) { | ||
public static function forDomain($requestDomain) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So the linting here looks "funny" because MediaWiki uses a very non-php standard code style. No objection to this being changed here since we don't lint this anyway but perhaps in future we should add linting to match the MediaWiki style to this weird codebase in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mean if we want a code style, then it should be applied everywhere. I just tried to make it consistent per function. Which is probably futile ... |
||
// Normalize the domain by setting to lowercase | ||
$requestDomain = strtolower($requestDomain); | ||
|
||
// If the domain is 'maint' then set the maint settings | ||
if($requestDomain === 'maint') { | ||
if ($requestDomain === 'maint') { | ||
self::forMaint(); | ||
return true; | ||
return; | ||
} | ||
|
||
$info = self::getCachedOrFreshInfo( $requestDomain ); | ||
$info = self::getCachedOrFreshInfo($requestDomain); | ||
|
||
if($info === null) { | ||
return false; | ||
$notFound = new \Exception("No wiki was found for domain $requestDomain", 404); | ||
throw $notFound; | ||
} | ||
|
||
self::setGlobal( $info ); | ||
return true; | ||
self::setGlobal($info); | ||
} | ||
|
||
/** | ||
|
@@ -120,16 +120,20 @@ private static function getInfoFromApi( $requestDomain ) { | |
curl_setopt($client, CURLOPT_HTTPHEADER, $headers); | ||
curl_setopt($client, CURLOPT_RETURNTRANSFER, true); | ||
curl_setopt( $client, CURLOPT_USERAGENT, "WBStack - MediaWiki - WBStackInfo::getInfoFromApi" ); | ||
|
||
$response = curl_exec($client); | ||
$responseCode = intval(curl_getinfo($client, CURLINFO_RESPONSE_CODE)); | ||
m90 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// TODO detect non 200 response here, and pass that out to the user as an error | ||
|
||
$info = WBStackInfo::newFromJsonString($response, $requestDomain); | ||
if (!$info) { | ||
if ($responseCode === 404) { | ||
return null; | ||
} | ||
|
||
if ($responseCode > 299) { | ||
throw new \Exception("Unexpected status code $responseCode from Platform API for domain $requestDomain.", $responseCode); | ||
} | ||
|
||
$info = WBStackInfo::newFromJsonString($response, $requestDomain); | ||
return $info; | ||
} | ||
|
||
} | ||
} |
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 think this makes sense and probably propagating the precise nature of the failure with an Exception.
In any case this probably makes sense and is then super clear that this is really a side effect only function.
Naturally trying to write this in a clean way is pretty hard since the desired behaviour is charging around a setting a load of globals