From d11423b97d74fbd7604088fa91613dff0c8b6ef3 Mon Sep 17 00:00:00 2001 From: acoulton Date: Mon, 26 Oct 2026 16:35:20 +0000 Subject: [PATCH 1/7] ci: Run PHPStan at level 7 --- phpstan.dist.neon | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/phpstan.dist.neon b/phpstan.dist.neon index fdf92f2..720c16f 100644 --- a/phpstan.dist.neon +++ b/phpstan.dist.neon @@ -1,6 +1,6 @@ parameters: treatPhpDocTypesAsCertain: false - level: 6 + level: 7 paths: - src - test From 830bf02d28fd541ed09c6181b579c404fc5a3be6 Mon Sep 17 00:00:00 2001 From: acoulton Date: Mon, 26 Oct 2026 16:44:26 +0000 Subject: [PATCH 2/7] fix: small phpstan issues in test code Resolve issues with typehints and edge cases that don't represent any actual runtime / logic issues. --- test/unit/CSV/CSVWriterTest.php | 35 +++-- .../DeploymentConfig/DeploymentConfigTest.php | 4 +- test/unit/Logging/DefaultLogMetadataTest.php | 2 + .../StackdriverApplicationLoggerTest.php | 133 +++++++++--------- test/unit/Object/ConstantDirectoryTest.php | 1 + .../AbstractArrayRepositoryTest.php | 2 +- test/unit/StringEncoding/JSONTest.php | 2 +- 7 files changed, 99 insertions(+), 80 deletions(-) diff --git a/test/unit/CSV/CSVWriterTest.php b/test/unit/CSV/CSVWriterTest.php index e76761a..b168889 100644 --- a/test/unit/CSV/CSVWriterTest.php +++ b/test/unit/CSV/CSVWriterTest.php @@ -65,7 +65,7 @@ public function testItThrowsIfWritingAfterClosingFile(): void public function testOpenAcceptsAndAllowsWritingToExistingStreamResource(): void { - $file = fopen('php://memory', 'w'); + $file = $this->openMemoryStream(); try { $subj = $this->newSubject(); $subj->open($file); @@ -79,7 +79,7 @@ public function testOpenAcceptsAndAllowsWritingToExistingStreamResource(): void public function testItThrowsIfWritingToExternallyClosedResource(): void { - $file = fopen('php://memory', 'w'); + $file = $this->openMemoryStream(); try { $subj = $this->newSubject(); $subj->open($file); @@ -108,7 +108,7 @@ public function testItCanOpenAndWriteToFilename(): void public function testItWritesColumnHeadersBeforeFirstRow(): void { - $file = fopen('php://memory', 'w'); + $file = $this->openMemoryStream(); $subj = $this->newSubject(); $subj->open($file); $subj->write(['our' => 'data', 'is' => 'here']); @@ -124,7 +124,7 @@ public function testItWritesColumnHeadersBeforeFirstRow(): void public function testItDoesNotWriteColumnHeadersBeforeSubsequentRows(): void { - $file = fopen('php://memory', 'w'); + $file = $this->openMemoryStream(); $subj = $this->newSubject(); $subj->open($file); $subj->write(['our' => 'data', 'is' => 'here']); @@ -177,12 +177,12 @@ public static function providerQuotedHeaders(): Iterator /** * @param array $row - * @param array $options + * @param array{write_utf8_bom?: bool, quote_headers?: bool, eol?: string} $options */ #[DataProvider('providerQuotedHeaders')] public function testItOptionallyWritesColumnHeadersWithoutQuotes(array $row, array $options, string $expect): void { - $file = fopen('php://memory', 'w'); + $file = $this->openMemoryStream(); $subj = $this->newSubject(); $subj->open($file, $options); $subj->write($row); @@ -193,7 +193,7 @@ public function testItOptionallyWritesColumnHeadersWithoutQuotes(array $row, arr public function testItThrowsIfColumnHeadersContainCommaWhenUnquoted(): void { - $file = fopen('php://memory', 'w'); + $file = $this->openMemoryStream(); $subj = $this->newSubject(); $subj->open($file, ['quote_headers' => false]); $this->expectException(RuntimeException::class); @@ -233,11 +233,11 @@ public static function providerLineEndings(): Iterator ]; } - /** @param array $options */ + /** @param array{write_utf8_bom?: bool, quote_headers?: bool, eol?: string} $options */ #[DataProvider('providerLineEndings')] public function testItsLineEndingsCanBeConfigured(array $options, string $expect): void { - $file = fopen('php://memory', 'w'); + $file = $this->openMemoryStream(); $subj = $this->newSubject(); $subj->open($file, $options); $subj->write(['our big' => 'data', 'is' => 'here']); @@ -250,7 +250,7 @@ public function testItsLineEndingsCanBeConfigured(array $options, string $expect #[TestWith([false])] public function testItOptionallyWritesByteOrderMarkAtStartOfFile(bool $write_bom): void { - $file = fopen('php://memory', 'w'); + $file = $this->openMemoryStream(); $subj = $this->newSubject(); $subj->open($file, ['write_utf8_bom' => $write_bom]); $subj->write(['first' => 'row']); @@ -266,7 +266,7 @@ public function testItOptionallyWritesByteOrderMarkAtStartOfFile(bool $write_bom #[TestWith([['our' => 'things', 'went' => 'bad']])] public function testItThrowsIfSubsequentRowHeadersDoNotMatch(array $second_row): void { - $file = fopen('php://memory', 'w'); + $file = $this->openMemoryStream(); try { $subj = $this->newSubject(); $subj->open($file); @@ -280,7 +280,7 @@ public function testItThrowsIfSubsequentRowHeadersDoNotMatch(array $second_row): public function testItDoesNotCloseExternallyProvidedStream(): void { - $file = fopen('php://memory', 'w'); + $file = $this->openMemoryStream(); try { $subj = $this->newSubject(); $subj->open($file); @@ -306,6 +306,17 @@ protected function assertCSVContent(array $expect, $file): void $this->assertSame($expect, $actual, 'CSV content should match'); } + /** @return resource */ + private function openMemoryStream() + { + $file = fopen('php://memory', 'w'); + if ($file === false) { + throw new RuntimeException('Unable to open in-memory CSV stream'); + } + + return $file; + } + private function newSubject(): CSVWriter { return new CSVWriter(); diff --git a/test/unit/DeploymentConfig/DeploymentConfigTest.php b/test/unit/DeploymentConfig/DeploymentConfigTest.php index e1ac38c..b831ab5 100644 --- a/test/unit/DeploymentConfig/DeploymentConfigTest.php +++ b/test/unit/DeploymentConfig/DeploymentConfigTest.php @@ -247,7 +247,7 @@ public function testItsReadJsonAlwaysReturnsNullInStandalone(): void #[TestWith([1234.5, 1234.5])] public function testItsReadJsonReturnsJsonDecodedFileContentIfPresentOutsideStandalone(mixed $json, mixed $expect): void { - $path = $this->givenConfigDirWithFile('config/some_scalar', json_encode($json)); + $path = $this->givenConfigDirWithFile('config/some_scalar', json_encode($json, JSON_THROW_ON_ERROR)); $subject = $this->newSubjectWithEnvAndConfigDir( DeploymentConfig::DEV, $path @@ -318,7 +318,7 @@ private function newSubjectWithNoArgs(): DeploymentConfig return $creator(); } - private function givenConfigDirWithFile(string $path, string|bool $content): string + private function givenConfigDirWithFile(string $path, string $content): string { $vfs = vfsStream::setup('var'); $cfg_dir = vfsStream::newDirectory('config') diff --git a/test/unit/Logging/DefaultLogMetadataTest.php b/test/unit/Logging/DefaultLogMetadataTest.php index 565faa8..05f20a5 100644 --- a/test/unit/Logging/DefaultLogMetadataTest.php +++ b/test/unit/Logging/DefaultLogMetadataTest.php @@ -78,6 +78,7 @@ public function testItsRequestTracingDataAddsRandomRequestId(): void public function testItsRequestTracingDataAddsTraceIdWithRequestIdIfNoHeadersAvailable( ): void { $result = DefaultLogMetadata::requestTrace(); + $this->assertArrayHasKey('logging.googleapis.com/trace', $result); $this->assertSame( $result['context']['req'], $result['logging.googleapis.com/trace'], @@ -125,6 +126,7 @@ public function testItsRequestTracingDataAddsTraceIdWithFromHeaderAndOptionallyP string $expect, ): void { $result = DefaultLogMetadata::requestTrace($server, $project); + $this->assertArrayHasKey('logging.googleapis.com/trace', $result); $this->assertSame( $expect, $result['logging.googleapis.com/trace'], diff --git a/test/unit/Logging/StackdriverApplicationLoggerTest.php b/test/unit/Logging/StackdriverApplicationLoggerTest.php index 8c49935..d09f022 100644 --- a/test/unit/Logging/StackdriverApplicationLoggerTest.php +++ b/test/unit/Logging/StackdriverApplicationLoggerTest.php @@ -11,6 +11,7 @@ use Ingenerator\PHPUtils\Logging\StackdriverApplicationLogger; use Ingenerator\PHPUtils\StringEncoding\JSON; use InvalidArgumentException; +use Iterator; use org\bovigo\vfs\vfsStream; use org\bovigo\vfs\vfsStreamDirectory; use PHPUnit\Framework\Attributes\DataProvider; @@ -27,7 +28,6 @@ use function array_map; use function array_pop; use function array_shift; -use function call_user_func_array; use function explode; use function file_get_contents; use function http_response_code; @@ -43,7 +43,7 @@ final class StackdriverApplicationLoggerTest extends TestCase private string $log_stream; - /** @var array */ + /** @var list|callable(): array|LogMetadataProvider> */ private array $meta_args = []; public function testItIsInitialisable(): void @@ -66,17 +66,15 @@ public function testItThrowsIfItCannotWriteLogEntry(): void } /** - * @return array> + * @return Iterator>> */ - public static function provider_log_lines(): array + public static function provider_log_lines(): Iterator { - return [ - [ - ['This is my info'], - ], - [ - ['This info', 'Another msg'], - ], + yield [ + ['This is my info'], + ]; + yield [ + ['This info', 'Another msg'], ]; } @@ -136,14 +134,12 @@ public function testItAppendsLogsToExistingContentIfFileSpecified(): void } /** - * @return array + * @return Iterator> */ - public static function provider_severity_levels(): array + public static function provider_severity_levels(): Iterator { - return [ - [LogLevel::DEBUG, 'DEBUG'], - [LogLevel::EMERGENCY, 'EMERGENCY'], - ]; + yield [LogLevel::DEBUG, 'DEBUG']; + yield [LogLevel::EMERGENCY, 'EMERGENCY']; } #[DataProvider('provider_severity_levels')] @@ -259,6 +255,7 @@ public function getCallCount(): int public function testItSwallowsAndLogsAnyErrorsWhileRetrievingMetadataCallbacks(): void { + // @phpstan-ignore assign.propertyType (this test deliberately supplies malformed metadata callbacks) $this->meta_args = [ ['sources' => ['first_ok']], /* @phpstan-ignore argument.type (intentional, to make it have an error) */ @@ -316,8 +313,14 @@ public function testItAssignsIngeneratorTypeUnlessOverridden(array $context, str #[TestWith(['info', ['Some info']])] public function testItLogsWithSourceLocationAsImmediateCaller(string $log_method, array $args): void { - $expect_line = __LINE__ + 1; - call_user_func_array([$this->newSubject(), $log_method], $args); + $logger = $this->newSubject(); + if ($log_method === 'log') { + $expect_line = __LINE__ + 1; + $logger->log(...$args); + } else { + $expect_line = __LINE__ + 1; + $logger->info(...$args); + } $entry = $this->assertLoggedOneLine(); $this->assertSame( [ @@ -378,9 +381,9 @@ public function testItPublishesExceptionStringInContextWithoutIssues(): void } /** - * @return array + * @return Iterator> */ - public static function provider_exception_chains(): array + public static function provider_exception_chains(): Iterator { $e1 = new InvalidArgumentException('I am an exception', 102); $e1_line = __LINE__ - 1; @@ -390,43 +393,40 @@ public static function provider_exception_chains(): array $e3 = new TypeError('Stuff happened', 02, $e2); $e3_line = __LINE__ - 1; - - return [ + yield [ + $e1, [ - $e1, - [ - 'class' => InvalidArgumentException::class, - 'msg' => 'I am an exception', - 'code' => 102, - 'file' => __FILE__, - 'line' => $e1_line, - 'trace' => self::makeExpectedSanitisedTrace($e1), - ], + 'class' => InvalidArgumentException::class, + 'msg' => 'I am an exception', + 'code' => 102, + 'file' => __FILE__, + 'line' => $e1_line, + 'trace' => self::makeExpectedSanitisedTrace($e1), ], + ]; + yield [ + $e3, [ - $e3, - [ - 'class' => TypeError::class, - 'msg' => 'Stuff happened', + 'class' => TypeError::class, + 'msg' => 'Stuff happened', + 'code' => 02, + 'file' => __FILE__, + 'line' => $e3_line, + 'trace' => self::makeExpectedSanitisedTrace($e1), + 'previous' => [ + 'class' => RuntimeException::class, + 'msg' => 'It went wrong', 'code' => 02, 'file' => __FILE__, - 'line' => $e3_line, - 'trace' => self::makeExpectedSanitisedTrace($e1), + 'line' => $e2_line, + 'trace' => self::makeExpectedSanitisedTrace($e2), 'previous' => [ - 'class' => RuntimeException::class, - 'msg' => 'It went wrong', - 'code' => 02, + 'class' => InvalidArgumentException::class, + 'msg' => 'I am an exception', + 'code' => 102, 'file' => __FILE__, - 'line' => $e2_line, - 'trace' => self::makeExpectedSanitisedTrace($e2), - 'previous' => [ - 'class' => InvalidArgumentException::class, - 'msg' => 'I am an exception', - 'code' => 102, - 'file' => __FILE__, - 'line' => $e1_line, - 'trace' => self::makeExpectedSanitisedTrace($e3), - ], + 'line' => $e1_line, + 'trace' => self::makeExpectedSanitisedTrace($e3), ], ], ], @@ -522,19 +522,17 @@ public function testItsRequestLoggerLogsRequestAsIngeneratorRequestLogType(): vo } /** - * @return array + * @return Iterator> */ - public static function provider_request_status_level(): array - { - return [ - [200, 'INFO'], - [202, 'INFO'], - [400, 'NOTICE'], - [403, 'WARNING'], - [422, 'NOTICE'], - [500, 'ERROR'], - [502, 'ERROR'], - ]; + public static function provider_request_status_level(): Iterator + { + yield [200, 'INFO']; + yield [202, 'INFO']; + yield [400, 'NOTICE']; + yield [403, 'WARNING']; + yield [422, 'NOTICE']; + yield [500, 'ERROR']; + yield [502, 'ERROR']; } #[DataProvider('provider_request_status_level')] @@ -677,6 +675,9 @@ protected function assertLoggedJSONLines(): array } $content = file_get_contents($this->log_stream); + if ($content === false) { + throw new RuntimeException('Unable to read log stream '.$this->log_stream); + } $lines = explode("\n", $content); $this->assertSame('', array_pop($lines), 'Expect trailing newline after last message'); @@ -692,7 +693,11 @@ protected function assertLoggedOneLine(): array $entries = $this->assertLoggedJSONLines(); $this->assertCount(1, $entries, 'Expected a single log entry'); - return array_shift($entries); + $entry = array_shift($entries); + // Could only happen if PHP or PHPUnit was broken, we asserted a nonzero array above + $this->assertNotNull($entry, 'Expected a single log entry'); + + return $entry; } protected static function makeExpectedSanitisedTrace(Throwable $e): string diff --git a/test/unit/Object/ConstantDirectoryTest.php b/test/unit/Object/ConstantDirectoryTest.php index 1614269..b8e9e00 100644 --- a/test/unit/Object/ConstantDirectoryTest.php +++ b/test/unit/Object/ConstantDirectoryTest.php @@ -62,6 +62,7 @@ public function testItReturnsListOfConstantsWithPrefix(): void ); } + /** @param class-string $class */ protected function newSubject(string $class = self::class): ConstantDirectory { return ConstantDirectory::forClass($class); diff --git a/test/unit/Repository/AbstractArrayRepositoryTest.php b/test/unit/Repository/AbstractArrayRepositoryTest.php index c3521a9..070f135 100644 --- a/test/unit/Repository/AbstractArrayRepositoryTest.php +++ b/test/unit/Repository/AbstractArrayRepositoryTest.php @@ -235,7 +235,7 @@ protected static function getEntityBaseClass(): string return AnyEntity::class; } - /** @return list */ + /** @return array */ public function getEntities(): array { return $this->entities; diff --git a/test/unit/StringEncoding/JSONTest.php b/test/unit/StringEncoding/JSONTest.php index e1317c6..ac611b0 100644 --- a/test/unit/StringEncoding/JSONTest.php +++ b/test/unit/StringEncoding/JSONTest.php @@ -55,7 +55,7 @@ public function testItThrowsFromDecodeArrayIfValueNotArray(): void public function testDecodeArrayReturnsEmptyArrayForJsonNullInput(): void { - $this->assertSame([], JSON::decodeArray(json_encode(null))); + $this->assertSame([], JSON::decodeArray('null')); } /** From 974887092e74aa71d21b5061979fbf41a61196ee Mon Sep 17 00:00:00 2001 From: acoulton Date: Mon, 26 Oct 2026 16:46:21 +0000 Subject: [PATCH 3/7] fix: session_create_id can return false We should handle this and throw, rather than attempting to insert empty to the database (which would fail, but later). --- src/Session/MysqlSession.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/Session/MysqlSession.php b/src/Session/MysqlSession.php index 0c4c2a1..0a30bd3 100644 --- a/src/Session/MysqlSession.php +++ b/src/Session/MysqlSession.php @@ -109,6 +109,9 @@ public function open($save_path, $name): bool public function create_sid(): string { $session_id = session_create_id(); + if ($session_id === false) { + throw new RuntimeException('Unable to create session ID'); + } /* * The session does not exist, so create it in the DB From 2bd274c41282733310f3512a2beda4b930307ca3 Mon Sep 17 00:00:00 2001 From: acoulton Date: Mon, 26 Oct 2026 16:53:04 +0000 Subject: [PATCH 4/7] fix: Be stricter about valid inputs & failure conditions Throw on more invalid input, or failures of e.g. regexes. --- src/ArrayHelpers/AssociativeArrayUtils.php | 14 +++++++++++--- src/CSV/CSVWriter.php | 4 ++-- src/DateTime/DateTimeImmutableFactory.php | 13 ++++++++++++- .../ConfigMapDeclarationParser.php | 17 ++++++++++++++--- src/Encryption/CryptoBox/CryptoBoxPublicKey.php | 7 ++++++- src/Encryption/CryptoBox/CryptoBoxString.php | 7 ++++++- src/Object/ConstantDirectory.php | 2 ++ src/Random/ConsistentStringScrambler.php | 9 +++++++-- src/Repository/AbstractArrayRepository.php | 3 +++ 9 files changed, 63 insertions(+), 13 deletions(-) diff --git a/src/ArrayHelpers/AssociativeArrayUtils.php b/src/ArrayHelpers/AssociativeArrayUtils.php index f289530..c893fc0 100644 --- a/src/ArrayHelpers/AssociativeArrayUtils.php +++ b/src/ArrayHelpers/AssociativeArrayUtils.php @@ -8,6 +8,8 @@ namespace Ingenerator\PHPUtils\ArrayHelpers; +use InvalidArgumentException; + use function array_key_exists; use function array_keys; use function array_shift; @@ -120,12 +122,18 @@ public static function pathsToNested(array $input): array */ public static function setPath(array &$array, string|array $path, mixed $value, string $delimiter = '.'): void { - // The path has already been separated into keys - $keys = $path; - if (!is_array($path)) { + if (is_array($path)) { + $keys = $path; + } else { + if ($delimiter === '') { + throw new InvalidArgumentException('Path delimiter cannot be empty'); + } // Split the keys by delimiter $keys = explode($delimiter, $path); } + if ($keys === []) { + throw new InvalidArgumentException('Path must not be empty'); + } // Set current $array to inner-most array path while (count($keys) > 1) { diff --git a/src/CSV/CSVWriter.php b/src/CSV/CSVWriter.php index 3fe9b08..2ca0aba 100644 --- a/src/CSV/CSVWriter.php +++ b/src/CSV/CSVWriter.php @@ -104,7 +104,7 @@ public function close(): void } /** - * @param int[]|string[] $keys + * @param list $keys */ private function writeHeaders(array $keys): void { @@ -113,7 +113,7 @@ private function writeHeaders(array $keys): void } else { array_walk( $keys, - fn (string $str): true => !str_contains($str, ',') + fn (int|string $str): true => !str_contains((string) $str, ',') ?: throw new RuntimeException( "Column header `$str` cannot contain comma if headers are not quoted" ) diff --git a/src/DateTime/DateTimeImmutableFactory.php b/src/DateTime/DateTimeImmutableFactory.php index 3f5eb5b..4f58d90 100644 --- a/src/DateTime/DateTimeImmutableFactory.php +++ b/src/DateTime/DateTimeImmutableFactory.php @@ -40,6 +40,9 @@ public static function atMicrotime(float|string $microtime): DateTimeImmutable // First, a float with a .00 casts to string with no decimal point, which causes the createFromFormat to fail // on exact seconds. Use sprintf to ensure there's always a decimal present $dt = DateTimeImmutable::createFromFormat('U.u', sprintf('%.6F', $microtime)); + if (!$dt instanceof DateTimeImmutable) { + throw new InvalidArgumentException(sprintf('Could not parse microtime value %s', $microtime)); + } // Second, creating from unix timestamp of any kind always sets the TZ to UTC (*even* if you specify a timezone // in the constructor). @@ -95,6 +98,10 @@ public static function fromIso(string $value): DateTimeImmutable { // Cope with Z for Zulu time instead of +00:00 - PHP offers `p` for this, but that then doesn't accept '+00:00' $fixed_value = preg_replace('/Z/i', '+00:00', $value); + if ($fixed_value === null) { + // regex failed + throw new InvalidArgumentException('Failed to replace text in iso string'); + } // Pad / truncate milliseconds to 6 digits as that's the precision PHP can support // Regex is a bit dull here, but we need to be sure we can reliably find the (possibly absent) @@ -111,10 +118,14 @@ public static function fromIso(string $value): DateTimeImmutable .$matches['tz_prefix'], $fixed_value ); + if ($fixed_value === null) { + // regex failed + throw new InvalidArgumentException("Cannot replace parts in `$value` ISO string"); + } // Not using fromStrictFormat as I want to throw with the original value, not the parsed value $date = DateTimeImmutable::createFromFormat('!Y-m-d\TH:i:s.uP', $fixed_value); - if (DateString::isoMS($date ?: null) === $fixed_value) { + if ($date instanceof DateTimeImmutable && DateString::isoMS($date) === $fixed_value) { return $date; } throw new InvalidArgumentException("`$value` cannot be parsed as a valid ISO date-time"); diff --git a/src/DeploymentConfig/ConfigMapDeclarationParser.php b/src/DeploymentConfig/ConfigMapDeclarationParser.php index c8a639c..93aa3b5 100644 --- a/src/DeploymentConfig/ConfigMapDeclarationParser.php +++ b/src/DeploymentConfig/ConfigMapDeclarationParser.php @@ -2,6 +2,8 @@ namespace Ingenerator\PHPUtils\DeploymentConfig; +use function array_values; + class ConfigMapDeclarationParser { /** @@ -60,10 +62,19 @@ protected function validateDeclaration(mixed $declaration): array throw InvalidConfigException::invalidMapDeclaration('must be indexed array with keys 0, 1'); } - if (is_string($declaration[0])) { - $declaration[0] = [$declaration[0]]; + $environments = $declaration[0]; + if (is_string($environments)) { + $environments = [$environments]; + } + if (!is_array($environments)) { + throw InvalidConfigException::invalidMapDeclaration('environments must be a string or array of strings'); + } + foreach ($environments as $environment) { + if (!is_string($environment)) { + throw InvalidConfigException::invalidMapDeclaration('environments must be a string or array of strings'); + } } - return $declaration; + return [array_values($environments), $declaration[1]]; } } diff --git a/src/Encryption/CryptoBox/CryptoBoxPublicKey.php b/src/Encryption/CryptoBox/CryptoBoxPublicKey.php index 1e690ba..ce096ff 100644 --- a/src/Encryption/CryptoBox/CryptoBoxPublicKey.php +++ b/src/Encryption/CryptoBox/CryptoBoxPublicKey.php @@ -27,8 +27,13 @@ public static function fromString(string $exported): static throw new InvalidArgumentException('Invalid public key string format'); } + $public_key = Base64Url::decode($matches['key_b64']); + if ($public_key === false) { + throw new InvalidArgumentException('Invalid public key encoding'); + } + return new self( - public_key: Base64Url::decode($matches['key_b64']), + public_key: $public_key, // public key is not secret, no need for constant-time encoding keypair_id: $matches['keypair_id'] ); diff --git a/src/Encryption/CryptoBox/CryptoBoxString.php b/src/Encryption/CryptoBox/CryptoBoxString.php index 695d3c6..65c7c39 100644 --- a/src/Encryption/CryptoBox/CryptoBoxString.php +++ b/src/Encryption/CryptoBox/CryptoBoxString.php @@ -25,8 +25,13 @@ public static function fromString(string $exported): static throw new InvalidArgumentException('Invalid ciphertext string format'); } + $ciphertext = Base64Url::decode($matches['ciphertext_b64']); + if ($ciphertext === false) { + throw new InvalidArgumentException('Invalid ciphertext encoding'); + } + return new self( - ciphertext: Base64Url::decode($matches['ciphertext_b64']), + ciphertext: $ciphertext, // ciphertext is not secret, no need for constant-time encoding keypair_id: $matches['keypair_id'] ); diff --git a/src/Object/ConstantDirectory.php b/src/Object/ConstantDirectory.php index 1498c7b..7775941 100644 --- a/src/Object/ConstantDirectory.php +++ b/src/Object/ConstantDirectory.php @@ -20,10 +20,12 @@ final class ConstantDirectory */ private array $constants; + /** @param class-string $class_name */ private function __construct(private readonly string $class_name) { } + /** @param class-string $class */ public static function forClass(string $class): static { return new self($class); diff --git a/src/Random/ConsistentStringScrambler.php b/src/Random/ConsistentStringScrambler.php index 6d1a6dc..84ca73b 100644 --- a/src/Random/ConsistentStringScrambler.php +++ b/src/Random/ConsistentStringScrambler.php @@ -5,6 +5,7 @@ use InvalidArgumentException; use Random\Engine\Mt19937; use Random\Randomizer; +use RuntimeException; use function array_filter; use function array_values; @@ -24,7 +25,11 @@ public function shuffleWords(?string $input, string $random_seed): ?string } // Break the string into words (and return null if there is no content / only whitespace) - $words = array_filter(preg_split('/\s+/', $input ?? '')); + $parts = preg_split('/\s+/', $input ?? ''); + if ($parts === false) { + throw new RuntimeException('Unable to split input into words'); + } + $words = array_values(array_filter($parts)); if ($words === []) { return null; @@ -60,6 +65,6 @@ private function seededShuffle(array $items, int $seed): array $items[$rnd] = $old_item_i; } - return $items; + return array_values($items); } } diff --git a/src/Repository/AbstractArrayRepository.php b/src/Repository/AbstractArrayRepository.php index 480e485..00183d1 100644 --- a/src/Repository/AbstractArrayRepository.php +++ b/src/Repository/AbstractArrayRepository.php @@ -79,6 +79,9 @@ public static function withList(array $entity_data) $entities = []; foreach ($entity_data as $entity) { if (!$entity instanceof $entity_class) { + if (!is_array($entity)) { + throw new InvalidArgumentException('Expected '.$entity_class.' or array, got '.get_debug_type($entity)); + } $entity = static::stubEntity($entity); } $entities[] = $entity; From f09035ed2352fade711cf0ec39455d40ca9c742b Mon Sep 17 00:00:00 2001 From: acoulton Date: Mon, 26 Oct 2026 16:59:39 +0000 Subject: [PATCH 5/7] fix: Handle errors accessing files and databases --- src/CSV/CSVWriter.php | 6 +++++- src/DeploymentConfig/ConfigValueDecrypter.php | 6 +++++- src/Mutex/DbBackedMutexWrapper.php | 21 +++++++++++++++---- 3 files changed, 27 insertions(+), 6 deletions(-) diff --git a/src/CSV/CSVWriter.php b/src/CSV/CSVWriter.php index 2ca0aba..471c4a9 100644 --- a/src/CSV/CSVWriter.php +++ b/src/CSV/CSVWriter.php @@ -52,7 +52,11 @@ public function open(mixed $file, array $options = []): void $this->resource = $file; $this->owns_resource = false; } elseif (is_string($file)) { - $this->resource = fopen($file, 'w'); + $resource = fopen($file, 'w'); + if ($resource === false) { + throw new RuntimeException('Unable to open CSV file '.$file); + } + $this->resource = $resource; $this->owns_resource = true; } else { throw new InvalidArgumentException( diff --git a/src/DeploymentConfig/ConfigValueDecrypter.php b/src/DeploymentConfig/ConfigValueDecrypter.php index c434e22..b0ed5c1 100644 --- a/src/DeploymentConfig/ConfigValueDecrypter.php +++ b/src/DeploymentConfig/ConfigValueDecrypter.php @@ -55,7 +55,11 @@ protected function loadAndValidateKeypair(string $name): string if (!is_file($keypair_file)) { throw InvalidConfigException::missingKeypair($keypair_file); } - $keypair = base64_decode(file_get_contents($keypair_file), true); + $keypair_content = file_get_contents($keypair_file); + if ($keypair_content === false) { + throw InvalidConfigException::corruptKeypair($keypair_file, 'could not read file'); + } + $keypair = base64_decode($keypair_content, true); if ($keypair === false) { throw InvalidConfigException::corruptKeypair($keypair_file, 'invalid base64'); } diff --git a/src/Mutex/DbBackedMutexWrapper.php b/src/Mutex/DbBackedMutexWrapper.php index 69b3e27..f248657 100644 --- a/src/Mutex/DbBackedMutexWrapper.php +++ b/src/Mutex/DbBackedMutexWrapper.php @@ -3,6 +3,8 @@ namespace Ingenerator\PHPUtils\Mutex; use PDO; +use PDOStatement; +use RuntimeException; class DbBackedMutexWrapper implements MutexWrapper { @@ -25,9 +27,8 @@ protected function getLock(string $name, int $timeout_seconds): void { // Note: These are borrowed from and could be used for our MysqlSession class $name_param = $this->db->quote($name); - $result = $this->db - ->query("SELECT GET_LOCK($name_param, $timeout_seconds)") - ->fetchAll(PDO::FETCH_COLUMN, 0); + $query = $this->runQuery("SELECT GET_LOCK($name_param, $timeout_seconds)"); + $result = $query->fetchAll(PDO::FETCH_COLUMN, 0); // Need to explicitly cast the result to an int for comparison as PDO value types vary between 8.0 and 8.1+ // And we should keep this cast even when we drop 8.0, because the PDO int/string mode is actually @@ -40,6 +41,18 @@ protected function getLock(string $name, int $timeout_seconds): void protected function releaseLock(string $name): void { $name_param = $this->db->quote($name); - $this->db->query("SELECT RELEASE_LOCK($name_param)")->fetchAll(); + $query = $this->runQuery("SELECT RELEASE_LOCK($name_param)"); + $query->fetchAll(); + } + + private function runQuery(string $str): PDOStatement + { + $query = $this->db->query($str); + if ($query === false) { + // If ATTR_ERRMODE was not set to PDO::ERRMODE_EXCEPTION, the method returns false rather than throwing, + throw new RuntimeException('Failed to execute query'.json_encode($this->db->errorInfo())); + } + + return $query; } } From 7ae1b8cf86e22f4901b76e37dab2fc83a9eb71df Mon Sep 17 00:00:00 2001 From: acoulton Date: Mon, 26 Oct 2026 17:00:58 +0000 Subject: [PATCH 6/7] fix: Handle errors binding callable functions --- src/Object/ObjectPropertyRipper.php | 13 +++++++++---- src/Object/ScopeChangingCaller.php | 9 ++++++--- 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/src/Object/ObjectPropertyRipper.php b/src/Object/ObjectPropertyRipper.php index e7e5d6a..d170709 100644 --- a/src/Object/ObjectPropertyRipper.php +++ b/src/Object/ObjectPropertyRipper.php @@ -10,6 +10,7 @@ use Closure; use DomainException; +use LogicException; use stdClass; use function get_object_vars; @@ -34,7 +35,8 @@ class ObjectPropertyRipper */ public static function rip(object $object, array $properties): array { - $ripper = static::getRipper($object::class); + $class = $object::class; + $ripper = static::getRipper($class); return $ripper($object, $properties); } @@ -83,6 +85,8 @@ public static function ripOne(object $object, string $property): mixed /** * Create a callback bound to the scope of the provided class so that it has access to internal * properties. + * + * @param class-string $class */ protected static function getRipper(string $class): Closure { @@ -102,9 +106,9 @@ function ($object, $properties): array { } /** - * @param object|class-string $scope + * @param object|class-string $scope */ - private static function bindScopedClosure(callable $callback, object|string $scope): Closure + private static function bindScopedClosure(Closure $callback, object|string $scope): Closure { $scope_class = is_object($scope) ? $scope::class : $scope; if ($scope_class === stdClass::class) { @@ -115,6 +119,7 @@ private static function bindScopedClosure(callable $callback, object|string $sco $scope = null; } - return Closure::bind($callback, newThis: null, newScope: $scope); + return Closure::bind($callback, newThis: null, newScope: $scope) + ?? throw new LogicException('Unable to bind closure to requested scope'); } } diff --git a/src/Object/ScopeChangingCaller.php b/src/Object/ScopeChangingCaller.php index 9c7f020..c2cfe39 100644 --- a/src/Object/ScopeChangingCaller.php +++ b/src/Object/ScopeChangingCaller.php @@ -3,6 +3,7 @@ namespace Ingenerator\PHPUtils\Object; use Closure; +use LogicException; class ScopeChangingCaller { @@ -27,12 +28,14 @@ class ScopeChangingCaller * @template TReturn of mixed * @template TArgs * - * @param callable(TArgs):TReturn $func + * @param class-string $class + * @param Closure(TArgs):TReturn $func * @param TArgs $args,... */ - public static function call(string $class, callable $func, mixed ...$args): mixed + public static function call(string $class, Closure $func, mixed ...$args): mixed { - $func = Closure::bind($func, null, $class); + $func = Closure::bind($func, null, $class) + ?? throw new LogicException('Unable to bind closure to requested scope'); return $func(...$args); } From 05b6d05615e275f13cdefd58cfb7f1c41201f1d9 Mon Sep 17 00:00:00 2001 From: acoulton Date: Mon, 26 Oct 2026 17:02:04 +0000 Subject: [PATCH 7/7] fix: Be stricter about types in logging code --- src/Logging/DefaultLogMetadata.php | 2 +- src/Logging/StackdriverApplicationLogger.php | 11 ++++++----- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/Logging/DefaultLogMetadata.php b/src/Logging/DefaultLogMetadata.php index 73ceb70..22193fe 100644 --- a/src/Logging/DefaultLogMetadata.php +++ b/src/Logging/DefaultLogMetadata.php @@ -55,7 +55,7 @@ public static function httpContext(array $server): array * * @param array $server * - * @return array> + * @return array{context: array{req: string}, 'logging.googleapis.com/trace': string} */ public static function requestTrace(array $server = [], ?string $gcp_project = null): array { diff --git a/src/Logging/StackdriverApplicationLogger.php b/src/Logging/StackdriverApplicationLogger.php index e1fd779..a7b7eb6 100644 --- a/src/Logging/StackdriverApplicationLogger.php +++ b/src/Logging/StackdriverApplicationLogger.php @@ -12,6 +12,7 @@ use Throwable; use function array_pad; +use function array_values; use function debug_backtrace; use function file_put_contents; use function http_response_code; @@ -115,7 +116,7 @@ public function __construct( protected string $log_destination, array|callable|LogMetadataProvider ...$metadata_sources, ) { - $this->metadata_sources = array_pad($metadata_sources, 2, []); + $this->metadata_sources = array_values(array_pad($metadata_sources, 2, [])); $this->call_site_finder = new ExternalCallSiteFinder(); } @@ -283,7 +284,7 @@ protected function formatException(Throwable $e): array * This method renders a string trace that follows the format of the standard trace (so it can be * parsed by tools that are looking for that) but without including any function argument details. * - * @param array $trace_stack + * @param list $trace_stack */ protected function getExceptionTraceAsStringWithoutArgs(array $trace_stack): string { @@ -293,10 +294,10 @@ protected function getExceptionTraceAsStringWithoutArgs(array $trace_stack): str $trace_lines[] = sprintf( '#%d %s: %s%s%s()', $index, - isset($trace['file']) ? $trace['file'].'('.$trace['line'].')' : '[internal function]', + isset($trace['file'], $trace['line']) ? $trace['file'].'('.$trace['line'].')' : '[internal function]', $trace['class'] ?? '', $trace['type'] ?? '', - $trace['function'] + $trace['function'] ?? '' ); } $trace_lines[] = '#'.count($trace_stack).' {main}'; @@ -382,7 +383,7 @@ public function logRequest(?array $server, ?float $request_start_time = null): v $meta = $this->getMetadata(); $log_entry = AssociativeArrayUtils::deepMerge( [ - 'severity' => strtoupper($this->getLogPriorityForHttpCode($http_code)), + 'severity' => strtoupper($this->getLogPriorityForHttpCode((string) $http_code)), self::PROP_INGEN_TYPE => 'rqst', 'httpRequest' => $meta['context']['httpRequest'] ?? [], 'context' => [