From b6dd2d8639f26983ce0570b8a90f1a6a888373e1 Mon Sep 17 00:00:00 2001 From: SpencerMalone Date: Thu, 8 Jan 2026 17:48:47 -0800 Subject: [PATCH] Fix up phpstan issues and formatting in templates and setup CI to run on generated example templates. --- .dagger/lint.go | 6 ++- phpstan.neon.dist | 7 +++ .../templates/global/TwirpError.php.tmpl | 17 ++---- .../templates/service/AbstractClient.php.tmpl | 52 ++++++++++++++++--- .../templates/service/Client.php.tmpl | 10 ++-- .../templates/service/JsonClient.php.tmpl | 10 ++-- .../templates/service/Server.php.tmpl | 47 ++++++----------- .../templates/service/_Service_.php.tmpl | 5 +- 8 files changed, 86 insertions(+), 68 deletions(-) diff --git a/.dagger/lint.go b/.dagger/lint.go index 2326a6c..48f01a2 100644 --- a/.dagger/lint.go +++ b/.dagger/lint.go @@ -43,10 +43,14 @@ func (m *Lint) Go() *dagger.Container { } func (m *Lint) Phpstan() *dagger.Container { + // Generate example files before running phpstan + source := m.Main.source(). + WithDirectory("example/generated", m.Main.Generate().Example()) + return dag.Phpstan(dagger.PhpstanOpts{ Version: phpstanVersion, PhpVersion: defaultPhpVersion, - }).Analyse(m.Main.source()) + }).Analyse(source) } func (m *Lint) PhpCsFixer() *dagger.Container { diff --git a/phpstan.neon.dist b/phpstan.neon.dist index ae5c972..e2e48e7 100644 --- a/phpstan.neon.dist +++ b/phpstan.neon.dist @@ -6,5 +6,12 @@ parameters: # checkMissingIterableValueType: false paths: - lib/src + - example/ + excludePaths: + analyse: + # These are generated by the protobuf php tooling, not us. + - example/generated/Twitch/Twirp/Example/Hat.php + - example/generated/Twitch/Twirp/Example/Size.php + - example/generated/GPBMetadata/Service.php bootstrapFiles: - lib/vendor/autoload.php diff --git a/protoc-gen-twirp_php/templates/global/TwirpError.php.tmpl b/protoc-gen-twirp_php/templates/global/TwirpError.php.tmpl index 259c2e0..41f299b 100644 --- a/protoc-gen-twirp_php/templates/global/TwirpError.php.tmpl +++ b/protoc-gen-twirp_php/templates/global/TwirpError.php.tmpl @@ -1,12 +1,13 @@ errorCode; } - /** - * {@inheritdoc} - */ public function setMeta(string $key, string $value): void { $this->meta[$key] = $value; } - /** - * {@inheritdoc} - */ public function getMeta(string $key): string { if (isset($this->meta[$key])) { @@ -58,9 +50,6 @@ final class TwirpError extends \Exception implements Error return ''; } - /** - * {@inheritdoc} - */ public function getMetaMap(): array { return $this->meta; diff --git a/protoc-gen-twirp_php/templates/service/AbstractClient.php.tmpl b/protoc-gen-twirp_php/templates/service/AbstractClient.php.tmpl index 97b150a..0e62346 100644 --- a/protoc-gen-twirp_php/templates/service/AbstractClient.php.tmpl +++ b/protoc-gen-twirp_php/templates/service/AbstractClient.php.tmpl @@ -1,6 +1,7 @@ addr; if (empty($this->prefix)) { - $url = $url.'/{{ $method | protoMethodFullName }}'; + $url .= '/{{ $method | protoMethodFullName }}'; } else { $url = $url.'/'.$this->prefix.'/{{ $method | protoMethodFullName }}'; } @@ -165,7 +166,7 @@ abstract class {{ .Service | phpServiceName .File }}AbstractClient return $this->twirpErrorFromIntermediary($statusCode, $msg, $location); } - $body = (string)$resp->getBody(); + $body = (string) $resp->getBody(); $rawError = json_decode($body, true); if ($rawError === null) { @@ -174,7 +175,19 @@ abstract class {{ .Service | phpServiceName .File }}AbstractClient return $this->twirpErrorFromIntermediary($statusCode, $msg, $body); } - $rawError = $rawError + ['code' => '', 'msg' => '', 'meta' => []]; + if (!is_array($rawError)) { + $msg = 'invalid type returned from server error response body'; + + return TwirpError::newError(ErrorCode::Internal, $msg); + } + + $rawError += ['code' => '', 'msg' => '', 'meta' => []]; + + if (!is_string($rawError['code'])) { + $msg = 'invalid type returned for error code.'; + + return TwirpError::newError(ErrorCode::Internal, $msg); + } if (ErrorCode::isValid($rawError['code']) === false) { $msg = 'invalid type returned from server error response: '.$rawError['code']; @@ -182,10 +195,33 @@ abstract class {{ .Service | phpServiceName .File }}AbstractClient return TwirpError::newError(ErrorCode::Internal, $msg); } + if (!is_string($rawError['msg'])) { + $msg = 'invalid type returned for error message.'; + + return TwirpError::newError(ErrorCode::Internal, $msg); + } + $error = TwirpError::newError($rawError['code'], $rawError['msg']); + if (!is_array($rawError['meta'])) { + $msg = 'invalid type returned for error meta.'; + + return TwirpError::newError(ErrorCode::Internal, $msg); + } + foreach ($rawError['meta'] as $key => $value) { - $error->setMeta($key, $value); + if (!is_string($key)) { + $msg = 'invalid type returned for error meta key.'; + + return TwirpError::newError(ErrorCode::Internal, $msg); + } + + if (!is_string($value)) { + $msg = 'invalid type returned for error meta value.'; + + return TwirpError::newError(ErrorCode::Internal, $msg); + } + $error->setMeta($key, $value); } return $error; @@ -230,7 +266,7 @@ abstract class {{ .Service | phpServiceName .File }}AbstractClient $error = TwirpError::newError($code, $msg); $error->setMeta('http_error_from_intermediary', 'true'); - $error->setMeta('status_code', (string)$status); + $error->setMeta('status_code', (string) $status); if ($this->isHttpRedirect($status)) { $error->setMeta('location', $bodyOrLocation); @@ -248,7 +284,7 @@ abstract class {{ .Service | phpServiceName .File }}AbstractClient protected function urlBase(string $addr): string { - $scheme = parse_url($addr, PHP_URL_SCHEME); + $scheme = parse_url($addr, \PHP_URL_SCHEME); // If parse_url fails, return the addr unchanged. if ($scheme === false) { diff --git a/protoc-gen-twirp_php/templates/service/Client.php.tmpl b/protoc-gen-twirp_php/templates/service/Client.php.tmpl index 075b855..c2f324f 100644 --- a/protoc-gen-twirp_php/templates/service/Client.php.tmpl +++ b/protoc-gen-twirp_php/templates/service/Client.php.tmpl @@ -1,6 +1,7 @@ serializeToString(); @@ -37,7 +35,7 @@ final class {{ .Service | phpServiceName .File }}Client extends {{ .Service | ph } try { - $out->mergeFromString((string)$resp->getBody()); + $out->mergeFromString((string) $resp->getBody()); } catch (GPBDecodeException $e) { throw $this->clientError('failed to unmarshal proto response', $e); } diff --git a/protoc-gen-twirp_php/templates/service/JsonClient.php.tmpl b/protoc-gen-twirp_php/templates/service/JsonClient.php.tmpl index e836c13..aea5d0e 100644 --- a/protoc-gen-twirp_php/templates/service/JsonClient.php.tmpl +++ b/protoc-gen-twirp_php/templates/service/JsonClient.php.tmpl @@ -1,6 +1,7 @@ serializeToJsonString(); @@ -37,7 +35,7 @@ final class {{ .Service | phpServiceName .File }}JsonClient extends {{ .Service } try { - $out->mergeFromJsonString((string)$resp->getBody()); + $out->mergeFromJsonString((string) $resp->getBody()); } catch (GPBDecodeException $e) { throw $this->clientError('failed to unmarshal json response', $e); } diff --git a/protoc-gen-twirp_php/templates/service/Server.php.tmpl b/protoc-gen-twirp_php/templates/service/Server.php.tmpl index fea75fa..aa46a66 100644 --- a/protoc-gen-twirp_php/templates/service/Server.php.tmpl +++ b/protoc-gen-twirp_php/templates/service/Server.php.tmpl @@ -1,6 +1,7 @@ {{ .Service.Desc.FullName }} */ -final class {{ .Service | phpServiceName .File }}Server implements - RequestHandlerInterface, - ServerWithPathPrefix +final class {{ .Service | phpServiceName .File }}Server implements RequestHandlerInterface, ServerWithPathPrefix { /** * A convenience constant that may identify URL paths. @@ -38,20 +37,11 @@ final class {{ .Service | phpServiceName .File }}Server implements */ public const PATH_PREFIX = '/twirp/{{ .Service.Desc.FullName }}/'; - /** - * @var ResponseFactoryInterface - */ - private $responseFactory; + private ResponseFactoryInterface $responseFactory; - /** - * @var StreamFactoryInterface - */ - private $streamFactory; + private StreamFactoryInterface $streamFactory; - /** - * @var {{ .Service | phpServiceName .File }} - */ - private $svc; + private {{ .Service | phpServiceName .File }} $svc; /** * @var ServerHooks @@ -104,6 +94,7 @@ final class {{ .Service | phpServiceName .File }}Server implements */ public function handle(ServerRequestInterface $req): ResponseInterface { + /** @var array $ctx */ $ctx = $req->getAttributes(); $ctx = Context::withPackageName($ctx, '{{ .File.Proto.GetPackage }}'); $ctx = Context::withServiceName($ctx, '{{ .Service.Desc.Name }}'); @@ -120,7 +111,7 @@ final class {{ .Service | phpServiceName .File }}Server implements return $this->writeError($ctx, $this->badRouteError($msg, $req->getMethod(), $req->getUri()->getPath())); } - list($prefix, $service, $method) = $this->parsePath($req->getUri()->getPath()); + [$prefix, $service, $method] = $this->parsePath($req->getUri()->getPath()); if ($service != '{{ .Service.Desc.FullName }}') { return $this->writeError($ctx, $this->noRouteError($req)); @@ -174,6 +165,8 @@ final class {{ .Service | phpServiceName .File }}Server implements return $this->writeError($ctx, $this->badRouteError($msg, $req->getMethod(), $req->getUri()->getPath())); } + // This is set by reference, so phpstan can't infer the contents + /** @phpstan-ignore foreach.emptyArray */ foreach ($respHeaders as $key => $value) { $resp = $resp->withHeader($key, $value); } @@ -192,14 +185,10 @@ final class {{ .Service | phpServiceName .File }}Server implements $ctx = $this->hook->requestRouted($ctx); $in = new {{ $inputType }}(); - $in->mergeFromJsonString((string)$req->getBody(), true); + $in->mergeFromJsonString((string) $req->getBody(), true); $out = $this->svc->{{ $method.Desc.Name }}($ctx, $in); - if ($out === null) { - return $this->writeError($ctx, TwirpError::newError(ErrorCode::Internal, 'received a null response while calling {{ $method.Desc.Name }}. null responses are not supported')); - } - $ctx = $this->hook->responsePrepared($ctx); } catch (GPBDecodeException $e) { return $this->writeError($ctx, TwirpError::newError(ErrorCode::Internal, 'failed to parse request json')); @@ -232,14 +221,10 @@ final class {{ .Service | phpServiceName .File }}Server implements $ctx = $this->hook->requestRouted($ctx); $in = new {{ $inputType }}(); - $in->mergeFromString((string)$req->getBody()); + $in->mergeFromString((string) $req->getBody()); $out = $this->svc->{{ $method.Desc.Name }}($ctx, $in); - if ($out === null) { - return $this->writeError($ctx, TwirpError::newError(ErrorCode::Internal, 'received a null response while calling {{ $method.Desc.Name }}. null responses are not supported')); - } - $ctx = $this->hook->responsePrepared($ctx); } catch (GPBDecodeException $e) { return $this->writeError($ctx, TwirpError::newError(ErrorCode::Internal, 'failed to parse request proto')); @@ -274,7 +259,7 @@ final class {{ .Service | phpServiceName .File }}Server implements $parts = explode('/', $path); if (count($parts) < 2) { - return ["", "", ""]; + return ['', '', '']; } $method = $parts[count($parts) - 1]; @@ -300,7 +285,7 @@ final class {{ .Service | phpServiceName .File }}Server implements private function badRouteError(string $msg, string $method, string $url): TwirpError { $e = TwirpError::newError(ErrorCode::BadRoute, $msg); - $e->setMeta('twirp_invalid_route', $method . ' ' . $url); + $e->setMeta('twirp_invalid_route', $method.' '.$url); return $e; } @@ -356,7 +341,7 @@ final class {{ .Service | phpServiceName .File }}Server implements $rawBody['meta'] = $e->getMetaMap(); } - $body = $this->streamFactory->createStream(json_encode($rawBody, \JSON_FORCE_OBJECT)); + $body = $this->streamFactory->createStream(json_encode($rawBody, \JSON_FORCE_OBJECT | \JSON_THROW_ON_ERROR)); return $this->responseFactory ->createResponse($statusCode) diff --git a/protoc-gen-twirp_php/templates/service/_Service_.php.tmpl b/protoc-gen-twirp_php/templates/service/_Service_.php.tmpl index 78eeec4..00debd0 100644 --- a/protoc-gen-twirp_php/templates/service/_Service_.php.tmpl +++ b/protoc-gen-twirp_php/templates/service/_Service_.php.tmpl @@ -1,6 +1,7 @@