441 upgrade php stan analysis from level 6 to level 7#442
Merged
armanist merged 39 commits intosoftberg:masterfrom Mar 31, 2026
Merged
Conversation
…dant nullable types
…r PHPStan level 7
…er-key TTL support to FileAdapter via touch(), fix error messages and return types
…ests for all commands
…ialize guard in SleekDB Join
…eturn type for getParsedFile
…c with polymorphism and enforce proper error reporting
…actory non-nullable and fail loudly on invalid migration classes
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUpgrade PHPStan level to 7 and apply widespread defensive fixes: null/false guards, stricter typing, runtime interface checks in factories, trait extraction for cache TTL handling, Mailer refactor to adapter hooks, and numerous unit tests validating TTLs and command metadata. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/Mailer/Adapters/SendinblueAdapter.php (1)
82-100:⚠️ Potential issue | 🟡 Minor
json_encode()can returnfalseon encoding failure.If
json_encode($this->data)fails (e.g., due to non-UTF-8 characters), it returnsfalse, which would be passed tosetData(). This could result in sending the literal string"false"as the request body.🛡️ Proposed fix to handle json_encode failure
protected function sendEmail(): bool { try { + $jsonData = json_encode($this->data); + if ($jsonData === false) { + return false; + } + $this->httpClient ->createRequest($this->apiUrl) ->setMethod('POST') ->setHeaders([ 'Accept' => 'application/json', 'Content-type' => 'application/json', 'api-key' => $this->apiKey, ]) - ->setData(json_encode($this->data)) + ->setData($jsonData) ->start(); return true; } catch (Exception $e) { return false; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Mailer/Adapters/SendinblueAdapter.php` around lines 82 - 100, In SendinblueAdapter::sendEmail(), json_encode($this->data) may return false and you must not pass that into setData(); instead encode with error checking (or use JSON_THROW_ON_ERROR) and handle failures by logging the json_last_error_msg() (or catching the JsonException) and returning false before calling setData() and starting the request; update the sendEmail() method to validate $encoded = json_encode($this->data) !== false (or wrap in try/catch for JSON_THROW_ON_ERROR) and only call ->setData($encoded)->start() when encoding succeeds.src/Mailer/Adapters/SendgridAdapter.php (1)
100-100:⚠️ Potential issue | 🟡 MinorSame
json_encode()concern as other adapters.
json_encode($this->data)can returnfalseon failure. Consider adding a guard for consistency with PHPStan level 7 requirements.Proposed fix
- ->setData(json_encode($this->data)) + ->setData(json_encode($this->data) ?: '')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Mailer/Adapters/SendgridAdapter.php` at line 100, The call to json_encode in SendgridAdapter (the expression json_encode($this->data) passed into setData) can return false and must be guarded; update the code to first encode into a variable (e.g., $payload = json_encode($this->data)), check if $payload === false, and on failure throw or return a clear RuntimeException (including json_last_error_msg()) so setData always receives a valid string; then pass the validated $payload to setData.src/Mailer/Adapters/MailgunAdapter.php (1)
99-99:⚠️ Potential issue | 🟡 Minor
json_encode()can returnfalseon encoding failure.Given PHPStan level 7 requires explicit handling of functions that may return
false,json_encode($this->data)should be guarded. If encoding fails (e.g., with non-UTF8 strings),falsewill be passed tosetData().Proposed fix
- ->setData(json_encode($this->data)) + ->setData(json_encode($this->data) ?: '')Or throw an exception for a more explicit failure mode:
$json = json_encode($this->data); if ($json === false) { throw new Exception('Failed to encode mail data: ' . json_last_error_msg()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Mailer/Adapters/MailgunAdapter.php` at line 99, Wrap the call to json_encode when calling setData so encoding failures are handled: assign $json = json_encode($this->data), check if $json === false and if so throw an exception (or otherwise handle the error) using json_last_error_msg() for diagnostics, and only call setData($json) when encoding succeeded; this change targets the setData(json_encode($this->data)) call in MailgunAdapter (use $this->data, json_encode, json_last_error_msg, and setData identifiers).src/Cache/Adapters/FileAdapter.php (1)
161-169:⚠️ Potential issue | 🟡 MinorMake
delete()idempotent for missing keys.When
delete()returnsfalsefor a non-existent file,deleteMultiple()reports overall failure even if all keys were properly handled. SincedeleteMultiple()returns!in_array(false, $results, true), a singlefalsefrom anydelete()call triggers failure. Cache deletion should treat "already deleted" as success.Suggested fix
public function delete($key): bool { $path = $this->getPath($key); - if ($this->fs->exists($path)) { - return $this->fs->remove($path); + if (!$this->fs->exists($path)) { + return true; } - return false; + return $this->fs->remove($path); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Cache/Adapters/FileAdapter.php` around lines 161 - 169, The delete() method currently returns false when the key's file is missing, causing deleteMultiple() to treat "already deleted" as a failure; update delete() (in FileAdapter, referencing getPath(), $this->fs->exists() and $this->fs->remove()) so that if the file does not exist it returns true (idempotent success), and only return false when an actual removal attempt via $this->fs->remove($path) fails.src/Mailer/Traits/MailerTrait.php (1)
223-235:⚠️ Potential issue | 🟠 MajorRead transport errors before resetting transport state.
send()now callsresetFields()first, andresetFields()now unconditionally delegates toresetTransportState(). Any adapter that clears its client/mailer there will lose the data thatgetTransportErrors()needs, so failed sends become silent.Suggested fix
public function send(): bool { $this->prepare(); $sent = config()->get('mailer.mail_trap') ? $this->saveEmail() : $this->sendEmail(); - - $this->resetFields(); - - if (!$sent) { - $errors = $this->getTransportErrors(); - if (!empty($errors)) { - warning(implode(', ', $errors), ['tab' => Debugger::MAILS]); - } + $errors = !$sent ? $this->getTransportErrors() : []; + $this->resetFields(); + + if (!empty($errors)) { + warning(implode(', ', $errors), ['tab' => Debugger::MAILS]); } return $sent; }Also applies to: 333-341
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Mailer/Traits/MailerTrait.php` around lines 223 - 235, The send() flow currently calls resetFields() (which delegates to resetTransportState()) before calling getTransportErrors(), clearing adapter state and losing transport errors; modify send() so it obtains transport errors prior to resetting state (call $this->getTransportErrors() immediately after attempting send via sendEmail()/saveEmail(), handle/log those errors, then call $this->resetFields()), or alternatively move the resetFields() call to after the error-check block; ensure the referenced methods send(), resetFields(), resetTransportState(), getTransportErrors(), sendEmail(), and saveEmail() are updated accordingly so adapters retain their client state until errors are read.src/Console/QtCommand.php (1)
87-98:⚠️ Potential issue | 🟠 MajorHandle
nullkeys explicitly instead of converting them to empty string.The methods advertise
?stringbut currently pass''(empty string) toresolveInput()->getArgument()when$key = null. In Symfony Console, this attempts to lookup an argument named "" which doesn't exist and will fail. All current call sites provide a string key, so this nullable path is unreachable; however, the public API signature permits null, creating a contract violation.Options:
- Remove the nullable parameter (
-?string) if null should never be used.- Explicitly handle null (e.g., early return
''if$key === null).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Console/QtCommand.php` around lines 87 - 98, The getArgument and getOption methods currently accept ?string $key but pass $key ?? '' into resolveInput()->getArgument()/getOption, which causes an invalid lookup when null; either remove the nullable from the signature if callers never pass null, or explicitly handle null by returning '' early (e.g., if $key === null return ''), ensuring you update both getArgument and getOption and keep references to resolveInput()->getArgument and resolveInput()->getOption so callers and behavior remain consistent.
🟡 Minor comments (4)
src/Storage/UploadedFile.php-233-233 (1)
233-233:⚠️ Potential issue | 🟡 MinorAvoid returning a non-hash sentinel from
getMd5().
''preserves the declared type, but it also makes a failed hash read look like a legitimate result to any caller that doesn't re-validate the format. Prefer throwing or otherwise surfacing the failure explicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Storage/UploadedFile.php` at line 233, The getMd5() method currently returns an empty string when md5_file($this->getPathname()) fails, which can be mistaken for a valid hash; change getMd5() to detect when md5_file(...) returns false and surface the failure explicitly (e.g., throw a RuntimeException or a custom exception) with a clear message including the pathname from getPathname(), rather than returning '' so callers cannot confuse a failure with a legitimate hash.src/Console/Commands/OpenApiCommand.php-133-138 (1)
133-138:⚠️ Potential issue | 🟡 MinorSilent failure when routes file cannot be read.
If
$this->fs->get($routes)returnsfalse, the code silently skips writing without informing the user. This could lead to confusion when the route update doesn't occur.🛡️ Proposed fix to log a warning on read failure
if (!route_group_exists('openapi', $module)) { $routeContent = $this->fs->get($routes); if ($routeContent !== false) { $this->fs->put($routes, str_replace('return function ($route) {', $this->openapiRoutes($module), $routeContent)); + } else { + $this->error('Failed to read routes file: ' . $routes); + return; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Console/Commands/OpenApiCommand.php` around lines 133 - 138, When reading route files with $this->fs->get($routes) the code currently ignores a false return and silently skips updates; modify the block around $routeContent so that when $this->fs->get($routes) === false you log a warning (e.g., via $this->error or $this->line depending on the command logging helper) including the $routes path and $module identifier, and only proceed to call $this->fs->put($routes, str_replace(..., $this->openapiRoutes($module), $routeContent)) when $routeContent is truthy; reference the existing $this->fs->get, $this->fs->put, $routeContent, $routes and openapiRoutes($module) symbols when applying the change.tests/Unit/Console/Commands/KeyGenerateCommandTest.php-71-79 (1)
71-79:⚠️ Potential issue | 🟡 MinorVerify the cancel path leaves
.env.testingunchanged.This only checks stdout. If the command writes the file before printing the cancellation message, the test still passes.
Suggested tightening
public function testExecCancelsWhenNotConfirmed(): void { + $before = (string) $this->fs->get($this->envFilePath); env('APP_KEY', 'existing-key'); $this->tester->setInputs(['n']); $this->tester->execute([]); $output = $this->tester->getDisplay(); $this->assertStringContainsString('Operation was canceled', $output); + $this->assertSame($before, (string) $this->fs->get($this->envFilePath)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Unit/Console/Commands/KeyGenerateCommandTest.php` around lines 71 - 79, Update the testExecCancelsWhenNotConfirmed test to also verify the .env.testing file is not modified: read and save the current contents of .env.testing before calling $this->tester->execute([]), run the command with $this->tester->setInputs(['n']) and $this->tester->execute([]) as currently done, then assert the saved contents equal the file contents after execution (in addition to asserting the 'Operation was canceled' output). Reference the test method testExecCancelsWhenNotConfirmed, the env('APP_KEY', ...) setup, and $this->tester->execute / $this->tester->getDisplay to locate where to insert the file-read and post-run equality assertion.tests/Unit/Console/Commands/KeyGenerateCommandTest.php-62-69 (1)
62-69:⚠️ Potential issue | 🟡 MinorAssert that
core:keyactually wrote a new key.
assertNotEmpty(env('APP_KEY'))can stay green when.env.testingalready had a key or the environment cache was not refreshed, so it doesn't prove generation/persistence.Suggested tightening
public function testExecGeneratesKey(): void { + $before = (string) $this->fs->get($this->envFilePath); $this->tester->execute(['--yes' => true, '--length' => 16]); $output = $this->tester->getDisplay(); $this->assertStringContainsString('Application key successfully generated', $output); - $this->assertNotEmpty(env('APP_KEY')); + $after = (string) $this->fs->get($this->envFilePath); + $this->assertNotSame($before, $after); + $this->assertStringContainsString('APP_KEY=', $after); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Unit/Console/Commands/KeyGenerateCommandTest.php` around lines 62 - 69, The test testExecGeneratesKey in KeyGenerateCommandTest currently only checks env('APP_KEY') which can be stale; instead capture the APP_KEY before running the command (read env('APP_KEY') or read the .env.testing contents), run $this->tester->execute(['--yes'=>true,'--length'=>16]) via the tester, then assert that the file-backed value changed and that the .env.testing (or the environment file your tests write to) now contains a new non-empty key string different from the original; update assertions to compare the previous and current values rather than only assertNotEmpty(env('APP_KEY')) so you verify the command wrote a new key.
🧹 Nitpick comments (9)
src/Asset/AssetManager.php (1)
65-65: Consider strict comparison for consistency.For consistency with the other strict comparisons introduced in this PR, consider using
===here as well.- if (self::$instance == null) { + if (self::$instance === null) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Asset/AssetManager.php` at line 65, In AssetManager (check the singleton getter where it tests self::$instance), replace the loose comparison "if (self::$instance == null)" with a strict comparison using "===" so it reads "if (self::$instance === null)" to match the other strict comparisons in this PR and ensure type-safe null checking.src/Router/PatternCompiler.php (1)
212-215: Consider simplifying the PHPDoc union type.The
@paramannotation includesarray<string, mixed>|list<array<string, mixed>>, but the function logic (iterating and accessing$param['name']) only works correctly withlist<array<string, mixed>>. Passing a flat associative array like['name' => 'foo']would cause the foreach to iterate over scalar values, breaking the$param['name']access.Since the only caller passes a list of param arrays, consider narrowing the type for accuracy:
📝 Suggested PHPDoc simplification
/** * Ensure the parameter name is unique within the route pattern. - * `@param` array<string, mixed>|list<array<string, mixed>> $params + * `@param` list<array<string, mixed>> $params * `@throws` RouteException */ protected function checkParamName(array $params, string $name): void🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Router/PatternCompiler.php` around lines 212 - 215, Update the PHPDoc for checkParamName to accurately reflect that $params is a list of param arrays (use `@param` list<array<string,mixed>> $params) and add a runtime guard inside checkParamName (e.g., if (!array_is_list($params)) throw RouteException) to fail fast if an associative array is passed; reference the protected function checkParamName(array $params, string $name): void and its use of $param['name'] when implementing the guard.src/Console/Commands/InstallToolkitCommand.php (1)
93-94: AddRuntimeExceptionto the method throws contract.Line 100 now throws
RuntimeException, butrunExternalCommand()docblock only declaresExceptionInterface.💡 Proposed fix
/** * Runs an external command * `@param` array<string, mixed> $arguments * `@throws` ExceptionInterface + * `@throws` RuntimeException */Also applies to: 99-101
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Console/Commands/InstallToolkitCommand.php` around lines 93 - 94, The docblock for runExternalCommand() currently only declares throws ExceptionInterface but the method can also throw RuntimeException (lines 99-101); update the method docblock for runExternalCommand (in class InstallToolkitCommand) to include `@throws` RuntimeException alongside ExceptionInterface so the throws contract matches actual behavior.src/Mailer/Adapters/MandrillAdapter.php (1)
95-95:json_encode()can returnfalseon failure.Same issue as in
ResendAdapter-json_encode($this->data)may returnfalseif encoding fails. Consider adding a guard for consistency with PHPStan level 7 hardening.Proposed fix
- ->setData(json_encode($this->data)) + ->setData(json_encode($this->data) ?: '{}')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Mailer/Adapters/MandrillAdapter.php` at line 95, The call to json_encode($this->data) can return false on failure; update the MandrillAdapter code that calls ->setData(json_encode($this->data)) to first json_encode into a variable, check that the result !== false (or handle json_last_error()) and only pass the string to setData or throw/log a descriptive error; mirror the same guard/behavior used in ResendAdapter and reference the ->setData(...) call and $this->data in MandrillAdapter when implementing the fix.src/Mailer/Adapters/ResendAdapter.php (1)
96-96:json_encode()can returnfalseon failure.
json_encode($this->data)returnsfalseif encoding fails (e.g., due to malformed UTF-8). Thisfalsevalue is passed directly tosetData(). Given the PR's objective to handletype|falsereturns from PHP built-ins, consider adding a guard here.Proposed fix
- ->setData(json_encode($this->data)) + ->setData(json_encode($this->data) ?: '{}')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Mailer/Adapters/ResendAdapter.php` at line 96, The call to json_encode($this->data) can return false and is being passed directly into setData(); break the fluent chain around ->setData(...) in ResendAdapter, assign $encoded = json_encode($this->data), check if $encoded === false and if so throw or return a clear error (include json_last_error_msg() for details), otherwise call setData($encoded); this ensures setData always receives a string and surfaces JSON encode failures.src/Environment/Environment.php (1)
173-180: Good defensive guard forfs()->get()return value.The
is_string()check correctly handles cases wherefs()->get()returnsfalseornull. However,preg_replace()on line 178 can also returnnullon error (e.g., regex failure), which is then silently cast to an empty string on line 180. Consider whether a regex error should also raise an exception to avoid writing corrupted content.💡 Optional: guard against preg_replace failure
$envFileContent = preg_replace('/^' . preg_quote($row, '/') . '/m', $key . '=' . $value, $envFileContent); - fs()->put($envFilePath, (string) $envFileContent); + if ($envFileContent === null) { + throw new \RuntimeException('Failed to update environment variable: regex error'); + } + + fs()->put($envFilePath, $envFileContent);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Environment/Environment.php` around lines 173 - 180, The preg_replace call in Environment (operating on $envFileContent) can return null on regex error and is currently cast to string and written via fs()->put($envFilePath), risking corrupted files; after calling preg_replace($pattern, $replacement, $envFileContent) check its return is not null (and is_string), and if it is null throw an exception (e.g., EnvException::fileWriteError or a RuntimeException) instead of proceeding to fs()->put; reference the preg_replace invocation, $envFileContent, $envFilePath, fs()->put and the Environment class to locate and implement this guard.tests/Unit/Console/Commands/RouteListCommandTest.php (1)
19-31: Add one execution-path test here as well.These assertions only verify static metadata and the command definition. They will not catch regressions in
RouteListCommand's runtime execution/rendering path. Please add a test that actually runs the command and asserts it completes and renders successfully.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Unit/Console/Commands/RouteListCommandTest.php` around lines 19 - 31, Add an execution-path test to RouteListCommandTest that actually runs the command and asserts it completes and renders output: create a new test method (e.g., testExecuteRendersOutput) that instantiates a CommandTester for $this->command (or finds the command by name via the application), calls execute() with any required options (e.g., [] or ['module' => null]), asserts the returned exit code is 0, and asserts the captured output from CommandTester contains expected text (non-empty or a known header/route string) to verify the runtime rendering path of RouteListCommand.tests/Unit/Console/Commands/DebugBarCommandTest.php (1)
37-52: Consider moving cleanup totearDown()for reliability.If an assertion fails before lines 49-51, the cleanup won't execute, leaving artifacts. Moving cleanup to
tearDown()ensures it runs regardless of test outcome.Suggested improvement
+ private ?string $testAssetsPath = null; + + public function tearDown(): void + { + if ($this->testAssetsPath !== null) { + `@unlink`($this->testAssetsPath . DS . 'debugbar.css'); + `@rmdir`($this->testAssetsPath); + `@rmdir`(assets_dir() . DS . 'DebugBar'); + } + parent::tearDown(); + } + public function testExecShowsErrorWhenAlreadyInstalled(): void { - $assetsPath = assets_dir() . DS . 'DebugBar' . DS . 'Resources'; + $this->testAssetsPath = assets_dir() . DS . 'DebugBar' . DS . 'Resources'; - mkdir($assetsPath, 0777, true); - file_put_contents($assetsPath . DS . 'debugbar.css', '/* stub */'); + mkdir($this->testAssetsPath, 0777, true); + file_put_contents($this->testAssetsPath . DS . 'debugbar.css', '/* stub */'); $this->tester->execute([]); $output = $this->tester->getDisplay(); $this->assertStringContainsString('already installed', $output); - - `@unlink`($assetsPath . DS . 'debugbar.css'); - `@rmdir`($assetsPath); - `@rmdir`(assets_dir() . DS . 'DebugBar'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Unit/Console/Commands/DebugBarCommandTest.php` around lines 37 - 52, The test method testExecShowsErrorWhenAlreadyInstalled currently creates files/directories (assetsPath and debugbar.css) and performs cleanup inline, which won't run if assertions fail; move the cleanup into the DebugBarCommandTest::tearDown() method so it always runs. Specifically, remove the unlink/rmdir calls from testExecShowsErrorWhenAlreadyInstalled and instead add logic in tearDown() to check for and remove assets_dir() . DS . 'DebugBar', the Resources folder and debugbar.css (using safe checks like file_exists/is_dir and suppressors if desired) to ensure deterministic cleanup after each test run.tests/Unit/Console/Commands/ResourceCacheClearCommandTest.php (1)
48-60: Isolate the cache dir/config inside the test.This mutates shared
view_cachestate, touchesbase_dir()/cache, and suppresses cleanup failures with@rmdir(). Using a test-specific cache dir and restoring the original config will keep later console tests from becoming order-dependent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Unit/Console/Commands/ResourceCacheClearCommandTest.php` around lines 48 - 60, The test mutates global view_cache config and uses base_dir() . DS . 'cache' with a suppressed cleanup; change the test to create and use a unique temp cache directory (e.g., via sys_get_temp_dir() or tmpfile-style helper) instead of base_dir() . DS . 'cache', set config()->set('view_cache', ['cache_dir' => $tempDir]) before calling $this->tester->execute([]), and restore the original config() value after the test (save original = config('view_cache') and reset it). Also replace the suppressed `@rmdir`($cacheDir) with a reliable cleanup that removes the created directory and asserts or throws on failure so leftover state does not affect other tests; ensure references to base_dir(), DS, config()->set('view_cache'), and $this->tester->execute are updated accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/App/App.php`:
- Line 45: The accessor currently returns self::$baseDir ?? '' which masks an
uninitialized base directory and allows root-relative paths; change the accessor
(the method that returns self::$baseDir, e.g., getBaseDir) to explicitly fail
fast: if self::$baseDir is null/empty, throw a clear exception (e.g.,
RuntimeException or InvalidStateException) with a descriptive message like "Base
directory is not initialized" instead of returning an empty string; otherwise
return self::$baseDir. Ensure the thrown exception type is appropriate for
initialization/state errors and update any callers/tests that expect a string
return accordingly.
In `@src/Auth/Adapters/JwtAuthAdapter.php`:
- Around line 41-47: The constructor change made JwtToken mandatory and breaks
existing 3-arg callers; restore backward compatibility by making the constructor
accept JwtToken as optional (e.g. ?JwtToken $jwt = null) and assign it to the
JwtAuthAdapter::$jwt property as nullable, then add an internal
accessor/assertion method (e.g. getJwt(): JwtToken) used by other methods to
throw AuthException if the token is not provided; update usages inside
JwtAuthAdapter to call getJwt() instead of accessing $this->jwt directly so null
is enforced at runtime while keeping the original constructor signature
compatible.
In `@src/Auth/Factories/AuthFactory.php`:
- Around line 136-141: In createJwtInstance() replace the incorrect config key
'auth.claims' with the JWT-specific path used elsewhere — either use
config()->get('auth.' . AuthType::JWT . '.claims') or hardcode 'auth.jwt.claims'
so JwtToken->setClaims(...) receives the actual JWT claims; update the call in
the createJwtInstance() method of AuthFactory to reference AuthType::JWT or
'auth.jwt.claims' accordingly.
In `@src/Cache/Adapters/FileAdapter.php`:
- Around line 126-135: The set() method currently returns true if
$this->fs->put($path, ...) succeeds even when touch($path, ...) fails, causing
has() (which relies on $this->fs->lastModified($path)) to treat the entry as
expired; update set() (in FileAdapter::set) to check the result of touch($path,
time() + $this->normalizeTtl($ttl)) and treat a false return as a write failure:
if touch fails, remove the written file (e.g. $this->fs->delete($path) or
unlink) and return false; ensure you still return true only when both put and
touch succeed so has() and lastModified stay consistent.
In `@src/Cache/Adapters/MemcachedAdapter.php`:
- Around line 82-86: The bulk methods (getMultiple, setMultiple, deleteMultiple)
in MemcachedAdapter.php currently reject Traversable inputs and throw the global
InvalidArgumentException; update each guard to use is_iterable($keys) instead of
is_array($keys) and throw an exception that implements
Psr\SimpleCache\InvalidArgumentException (use the PSR-compatible
InvalidArgumentException type) while preserving the existing message from
ExceptionMessages::ARGUMENT_NOT_ITERABLE and the '$keys' context so callers
receive the correct contract-compliant exception and accept any iterable.
In `@src/Cache/Adapters/RedisAdapter.php`:
- Around line 77-81: The bulk methods in RedisAdapter (the getMultiple,
setMultiple, deleteMultiple implementations) currently reject Traversable by
checking is_array and throw the global \InvalidArgumentException; update each to
accept any iterable by using is_iterable($keys) (or equivalent) and when invalid
throw the PSR-16 compatible exception (Psr\SimpleCache\InvalidArgumentException)
instead of global \InvalidArgumentException; ensure the class imports/aliases
the PSR exception and update the three locations (around the checks at the
blocks referenced) to perform is_iterable checks and throw the PSR-compatible
exception so callers can catch Psr\SimpleCache\InvalidArgumentException.
In `@src/Cron/Schedule.php`:
- Around line 353-356: The at() method currently explodes $this->expression
(possibly empty) and overwrites parts[0]/[1], which can produce a 2-field cron
and later slip through build(); ensure you normalize the expression to a full
five-field cron before modifying it: in Schedule::at() (and where
$this->expression is used) treat an empty or short expression by filling missing
fields with '*' (i.e., expand to five parts) or use a default full expression,
then set parts[0] and parts[1] to $minute and $hour and re-implode; this
guarantees build() never receives an invalid 2-field expression.
In `@src/Database/Adapters/Sleekdb/Statements/Join.php`:
- Around line 53-55: If there are any joins present in $this->joins but
$this->queryBuilder is null, fail loudly instead of silently skipping: in
Join.php check for the case where !empty($this->joins) and $this->queryBuilder
=== null and throw a clear exception (or trigger an error) explaining that joins
cannot be applied without a queryBuilder; keep the existing call to
$this->applyJoin($this->queryBuilder, $this, $this->joins[0]) when queryBuilder
is non-null so behavior is unchanged otherwise.
In `@src/Mailer/Adapters/SmtpAdapter.php`:
- Around line 179-182: In resolveMessageId(), the subject passed to preg_match
uses preg_quote around $this->mailer->getLastMessageID(), which escapes the
angle-bracket delimiters and prevents the regex from matching the Message-ID;
remove preg_quote and call preg_match('/<(.*?)@/',
$this->mailer->getLastMessageID(), $matches) so the pattern can extract the
local part; keep the fallback to bin2hex(random_bytes(16)) if no match, but
ensure you pass the raw message ID from getLastMessageID() to preg_match instead
of an escaped string.
In `@src/Mailer/Traits/MailerTrait.php`:
- Around line 109-112: The default getRenderedMessage() returning null causes
saveEmail() to fall back to generateMessage(), which only appends $this->message
when it's a string and thus drops template-backed bodies; update the default
getRenderedMessage() (and the analogous code region around generateMessage()
usage referenced in the comment) to return the fully rendered body: if
$this->message is a string return it, otherwise if $this->message is an array
and a templatePath is present invoke the template rendering logic (the same
renderer used elsewhere) or call generateMessage() to produce the rendered
string and return that so template-backed mails keep their body when adapters
don't override getRenderedMessage().
In `@src/Migration/MigrationManager.php`:
- Around line 172-174: Replace the raw RuntimeException thrown in
MigrationManager when a migration is not an instance of QtMigration with a
migration-domain exception (e.g., MigrationException) so callers can handle
framework migration errors uniformly; update the checks in the methods that use
the condition "if (!$migration instanceof QtMigration)" to throw
MigrationException::invalidMigrationClass($migrationClassName) (or a similarly
named factory method) and, if that factory does not exist, add
MigrationException::invalidMigrationClass(string $className): self to create a
descriptive, typed exception that wraps the original context.
In `@src/Model/DbModel.php`:
- Around line 133-140: The loop over $this->getOrmInstance()->get() currently
discards source keys and silently skips null wraps; change the foreach to
iterate with the original keys (foreach ($this->getOrmInstance()->get() as $key
=> $item)) and assign $models[$key] = $model so the result preserves the ORM
result shape; also handle wrapToModel($item, static::class) returning null
explicitly (for example by throwing a descriptive exception or storing null with
a clear comment/log) instead of filtering it out so callers can detect
unexpected failures when building the ModelCollection.
In `@src/Model/Factories/ModelFactory.php`:
- Around line 103-117: Check and validate the configured ORM class before
calling new: retrieve $ormClass via Database::getInstance()->getOrmClass(),
ensure class_exists($ormClass) and that it implements DbalInterface (e.g.,
is_subclass_of or implements_interface check) and throw
ModelException::notInstanceOf($ormClass, DbalInterface::class) if not, then
instantiate the class with new $ormClass($table, $modelName, $idColumn,
$foreignKeys, $hidden) and return the instance; follow the same pre-construction
validation pattern used in the get() method.
In `@src/Module/ModuleManager.php`:
- Around line 161-163: The early return when is_array($dir) is false silently
hides filesystem failures and should fail fast; in the function containing the
$dir and $copiedFiles variables (ModuleManager.php) replace the silent "return
$copiedFiles" with an explicit failure path: throw a clear exception (e.g.
RuntimeException/InvalidArgumentException) including the directory identifier so
callers know listing failed, or alternatively return a distinct error value
(false/null) and ensure callers of this function check and propagate that
failure instead of assuming success. Ensure the change references the same $dir
and $copiedFiles symbols so the failure is reported where listing fails.
In `@src/Router/RouteDispatcher.php`:
- Around line 60-61: Replace the suppressed callable assumption with explicit
runtime callable checks: before calling $this->invoke($callable, $params) (and
the other occurrence around the second method call at the other block), validate
that the target is actually invocable via is_callable($callable) rather than
relying on method_exists(); if is_callable returns false, log or throw a clear
error/exception instead of invoking; update the checks that currently use
method_exists() and remove the /** `@phpstan-ignore` argument.type */ suppression
so invoke(...) only receives values that passed is_callable().
In `@src/Storage/Traits/CloudAppTrait.php`:
- Line 63: The retry path in CloudAppTrait is calling sendRequest($prevUrl ??
'', ...) which can pass an empty string URI; update the retry logic in
CloudAppTrait so you only call sendRequest when $prevUrl is a non-empty string
(or otherwise bail/throw a clear exception/return an error) instead of recursing
with ''. Specifically, check the $prevUrl variable before calling sendRequest
and handle the empty case explicitly (e.g., return an error/throw) in the method
that performs the refresh/retry to avoid forming a malformed outbound request.
In `@src/Storage/UploadedFile.php`:
- Around line 220-221: The code silently replaces a failed MIME inspection with
an empty string which causes save() to raise fileTypeNotAllowed(); instead, when
preg_split fails or $mimetype is not a string, throw a clear domain exception
(e.g., DomainException or a project-specific InspectionException) from
UploadedFile so the actual I/O/inspection failure is surfaced. Specifically, in
the block that sets $this->mimetype (in class UploadedFile), check that
is_string($mimetype) and preg_split returned an array and set $this->mimetype =
strtolower($mimetypeParts[0]); otherwise throw a descriptive domain exception
including the original $mimetype or inspection error so callers (like save())
see the real failure instead of an empty-string sentinel.
- Around line 440-446: The new RuntimeException thrown in
applyModifications()/save() for an invalid image modifier breaks the documented
exception contract; either move the is_callable validation into modify()
(validate $this->imageModifierFuncName when modifiers are registered and throw
the existing domain exception used elsewhere) or replace the RuntimeException in
applyModifications() with the same domain/framework exception class that
save()/applyModifications() already document; locate the callable construction
($callable = [$image, $this->imageModifierFuncName ?? '']) and change the
validation there (or relocate it to modify()) so callers continue to receive the
existing documented exception type instead of RuntimeException.
In `@tests/Unit/Console/Commands/EnvCommandTest.php`:
- Around line 32-46: The test currently renames .env.example to a fixed backup
and only restores it on the success path; wrap the operation in a try/finally so
the original file is always restored, and use a unique backup name (e.g. based
on uniqid or tempnam) to avoid collisions; specifically, capture $envExample,
compute a unique $backup = $envExample . '.' . uniqid('.bak', true) before
rename, perform $this->tester->execute([]) and assertions inside the try, and in
the finally check if file_exists($backup) then rename($backup, $envExample) to
guarantee restoration even on failures.
---
Outside diff comments:
In `@src/Cache/Adapters/FileAdapter.php`:
- Around line 161-169: The delete() method currently returns false when the
key's file is missing, causing deleteMultiple() to treat "already deleted" as a
failure; update delete() (in FileAdapter, referencing getPath(),
$this->fs->exists() and $this->fs->remove()) so that if the file does not exist
it returns true (idempotent success), and only return false when an actual
removal attempt via $this->fs->remove($path) fails.
In `@src/Console/QtCommand.php`:
- Around line 87-98: The getArgument and getOption methods currently accept
?string $key but pass $key ?? '' into resolveInput()->getArgument()/getOption,
which causes an invalid lookup when null; either remove the nullable from the
signature if callers never pass null, or explicitly handle null by returning ''
early (e.g., if $key === null return ''), ensuring you update both getArgument
and getOption and keep references to resolveInput()->getArgument and
resolveInput()->getOption so callers and behavior remain consistent.
In `@src/Mailer/Adapters/MailgunAdapter.php`:
- Line 99: Wrap the call to json_encode when calling setData so encoding
failures are handled: assign $json = json_encode($this->data), check if $json
=== false and if so throw an exception (or otherwise handle the error) using
json_last_error_msg() for diagnostics, and only call setData($json) when
encoding succeeded; this change targets the setData(json_encode($this->data))
call in MailgunAdapter (use $this->data, json_encode, json_last_error_msg, and
setData identifiers).
In `@src/Mailer/Adapters/SendgridAdapter.php`:
- Line 100: The call to json_encode in SendgridAdapter (the expression
json_encode($this->data) passed into setData) can return false and must be
guarded; update the code to first encode into a variable (e.g., $payload =
json_encode($this->data)), check if $payload === false, and on failure throw or
return a clear RuntimeException (including json_last_error_msg()) so setData
always receives a valid string; then pass the validated $payload to setData.
In `@src/Mailer/Adapters/SendinblueAdapter.php`:
- Around line 82-100: In SendinblueAdapter::sendEmail(),
json_encode($this->data) may return false and you must not pass that into
setData(); instead encode with error checking (or use JSON_THROW_ON_ERROR) and
handle failures by logging the json_last_error_msg() (or catching the
JsonException) and returning false before calling setData() and starting the
request; update the sendEmail() method to validate $encoded =
json_encode($this->data) !== false (or wrap in try/catch for
JSON_THROW_ON_ERROR) and only call ->setData($encoded)->start() when encoding
succeeds.
In `@src/Mailer/Traits/MailerTrait.php`:
- Around line 223-235: The send() flow currently calls resetFields() (which
delegates to resetTransportState()) before calling getTransportErrors(),
clearing adapter state and losing transport errors; modify send() so it obtains
transport errors prior to resetting state (call $this->getTransportErrors()
immediately after attempting send via sendEmail()/saveEmail(), handle/log those
errors, then call $this->resetFields()), or alternatively move the resetFields()
call to after the error-check block; ensure the referenced methods send(),
resetFields(), resetTransportState(), getTransportErrors(), sendEmail(), and
saveEmail() are updated accordingly so adapters retain their client state until
errors are read.
---
Minor comments:
In `@src/Console/Commands/OpenApiCommand.php`:
- Around line 133-138: When reading route files with $this->fs->get($routes) the
code currently ignores a false return and silently skips updates; modify the
block around $routeContent so that when $this->fs->get($routes) === false you
log a warning (e.g., via $this->error or $this->line depending on the command
logging helper) including the $routes path and $module identifier, and only
proceed to call $this->fs->put($routes, str_replace(...,
$this->openapiRoutes($module), $routeContent)) when $routeContent is truthy;
reference the existing $this->fs->get, $this->fs->put, $routeContent, $routes
and openapiRoutes($module) symbols when applying the change.
In `@src/Storage/UploadedFile.php`:
- Line 233: The getMd5() method currently returns an empty string when
md5_file($this->getPathname()) fails, which can be mistaken for a valid hash;
change getMd5() to detect when md5_file(...) returns false and surface the
failure explicitly (e.g., throw a RuntimeException or a custom exception) with a
clear message including the pathname from getPathname(), rather than returning
'' so callers cannot confuse a failure with a legitimate hash.
In `@tests/Unit/Console/Commands/KeyGenerateCommandTest.php`:
- Around line 71-79: Update the testExecCancelsWhenNotConfirmed test to also
verify the .env.testing file is not modified: read and save the current contents
of .env.testing before calling $this->tester->execute([]), run the command with
$this->tester->setInputs(['n']) and $this->tester->execute([]) as currently
done, then assert the saved contents equal the file contents after execution (in
addition to asserting the 'Operation was canceled' output). Reference the test
method testExecCancelsWhenNotConfirmed, the env('APP_KEY', ...) setup, and
$this->tester->execute / $this->tester->getDisplay to locate where to insert the
file-read and post-run equality assertion.
- Around line 62-69: The test testExecGeneratesKey in KeyGenerateCommandTest
currently only checks env('APP_KEY') which can be stale; instead capture the
APP_KEY before running the command (read env('APP_KEY') or read the .env.testing
contents), run $this->tester->execute(['--yes'=>true,'--length'=>16]) via the
tester, then assert that the file-backed value changed and that the .env.testing
(or the environment file your tests write to) now contains a new non-empty key
string different from the original; update assertions to compare the previous
and current values rather than only assertNotEmpty(env('APP_KEY')) so you verify
the command wrote a new key.
---
Nitpick comments:
In `@src/Asset/AssetManager.php`:
- Line 65: In AssetManager (check the singleton getter where it tests
self::$instance), replace the loose comparison "if (self::$instance == null)"
with a strict comparison using "===" so it reads "if (self::$instance === null)"
to match the other strict comparisons in this PR and ensure type-safe null
checking.
In `@src/Console/Commands/InstallToolkitCommand.php`:
- Around line 93-94: The docblock for runExternalCommand() currently only
declares throws ExceptionInterface but the method can also throw
RuntimeException (lines 99-101); update the method docblock for
runExternalCommand (in class InstallToolkitCommand) to include `@throws`
RuntimeException alongside ExceptionInterface so the throws contract matches
actual behavior.
In `@src/Environment/Environment.php`:
- Around line 173-180: The preg_replace call in Environment (operating on
$envFileContent) can return null on regex error and is currently cast to string
and written via fs()->put($envFilePath), risking corrupted files; after calling
preg_replace($pattern, $replacement, $envFileContent) check its return is not
null (and is_string), and if it is null throw an exception (e.g.,
EnvException::fileWriteError or a RuntimeException) instead of proceeding to
fs()->put; reference the preg_replace invocation, $envFileContent, $envFilePath,
fs()->put and the Environment class to locate and implement this guard.
In `@src/Mailer/Adapters/MandrillAdapter.php`:
- Line 95: The call to json_encode($this->data) can return false on failure;
update the MandrillAdapter code that calls ->setData(json_encode($this->data))
to first json_encode into a variable, check that the result !== false (or handle
json_last_error()) and only pass the string to setData or throw/log a
descriptive error; mirror the same guard/behavior used in ResendAdapter and
reference the ->setData(...) call and $this->data in MandrillAdapter when
implementing the fix.
In `@src/Mailer/Adapters/ResendAdapter.php`:
- Line 96: The call to json_encode($this->data) can return false and is being
passed directly into setData(); break the fluent chain around ->setData(...) in
ResendAdapter, assign $encoded = json_encode($this->data), check if $encoded ===
false and if so throw or return a clear error (include json_last_error_msg() for
details), otherwise call setData($encoded); this ensures setData always receives
a string and surfaces JSON encode failures.
In `@src/Router/PatternCompiler.php`:
- Around line 212-215: Update the PHPDoc for checkParamName to accurately
reflect that $params is a list of param arrays (use `@param`
list<array<string,mixed>> $params) and add a runtime guard inside checkParamName
(e.g., if (!array_is_list($params)) throw RouteException) to fail fast if an
associative array is passed; reference the protected function
checkParamName(array $params, string $name): void and its use of $param['name']
when implementing the guard.
In `@tests/Unit/Console/Commands/DebugBarCommandTest.php`:
- Around line 37-52: The test method testExecShowsErrorWhenAlreadyInstalled
currently creates files/directories (assetsPath and debugbar.css) and performs
cleanup inline, which won't run if assertions fail; move the cleanup into the
DebugBarCommandTest::tearDown() method so it always runs. Specifically, remove
the unlink/rmdir calls from testExecShowsErrorWhenAlreadyInstalled and instead
add logic in tearDown() to check for and remove assets_dir() . DS . 'DebugBar',
the Resources folder and debugbar.css (using safe checks like file_exists/is_dir
and suppressors if desired) to ensure deterministic cleanup after each test run.
In `@tests/Unit/Console/Commands/ResourceCacheClearCommandTest.php`:
- Around line 48-60: The test mutates global view_cache config and uses
base_dir() . DS . 'cache' with a suppressed cleanup; change the test to create
and use a unique temp cache directory (e.g., via sys_get_temp_dir() or
tmpfile-style helper) instead of base_dir() . DS . 'cache', set
config()->set('view_cache', ['cache_dir' => $tempDir]) before calling
$this->tester->execute([]), and restore the original config() value after the
test (save original = config('view_cache') and reset it). Also replace the
suppressed `@rmdir`($cacheDir) with a reliable cleanup that removes the created
directory and asserts or throws on failure so leftover state does not affect
other tests; ensure references to base_dir(), DS, config()->set('view_cache'),
and $this->tester->execute are updated accordingly.
In `@tests/Unit/Console/Commands/RouteListCommandTest.php`:
- Around line 19-31: Add an execution-path test to RouteListCommandTest that
actually runs the command and asserts it completes and renders output: create a
new test method (e.g., testExecuteRendersOutput) that instantiates a
CommandTester for $this->command (or finds the command by name via the
application), calls execute() with any required options (e.g., [] or ['module'
=> null]), asserts the returned exit code is 0, and asserts the captured output
from CommandTester contains expected text (non-empty or a known header/route
string) to verify the runtime rendering path of RouteListCommand.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1b1d7658-85f0-447e-81be-f2ef7ea5b585
📒 Files selected for processing (112)
phpstan.neon.distsrc/App/Adapters/ConsoleAppAdapter.phpsrc/App/Adapters/WebAppAdapter.phpsrc/App/App.phpsrc/App/Exceptions/StopExecutionException.phpsrc/App/Helpers/misc.phpsrc/App/Traits/AppTrait.phpsrc/App/Traits/ConsoleAppTrait.phpsrc/Archive/Adapters/PharAdapter.phpsrc/Archive/Adapters/ZipAdapter.phpsrc/Asset/Asset.phpsrc/Asset/AssetManager.phpsrc/Auth/Adapters/JwtAuthAdapter.phpsrc/Auth/Factories/AuthFactory.phpsrc/Auth/Traits/AuthTrait.phpsrc/Cache/Adapters/DatabaseAdapter.phpsrc/Cache/Adapters/FileAdapter.phpsrc/Cache/Adapters/MemcachedAdapter.phpsrc/Cache/Adapters/RedisAdapter.phpsrc/Cache/Factories/CacheFactory.phpsrc/Cache/Traits/CacheTrait.phpsrc/Captcha/Factories/CaptchaFactory.phpsrc/Captcha/Traits/CaptchaTrait.phpsrc/Console/Commands/DebugBarCommand.phpsrc/Console/Commands/InstallToolkitCommand.phpsrc/Console/Commands/KeyGenerateCommand.phpsrc/Console/Commands/OpenApiCommand.phpsrc/Console/Commands/ResourceCacheClearCommand.phpsrc/Console/Commands/RouteListCommand.phpsrc/Console/Commands/ServeCommand.phpsrc/Console/QtCommand.phpsrc/Cron/CronLock.phpsrc/Cron/CronTask.phpsrc/Cron/Schedule.phpsrc/Csrf/Csrf.phpsrc/Database/Adapters/Idiorm/IdiormDbal.phpsrc/Database/Adapters/Idiorm/IdiormPatch.phpsrc/Database/Adapters/Idiorm/Statements/Query.phpsrc/Database/Adapters/Idiorm/Statements/Result.phpsrc/Database/Adapters/Sleekdb/SleekDbal.phpsrc/Database/Adapters/Sleekdb/Statements/Join.phpsrc/Database/Adapters/Sleekdb/Statements/Model.phpsrc/Database/Adapters/Sleekdb/Statements/Result.phpsrc/Di/Di.phpsrc/Di/Exceptions/DiException.phpsrc/Encryption/Adapters/AsymmetricEncryptionAdapter.phpsrc/Encryption/Adapters/SymmetricEncryptionAdapter.phpsrc/Environment/Environment.phpsrc/Http/Request/HttpRequest.phpsrc/Http/Traits/Request/Params.phpsrc/Http/Traits/Request/RawInput.phpsrc/Http/Traits/Request/Url.phpsrc/Http/Traits/Response/Body.phpsrc/HttpClient/HttpClient.phpsrc/Lang/Lang.phpsrc/Lang/Translator.phpsrc/Loader/Loader.phpsrc/Logger/Factories/LoggerFactory.phpsrc/Mailer/Adapters/MailgunAdapter.phpsrc/Mailer/Adapters/MandrillAdapter.phpsrc/Mailer/Adapters/ResendAdapter.phpsrc/Mailer/Adapters/SendgridAdapter.phpsrc/Mailer/Adapters/SendinblueAdapter.phpsrc/Mailer/Adapters/SmtpAdapter.phpsrc/Mailer/Contracts/MailerInterface.phpsrc/Mailer/Factories/MailerFactory.phpsrc/Mailer/Traits/MailerTrait.phpsrc/Migration/MigrationManager.phpsrc/Migration/MigrationTable.phpsrc/Migration/QtMigration.phpsrc/Model/DbModel.phpsrc/Model/Factories/ModelFactory.phpsrc/Model/ModelCollection.phpsrc/Module/ModuleManager.phpsrc/Paginator/Traits/PaginatorTrait.phpsrc/Renderer/Adapters/HtmlAdapter.phpsrc/Renderer/Factories/RendererFactory.phpsrc/ResourceCache/ViewCache.phpsrc/Router/Helpers/router.phpsrc/Router/PatternCompiler.phpsrc/Router/Route.phpsrc/Router/RouteBuilder.phpsrc/Router/RouteDispatcher.phpsrc/Router/RouteFinder.phpsrc/Session/Factories/SessionFactory.phpsrc/Session/Traits/SessionTrait.phpsrc/Storage/Adapters/Dropbox/DropboxApp.phpsrc/Storage/Adapters/Local/LocalFileSystemAdapter.phpsrc/Storage/Factories/FileSystemFactory.phpsrc/Storage/Traits/CloudAppTrait.phpsrc/Storage/UploadedFile.phpsrc/Tracer/ErrorHandler.phpsrc/Validation/Traits/General.phpsrc/Validation/Traits/Resource.phpsrc/Validation/Validator.phpsrc/View/QtView.phptests/Unit/Cache/Adapters/DatabaseAdapterTest.phptests/Unit/Cache/Adapters/FileAdapterTest.phptests/Unit/Cache/Adapters/MemcachedAdapterTest.phptests/Unit/Cache/Adapters/RedisAdapterTest.phptests/Unit/Console/Commands/DebugBarCommandTest.phptests/Unit/Console/Commands/EnvCommandTest.phptests/Unit/Console/Commands/InstallToolkitCommandTest.phptests/Unit/Console/Commands/KeyGenerateCommandTest.phptests/Unit/Console/Commands/MigrationGenerateCommandTest.phptests/Unit/Console/Commands/MigrationMigrateCommandTest.phptests/Unit/Console/Commands/ModuleGenerateCommandTest.phptests/Unit/Console/Commands/OpenApiCommandTest.phptests/Unit/Console/Commands/ResourceCacheClearCommandTest.phptests/Unit/Console/Commands/RouteListCommandTest.phptests/Unit/Console/Commands/ServeCommandTest.phptests/Unit/Console/Commands/VersionCommandTest.php
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #442 +/- ##
============================================
- Coverage 82.65% 82.58% -0.08%
- Complexity 2730 2831 +101
============================================
Files 242 243 +1
Lines 7334 7545 +211
============================================
+ Hits 6062 6231 +169
- Misses 1272 1314 +42 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Renderer/Adapters/TwigAdapter.php (1)
70-70: LGTM! Correct null-safety fix for Twig Environment instantiation.The null coalescing operator ensures the
Environmentconstructor always receives an array, addressing the PHPStan level 7 union-type checking requirement.💡 Optional: Consider normalizing at assignment for consistency
Alternatively, the null coalescing could be applied in the constructor at assignment time, which would allow tightening the property type from
?arraytoarray:- protected ?array $configs; + /** `@var` array<string, mixed> */ + protected array $configs;public function __construct(?array $configs = []) { - $this->configs = $configs; + $this->configs = $configs ?? [];- $twig = new Environment($loader, $this->configs ?? []); + $twig = new Environment($loader, $this->configs);This is a minor style preference—the current fix is correct and minimal per PR objectives.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Renderer/Adapters/TwigAdapter.php` at line 70, Move the null-coalescing of configs into the TwigAdapter constructor so the property can be normalized to an array: in class TwigAdapter set the property type from ?array to array, assign $this->configs = $configs ?? [] in the constructor, and then update the Environment instantiation call (Environment(..., $this->configs)) to remove the inline ??[]; reference the TwigAdapter class, its constructor, the $this->configs property, and the Environment instantiation to locate the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/Renderer/Adapters/TwigAdapter.php`:
- Line 70: Move the null-coalescing of configs into the TwigAdapter constructor
so the property can be normalized to an array: in class TwigAdapter set the
property type from ?array to array, assign $this->configs = $configs ?? [] in
the constructor, and then update the Environment instantiation call
(Environment(..., $this->configs)) to remove the inline ??[]; reference the
TwigAdapter class, its constructor, the $this->configs property, and the
Environment instantiation to locate the changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a7ff3bf1-fb64-4a20-916c-6db8f33c51c0
📒 Files selected for processing (1)
src/Renderer/Adapters/TwigAdapter.php
armanist
added
enhancement
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Mailer/Traits/MailerTrait.php (1)
245-253:⚠️ Potential issue | 🟠 MajorMake the cached message id instance-scoped.
self::$messageIdsurvives across sends and across adapter instances of the same class. In mail-trap mode,src/Mailer/MailTrap.php:77-84uses that value as the filename, so the next message in the same process can overwrite the previous.eml; outside mail trap, it can also reuse the oldMessage-ID.Suggested fix
- private static ?string $messageId = null; + private ?string $messageId = null; public function getMessageId(): ?string { - if (self::$messageId) { - return self::$messageId; + if ($this->messageId !== null) { + return $this->messageId; } - self::$messageId = $this->resolveMessageId(); + $this->messageId = $this->resolveMessageId(); - return self::$messageId; + return $this->messageId; } private function resetFields(): void { $this->from = []; $this->addresses = []; $this->subject = null; $this->message = null; $this->templatePath = null; + $this->messageId = null; $this->resetTransportState(); }Also applies to: 338-346
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Mailer/Traits/MailerTrait.php` around lines 245 - 253, The code currently caches Message-ID in the static property self::$messageId which is shared across all instances and requests; change this to an instance-scoped cache by using a non-static property (e.g., $this->messageId) and update getMessageId() to check and set $this->messageId via $this->resolveMessageId(), and likewise update the other cached-accessor (the one around lines 338-346) to use the same instance property; ensure any references to self::$messageId are replaced with $this->messageId and that resolveMessageId() remains the source of truth for generation.src/Mailer/Adapters/SmtpAdapter.php (1)
243-259:⚠️ Potential issue | 🟠 MajorReset
SubjectandBodyon everyprepare()call.Because this adapter now reuses one
PHPMailerinstance, the truthy checks here are unsafe: a template-only mail with an empty context never renders, and a later send withnull/''subject or body can reuse the previous email’s content.Suggested fix
protected function prepare(): void { $this->mailer->setFrom($this->from['email'], $this->from['name']); - if ($this->subject) { - $this->mailer->Subject = $this->subject; - } + $this->mailer->Subject = $this->subject ?? ''; - if ($this->message) { - if ($this->templatePath) { - $body = $this->createFromTemplate(); - } else { - $body = is_array($this->message) ? implode('', $this->message) : $this->message; - } - - $this->mailer->Body = trim(str_replace("\n", '', $body)); + if ($this->templatePath) { + $body = $this->createFromTemplate(); + } else { + $body = is_array($this->message) ? implode('', $this->message) : (string) ($this->message ?? ''); } + + $this->mailer->Body = trim(str_replace("\n", '', $body)); $this->fillProperties('addAddress', $this->addresses); $this->fillProperties('addReplyTo', $this->replyToAddresses); $this->fillProperties('addCC', $this->ccAddresses);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Mailer/Adapters/SmtpAdapter.php` around lines 243 - 259, The prepare() method must explicitly reset the PHPMailer instance's Subject and Body on every call to avoid reusing previous values: set $this->mailer->Subject = '' and $this->mailer->Body = '' (or otherwise assign empty strings) at the start of prepare() before checking $this->subject, $this->message or $this->templatePath; then proceed to set Subject from $this->subject and compute Body via createFromTemplate() or implode($this->message) as currently done so that a null/empty subject or body does not inherit prior content from the reused $this->mailer.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Migration/MigrationManager.php`:
- Around line 171-173: Pre-validate the migration class before instantiating it:
before calling new $migrationClassName() in MigrationManager (both the
instantiation around the current check that uses "instanceof QtMigration" and
the second occurrence at the later block), use class_exists($migrationClassName)
and is_subclass_of($migrationClassName, QtMigration::class) (or equivalent) and
throw MigrationException::invalidMigrationClass($migrationClassName) if those
checks fail; only after those checks pass, instantiate the class and you can
safely assert it as a QtMigration instance.
---
Outside diff comments:
In `@src/Mailer/Adapters/SmtpAdapter.php`:
- Around line 243-259: The prepare() method must explicitly reset the PHPMailer
instance's Subject and Body on every call to avoid reusing previous values: set
$this->mailer->Subject = '' and $this->mailer->Body = '' (or otherwise assign
empty strings) at the start of prepare() before checking $this->subject,
$this->message or $this->templatePath; then proceed to set Subject from
$this->subject and compute Body via createFromTemplate() or
implode($this->message) as currently done so that a null/empty subject or body
does not inherit prior content from the reused $this->mailer.
In `@src/Mailer/Traits/MailerTrait.php`:
- Around line 245-253: The code currently caches Message-ID in the static
property self::$messageId which is shared across all instances and requests;
change this to an instance-scoped cache by using a non-static property (e.g.,
$this->messageId) and update getMessageId() to check and set $this->messageId
via $this->resolveMessageId(), and likewise update the other cached-accessor
(the one around lines 338-346) to use the same instance property; ensure any
references to self::$messageId are replaced with $this->messageId and that
resolveMessageId() remains the source of truth for generation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7d062388-e3fa-4d99-87a1-5c563e518176
📒 Files selected for processing (16)
src/App/App.phpsrc/Auth/Factories/AuthFactory.phpsrc/Cache/Adapters/FileAdapter.phpsrc/Database/Adapters/Sleekdb/Statements/Join.phpsrc/Mailer/Adapters/SmtpAdapter.phpsrc/Mailer/Traits/MailerTrait.phpsrc/Migration/Enums/ExceptionMessages.phpsrc/Migration/Exceptions/MigrationException.phpsrc/Migration/MigrationManager.phpsrc/Module/Enums/ExceptionMessages.phpsrc/Module/Exceptions/ModuleException.phpsrc/Module/ModuleManager.phpsrc/Storage/Traits/CloudAppTrait.phpsrc/Storage/UploadedFile.phptests/Unit/Console/Commands/EnvCommandTest.phptests/_root/cron-tests/failing-task.php
✅ Files skipped from review due to trivial changes (4)
- src/Module/Enums/ExceptionMessages.php
- src/Migration/Enums/ExceptionMessages.php
- tests/_root/cron-tests/failing-task.php
- src/Storage/UploadedFile.php
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Module/ModuleManager.php
- src/App/App.php
- tests/Unit/Console/Commands/EnvCommandTest.php
armanist
requested review from
Arno2005,
andrey-smaelov,
charoyan88,
grigoryanmartin20 and
live-soft
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/Unit/Mailer/MailerTest.php (1)
61-75: Test validates trivial case only.This test verifies that
getTransportErrors()returns an empty array whencreateRequest()is called but no actual HTTP request is executed. Per theHttpClientimplementation,createRequest()only initializes internal state without dispatching a network call, so errors will naturally be empty.While this provides basic coverage for the newly introduced
getTransportErrors()method, consider adding a follow-up test that validates error handling when actual transport errors occur (e.g., by mocking theHttpClientto simulate error responses).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Unit/Mailer/MailerTest.php` around lines 61 - 75, The current test MailerTest::testHttpAdapterGetTransportErrorsReturnsArray only covers the trivial path where createRequest() initializes state and no transport error occurs; extend it by adding a second test that mocks the internal httpClient (the private 'httpClient' property) to simulate a real transport failure (e.g., make send/request throw an exception or return an error response) and then invoke the protected getTransportErrors via ReflectionMethod('getTransportErrors') to assert it returns the expected array of error details; update the test to inject the mock into the adapter instance, trigger the failing call (instead of just createRequest()), then assertIsArray and assertNotEmpty (and that contents match the simulated error) so error-handling behavior is validated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/Unit/Mailer/MailerTest.php`:
- Around line 61-75: The current test
MailerTest::testHttpAdapterGetTransportErrorsReturnsArray only covers the
trivial path where createRequest() initializes state and no transport error
occurs; extend it by adding a second test that mocks the internal httpClient
(the private 'httpClient' property) to simulate a real transport failure (e.g.,
make send/request throw an exception or return an error response) and then
invoke the protected getTransportErrors via
ReflectionMethod('getTransportErrors') to assert it returns the expected array
of error details; update the test to inject the mock into the adapter instance,
trigger the failing call (instead of just createRequest()), then assertIsArray
and assertNotEmpty (and that contents match the simulated error) so
error-handling behavior is validated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: efada51f-07ac-4b14-8f80-fc50bceb8d24
📒 Files selected for processing (2)
tests/Unit/Console/QtCommandTest.phptests/Unit/Mailer/MailerTest.php
andrey-smaelov
approved these changes
Mar 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #441
Summary by CodeRabbit
Bug Fixes
New Features
Chores