feat: use Guzzle and handle RequestErrors
#154
Conversation
| // phpcs:disable Generic.CodeAnalysis.UselessOverridingMethod.Found | ||
| /** | ||
| * Create and send an HTTP GET request. | ||
| * | ||
| * Use an absolute path to override the base path of the client, or a relative path to append to the base path of the client. | ||
| * The URL can contain the query string as well. | ||
| * | ||
| * @param UriInterface|string $uri | ||
| * @param array $options | ||
| * @return ResponseInterface | ||
| * @throws GuzzleException | ||
| * @throws request_error | ||
| * @throws options_form_validation_error | ||
| */ | ||
| public function get($uri, array $options = []): ResponseInterface { | ||
| return parent::get($uri, $options); | ||
| } | ||
|
|
||
| /** | ||
| * Create and send an HTTP POST request. | ||
| * | ||
| * Use an absolute path to override the base path of the client, or a relative path to append to the base path of the client. | ||
| * The URL can contain the query string as well. | ||
| * | ||
| * @param UriInterface|string $uri | ||
| * @param array $options | ||
| * @return ResponseInterface | ||
| * @throws GuzzleException | ||
| * @throws request_error | ||
| * @throws options_form_validation_error | ||
| */ | ||
| public function post($uri, array $options = []): ResponseInterface { | ||
| return parent::post($uri, $options); | ||
| } | ||
| // phpcs:enable |
There was a problem hiding this comment.
Wegen den @throws options_form_validation_error und @throws request_error. PhpStorm kennzeichnet diese Stellen zwar mit Exception 'request_error' is never thrown in the function, doch ohne diesen Docstring wird z.B. das request_error in
try {
return $this->client->post($uri, $options);
} catch (request_error $e) {/*...*/}grau hinterlegt und beim Hovern wird Exception 'request_error' is never thrown in the corresponding 'try' block angezeigt, mit der Option die gesamte 'catch clause' zu entfernen.
Der Linter schlägt in beiden Fällen aber nicht an.
Eine Überlegung wäre es auch die * @throws X_error nur in den Docstrings der Funktionen zu schreiben, wo get/post aufgerufen wird.
| if (is_null($value)) { | ||
| continue; | ||
| } | ||
|
|
||
| // Cast arrays to objects so that empty arrays get serialized to JSON objects, not arrays. | ||
| $transformed[$key] = is_array($value) ? (object) $value : $value; |
There was a problem hiding this comment.
I guess bei dem was wir aktuell senden passt das so, ich will nur anmerken, dass wir beide diese Annahmen ("null == serverseitiges default" und "keine Listen auf den obersten beiden Ebenen") vermutlich irgendwann mit neuen Requests brechen. Ich hätte es deshalb beim Caller gesehen, das sicherzustellen. Aber da es gerade stimmt kann ich es auch nicht wirklich beanstanden.
There was a problem hiding this comment.
Ja, das stimmt. Ich würde deshalb erstmal eine NOTE: ... im Docstring hinzufügen, wäre das ein Kompromiss?
| } catch (BadResponseException $e) { | ||
| if ($e->getResponse()->getStatusCode() == 404) { | ||
| // The static file was not found. | ||
| return null; | ||
| } | ||
|
|
There was a problem hiding this comment.
Hier sollte doch jetzt keine BadResponseException mehr ankommen, sondern ein request_error, oder? Und meine coding_exception unten ist auf jeden Fall weniger geil als den request_error wiederzuwerfen.
There was a problem hiding this comment.
Falls die statische Datei nicht gefunden wurde, wird direkt eine aiohttp.web.HTTPNotFound-Exception im Server geschmissen (siehe). Von der Server-Middleware werden diese Exceptions einfach gereraised ohne transformiert zu werden (siehe). Deshalb kommt hier eine BadResponseException und kein request_error an.
Da gebe ich dir Recht. Die Stelle sollte aber nur erreicht werden, wenn statt raise web.HTTPNotFound eine andere aiohttp.web.Exception geraised werden würde, die kein RequestError ist und das wäre eine "coding exception", da wir nur aiohttp.web.HTTPNotFound vorgesehen haben.
| }, | ||
| function (GuzzleException $exception) { | ||
| if (!($exception instanceof RequestException) || !$exception->hasResponse()) { | ||
| // TODO: also handle other guzzle exceptions like `ConnectException`? |
There was a problem hiding this comment.
Halte ich für sinnvoll, die Standard-Fehlermeldung ist doch eher kryptisch. Hat aber denke ich auch keine große Priorität.
There was a problem hiding this comment.
Dann würde ich das jetzt erstmal nur als TODO stehen lassen.
Benötigt für: #152
Folgende Änderungen habe ich in diesem PR vorgenommen:
RequestErroroderOptionsFormValidationErrorwird jeweils einrequest_erroroderoptions_form_validation_errorgeschmissen. Als Fallback wird einfach dieGuzzleExceptiongeschmissen.