Conversation
…gletons Drop static state from Route, Hook, Config, DI, ErrorHandler, ConnectionManager, TransactionManager, DebugTimer, RequestRecorder — each is now a regular class registered as a singleton on the app container and resolved through DI. - Route is one class doing both registry and value-object work; route files receive $route in scope from Kernel::loadRoutes() (no facade layer) - Hook is shared across the app via the container's Hookable trait and Container::before/after delegates so test-local Containers still observe the same hook registry - Db keeps its public static surface (user-facing API) but resolves ConnectionManager/TransactionManager from the container internally - DebugTimer keeps a no-op-when-disabled guard so dt()->mark() calls on hot paths cost nothing until dt()->start() is called - Added app()/dt()/recorder() global helpers to keep call sites compact - Container::make() short-circuits singleton(\$abstract) with no explicit concrete to build() directly, fixing infinite recursion on self-referential bindings 83/83 tests passing, full request pipeline + CLI optimize verified.
📝 WalkthroughWalkthroughThis PR performs a comprehensive architectural refactoring of the SilverEngine framework from static/singleton APIs to container-resolved dependency injection. It removes the external phpdotenv dependency and implements native ChangesDependency Injection and Core Infrastructure
Environment and Dependency Management
Error Handling and Database Services
Routing System
Debug Timing and Request Recording
View Compilation and Rendering
FileSystem Abstraction and Scaffolder
Application Integration
Testing and CLI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/silverengine/core/src/Core/DI.php (1)
56-63:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove leading backslash from object key storage in positional injection.
Line 62 stores numeric-indexed objects under
\ClassName, but line 36 resolves typed parameters asClassName. This mismatch breaks positional object injection, causingarray_key_exists($name, $vars)on line 40 to fail and throw "DI: Unable to inject parameter" errors.Proposed fix
- $prepared['\\' . get_class($value)] = $value; + $prepared[get_class($value)] = $value;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/silverengine/core/src/Core/DI.php` around lines 56 - 63, prepareVars stores numeric-indexed objects using a leading backslash ('\\' . get_class($value)) while parameter resolution (array_key_exists($name, $vars)) looks up unprefixed class names, causing positional injections to miss matches. Change prepareVars so it uses the class name without the leading backslash when setting keys for object values (i.e., use get_class($value) as the key for $prepared), ensuring the keys produced by prepareVars match the parameter names checked by array_key_exists in the injection flow.
🧹 Nitpick comments (4)
packages/silverengine/core/src/Core/Route.php (1)
238-261: 💤 Low valueParameter
$routeshadows property of the same name.The parameter
$routeshadows$this->route, which can be confusing when reading code that references$routevs$this->route. Consider renaming to$pathor$pattern.♻️ Suggested rename
public function register( string $method, - string $route, + string $path, mixed $action, ?string $name = null, string $middleware = 'public', string $type = '', ): void { - $route = $this->prefix . $route; + $path = $this->prefix . $path; foreach (explode('|', $method) as $m) { $r = new self(); $r->method = strtolower($m); - $r->route = $route; + $r->route = $path; $r->action = $action; $r->jails = $this->jailStack; $r->name = $name; $r->middleware = $middleware; $r->type = $type; $this->routes[] = $r; if ($name !== null) { $this->routeIndex[$name] = $r; } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/silverengine/core/src/Core/Route.php` around lines 238 - 261, In Route::register rename the parameter $route to $path (or $pattern) to avoid shadowing the Route::$route property; update the function signature in register(string $method, string $path, mixed $action, ?string $name = null, string $middleware = 'public', string $type = '') and replace all uses of the parameter inside the method (e.g. $route = $this->prefix . $route) to use $path (e.g. $path = $this->prefix . $path) while continuing to assign the resolved path to the new route instance via $r->route = $path and preserving the rest of the logic including $this->routeIndex population.packages/silverengine/core/src/Core/Kernel.php (1)
151-153: 💤 Low value
clearstatcache()triggers on any namespaced class.The condition
str_contains($action, '\\')matches any fully-qualified class name, causingclearstatcache(true)to run on nearly every request. If this is for development hot-reload scenarios, consider gating it behind a debug/dev mode check.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/silverengine/core/src/Core/Kernel.php` around lines 151 - 153, The current condition uses str_contains($action, '\\') to call clearstatcache(true) for any namespaced class, causing it to run on almost every request; change the guard to require a development/debug mode as well (e.g. combine the existing check with your app debug flag such as a Kernel debug getter, environment variable, or config flag) so only when both str_contains($action, '\\') and the app is in debug/dev mode do you call clearstatcache(true); locate this logic around $action and replace the plain str_contains check with a compound condition (e.g. str_contains(...) && $this->isDebug() or str_contains(...) && getenv('APP_DEBUG')) to prevent clearing the stat cache in production.packages/silverengine/core/src/Core/Env.php (1)
234-294: 💤 Low valueNative parser looks solid for the documented scope.
The implementation correctly handles:
- Cross-platform line endings via
\R- Immutable semantics matching
safeLoad()- Both quote styles with appropriate escape handling
One observation: the double-quote escape handling (line 279) supports only
\n,\t,\",\\. phpdotenv also supports\r,\f,\v, and\$. If any.envfiles use those escapes, they'll be preserved literally. Given this is documented as a "spike" implementation, this is acceptable but worth noting for migration awareness.LGTM!
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/silverengine/core/src/Core/Env.php` around lines 234 - 294, The double-quote escape handling in Core\Env::loadEnvFile only maps ['\\n'=>'\n','\\t'=>"\t",'\\\"'=>'"','\\\\'=>'\\'] and thus misses phpdotenv-supported escapes (\r, \f, \v, \$); update the strtr call inside the quoted-value branch (the $quote === '"' branch) to include mappings for '\\r'=>"\r", '\\f'=>"\f", '\\v'=>"\v", and '\\$'=>'$' so those escapes are unescaped correctly; adjust any related tests or comments to reflect parity with phpdotenv.packages/silverengine/core/src/Engine/Ghost/Wisp.php (1)
73-87: 💤 Low valueConsider adding
JSON_THROW_ON_ERRORfor explicit failure handling.If
json_encode()fails (e.g., non-UTF8 strings in page data), it returnsfalse, which silently becomes an empty string. The client would receivedata-page=""and fail to mount without a clear server-side indication.Adding the flag surfaces encoding issues earlier:
$json = htmlspecialchars( - (string) json_encode($page, JSON_UNESCAPED_SLASHES | JSON_UNESCAPED_UNICODE), + json_encode($page, JSON_UNESCAPED_SLASHES | JSON_UNESCAPED_UNICODE | JSON_THROW_ON_ERROR), ENT_QUOTES, 'UTF-8', );This is low-risk since typical page data encodes cleanly, but it aids debugging when edge cases occur.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/silverengine/core/src/Engine/Ghost/Wisp.php` around lines 73 - 87, In the el(array $page): string method, add the JSON_THROW_ON_ERROR flag to the json_encode call and wrap the encoding in a try/catch for JsonException so encoding failures (e.g., non-UTF8 data) are surfaced instead of silently producing an empty string; on catch, either rethrow a clear exception or log the error and fail fast so callers know encoding failed for $page before returning the data-page string.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/silverengine/core/src/App/Controllers/ScaffoldController.php`:
- Around line 31-46: The controller currently only checks for non-empty
$url/$name and blindly redirects to $result['url']; update ScaffoldController to
validate and normalize scaffold targets by ensuring the provided $url is an
internal path (e.g., begins with '/' and contains no scheme/host or authority),
reject or sanitize any input containing "http(s)://", host, or back-reference
segments (..), normalize the path (remove ../, duplicate slashes, ensure single
leading slash) before passing to $scaffolder->scaffold and before emitting the
redirect for $result['url'], and when sending the Location header use the
normalized, internal-only path (and keep http_response_code(303) and exit) to
prevent open-redirects and path-traversal; reference $url, $name,
$scaffolder->scaffold(), $result['url'], header('Location: ...') when making
these changes.
In `@packages/silverengine/core/src/Core/ErrorHandler.php`:
- Around line 32-39: The withFilter method currently sets a new filter, invokes
the callback, and then restores the old filter but does not handle exceptions —
if $cb throws the original filter remains lost; wrap the callback invocation in
a try/finally inside withFilter so that setFilter($old) is always executed
(restore in finally), ensure the callback's return value is returned and any
exception is rethrown naturally by not catching it in the try block. Use the
existing method name withFilter and the getFilter/setFilter helpers when
implementing the try/finally.
In `@packages/silverengine/core/src/Core/Route.php`:
- Around line 43-47: The 'string' regex in the Route class types array is
malformed (extra `$` after the closing delimiter) causing preg_match warnings;
update the 'string' entry in the private array $types within class Route to a
valid pattern that anchors the end inside the delimiters (e.g. ensure the
pattern ends with +$ before the closing slash) so preg_match() works correctly.
In `@packages/silverengine/core/src/Engine/CLI.php`:
- Around line 57-62: The $loadRouteFile closure currently uses include_once
which can skip execution if a route file was previously loaded (breaking route
registration after reset()); change it to use a plain include guarded by checks:
validate the constructed path before including by ensuring it is a real file
(file_exists/is_readable) and that its resolved realpath is under the ROOT
directory to prevent out-of-root loads, then perform include so the file always
executes when expected (reference the $loadRouteFile closure and the loop
iterating Env::get('routes')).
In `@packages/silverengine/core/src/FileSystem/LocalFileSystem.php`:
- Around line 45-50: The write() (and append()) implementation currently returns
0 for both a failed file_put_contents and a legitimate zero-byte write; change
the behavior to distinguish error vs zero-length success by detecting
file_put_contents === false and throwing an exception (e.g., RuntimeException)
with contextual message rather than returning 0, while returning the integer
byte count for successes (including 0). Update the closures passed to intercept
in LocalFileSystem::write and LocalFileSystem::append to check for === false and
throw, keeping the rest of the intercept call/site unchanged so callers can
catch the exception instead of relying on ambiguous 0.
In `@packages/silverengine/core/src/Support/Scaffolder.php`:
- Around line 40-43: The code currently appends entries to the created[] array
immediately after calling $this->fs->write (e.g., the controller creation block
using $this->fs->isFile, $this->fs->write and $this->controllerStub($name)),
which can record artifacts as created even if the write failed; update those
blocks (including the similar spots around lines 45-48 and 172-173) to check the
return value or throw status of $this->fs->write and only push the corresponding
'app/Controllers/...Controller.php' (and other artifact paths) into $created
when the write succeeded, otherwise handle/log the failure and do not mark the
artifact as created.
- Around line 31-32: The generated PHP embeds $url into a single-quoted string
without escaping, so any single-quote in $url will break the output; update the
Scaffolder code that builds the URL (the assignment using $url = '/' .
ltrim($url, '/'); and the other occurrence around line 153) to either validate
the URL (e.g., filter_var($url, FILTER_VALIDATE_URL) and reject/normalize
invalid input) and then escape single quotes before embedding (e.g., $safeUrl =
str_replace("'", "\\'", $url) or use var_export($url, true) / addslashes($url))
so the generated single-quoted PHP string remains syntactically correct. Ensure
you apply the same change at both places where $url is embedded.
- Around line 159-167: The current insertion logic in Scaffolder.php checks for
existing use statements and uses preg_replace with a pattern that is a no-op
when no use lines exist, so $useLine never gets added (leading to missing
imports for {$name}Controller). Fix by detecting the absence of any "use ...;"
lines (using the existing str_contains($src, $useLine) check or a preg_match for
'^use ') and, when none exist, insert $useLine right after the opening PHP tag
(<?php) or at the top of the file before any routes definitions; update the code
paths around $src, $useLine and the preg_replace call so the fallback branch
performs a safe insert after the <?php header.
In `@tests/Unit/Framework/Core/HookTest.php`:
- Around line 35-39: The test uses a fixed tmp path ($tmp =
'/tmp/hooked-hook-target.txt') and a fixed fs write target
('/anywhere/hook-target.txt') which can collide in parallel runs; change the
$tmp assignment to generate a unique temp filename (e.g., using
tempnam(sys_get_temp_dir(), 'hooked-') or uniqid) and update the corresponding
fs->write target to use that same unique path so each test worker operates on
its own file (adjust references to $tmp and the string literal
'/anywhere/hook-target.txt' in HookTest.php).
---
Outside diff comments:
In `@packages/silverengine/core/src/Core/DI.php`:
- Around line 56-63: prepareVars stores numeric-indexed objects using a leading
backslash ('\\' . get_class($value)) while parameter resolution
(array_key_exists($name, $vars)) looks up unprefixed class names, causing
positional injections to miss matches. Change prepareVars so it uses the class
name without the leading backslash when setting keys for object values (i.e.,
use get_class($value) as the key for $prepared), ensuring the keys produced by
prepareVars match the parameter names checked by array_key_exists in the
injection flow.
---
Nitpick comments:
In `@packages/silverengine/core/src/Core/Env.php`:
- Around line 234-294: The double-quote escape handling in Core\Env::loadEnvFile
only maps ['\\n'=>'\n','\\t'=>"\t",'\\\"'=>'"','\\\\'=>'\\'] and thus misses
phpdotenv-supported escapes (\r, \f, \v, \$); update the strtr call inside the
quoted-value branch (the $quote === '"' branch) to include mappings for
'\\r'=>"\r", '\\f'=>"\f", '\\v'=>"\v", and '\\$'=>'$' so those escapes are
unescaped correctly; adjust any related tests or comments to reflect parity with
phpdotenv.
In `@packages/silverengine/core/src/Core/Kernel.php`:
- Around line 151-153: The current condition uses str_contains($action, '\\') to
call clearstatcache(true) for any namespaced class, causing it to run on almost
every request; change the guard to require a development/debug mode as well
(e.g. combine the existing check with your app debug flag such as a Kernel debug
getter, environment variable, or config flag) so only when both
str_contains($action, '\\') and the app is in debug/dev mode do you call
clearstatcache(true); locate this logic around $action and replace the plain
str_contains check with a compound condition (e.g. str_contains(...) &&
$this->isDebug() or str_contains(...) && getenv('APP_DEBUG')) to prevent
clearing the stat cache in production.
In `@packages/silverengine/core/src/Core/Route.php`:
- Around line 238-261: In Route::register rename the parameter $route to $path
(or $pattern) to avoid shadowing the Route::$route property; update the function
signature in register(string $method, string $path, mixed $action, ?string $name
= null, string $middleware = 'public', string $type = '') and replace all uses
of the parameter inside the method (e.g. $route = $this->prefix . $route) to use
$path (e.g. $path = $this->prefix . $path) while continuing to assign the
resolved path to the new route instance via $r->route = $path and preserving the
rest of the logic including $this->routeIndex population.
In `@packages/silverengine/core/src/Engine/Ghost/Wisp.php`:
- Around line 73-87: In the el(array $page): string method, add the
JSON_THROW_ON_ERROR flag to the json_encode call and wrap the encoding in a
try/catch for JsonException so encoding failures (e.g., non-UTF8 data) are
surfaced instead of silently producing an empty string; on catch, either rethrow
a clear exception or log the error and fail fast so callers know encoding failed
for $page before returning the data-page string.
🪄 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 Plus
Run ID: e7295528-00e1-47c2-8c0b-19826c0bac8d
📒 Files selected for processing (47)
app/Controllers/WelcomeController.phpapp/Controllers/WispDemoController.phpapp/Routes/Api.phpapp/Routes/Web.phpapp/Views/errors/404.ghost.tplpackages/silverengine/core/composer.jsonpackages/silverengine/core/src/App/Controllers/DebugController.phppackages/silverengine/core/src/App/Controllers/ScaffoldController.phppackages/silverengine/core/src/App/Middlewares/ErrorHandler.phppackages/silverengine/core/src/App/Routes.phppackages/silverengine/core/src/App/Views/debug/index.phppackages/silverengine/core/src/App/Views/demo/default.ghost.phppackages/silverengine/core/src/App/Views/errors/404.phppackages/silverengine/core/src/App/Views/errors/500.ghost.phppackages/silverengine/core/src/Concerns/Hookable.phppackages/silverengine/core/src/Core/App.phppackages/silverengine/core/src/Core/Config.phppackages/silverengine/core/src/Core/Container.phppackages/silverengine/core/src/Core/DI.phppackages/silverengine/core/src/Core/Env.phppackages/silverengine/core/src/Core/ErrorHandler.phppackages/silverengine/core/src/Core/Hook.phppackages/silverengine/core/src/Core/Kernel.phppackages/silverengine/core/src/Core/Library.phppackages/silverengine/core/src/Core/Route.phppackages/silverengine/core/src/Database/ConnectionManager.phppackages/silverengine/core/src/Database/Db.phppackages/silverengine/core/src/Database/TransactionManager.phppackages/silverengine/core/src/Engine/CLI.phppackages/silverengine/core/src/Engine/Ghost/Compiler.phppackages/silverengine/core/src/Engine/Ghost/Template.phppackages/silverengine/core/src/Engine/Ghost/Vite.phppackages/silverengine/core/src/Engine/Ghost/Wisp.phppackages/silverengine/core/src/FileSystem/FileSystem.phppackages/silverengine/core/src/FileSystem/LocalFileSystem.phppackages/silverengine/core/src/Http/Request.phppackages/silverengine/core/src/Http/View.phppackages/silverengine/core/src/Support/DebugTimer.phppackages/silverengine/core/src/Support/RequestRecorder.phppackages/silverengine/core/src/Support/Scaffolder.phppublic/index.phptests/Unit/Framework/Core/ContainerContractTest.phptests/Unit/Framework/Core/ErrorPageTest.phptests/Unit/Framework/Core/HookTest.phptests/Unit/Framework/Core/OptimizeCacheTest.phptests/Unit/Framework/Core/RouteTest.phptests/Unit/Framework/Support/RequestRecorderTest.php
💤 Files with no reviewable changes (1)
- app/Views/errors/404.ghost.tpl
Comment on lines
+31
to
+46
| $url = (string) $request->input('url'); | ||
| $name = (string) $request->input('name'); | ||
| if ($url === '' || $name === '') { | ||
| return $this->json(['ok' => false, 'error' => 'url and name are required'], 422); | ||
| } | ||
|
|
||
| try { | ||
| $result = $scaffolder->scaffold($url, $name); | ||
| } catch (\Throwable $e) { | ||
| return $this->json(['ok' => false, 'error' => $e->getMessage()], 422); | ||
| } | ||
|
|
||
| // The original 404'd URL is now (or will be on next request) live — | ||
| // redirect there so the dev sees the new page immediately. | ||
| header('Location: ' . $result['url']); | ||
| http_response_code(303); |
There was a problem hiding this comment.
Harden scaffold URL handling and redirect target.
Line 31-Line 33 only validate non-empty input, and Line 45 redirects using a derived URL directly. Please constrain scaffold URLs to internal path format and normalize the redirect target before emitting Location.
[sraise_placeholder]
Suggested hardening patch
- $url = (string) $request->input('url');
+ $url = '/' . ltrim(trim((string) $request->input('url')), '/');
$name = (string) $request->input('name');
- if ($url === '' || $name === '') {
+ if ($url === '/' || $name === '') {
return $this->json(['ok' => false, 'error' => 'url and name are required'], 422);
}
+ if (!preg_match('#^/[A-Za-z0-9/_-]+$#', $url)) {
+ return $this->json(['ok' => false, 'error' => 'invalid url'], 422);
+ }
@@
- header('Location: ' . $result['url']);
- http_response_code(303);
+ $redirect = (string) ($result['url'] ?? '/');
+ if (!str_starts_with($redirect, '/')) {
+ $redirect = '/' . ltrim($redirect, '/');
+ }
+ header('Location: ' . $redirect, true, 303);
return '';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| $url = (string) $request->input('url'); | |
| $name = (string) $request->input('name'); | |
| if ($url === '' || $name === '') { | |
| return $this->json(['ok' => false, 'error' => 'url and name are required'], 422); | |
| } | |
| try { | |
| $result = $scaffolder->scaffold($url, $name); | |
| } catch (\Throwable $e) { | |
| return $this->json(['ok' => false, 'error' => $e->getMessage()], 422); | |
| } | |
| // The original 404'd URL is now (or will be on next request) live — | |
| // redirect there so the dev sees the new page immediately. | |
| header('Location: ' . $result['url']); | |
| http_response_code(303); | |
| $url = '/' . ltrim(trim((string) $request->input('url')), '/'); | |
| $name = (string) $request->input('name'); | |
| if ($url === '/' || $name === '') { | |
| return $this->json(['ok' => false, 'error' => 'url and name are required'], 422); | |
| } | |
| if (!preg_match('#^/[A-Za-z0-9/_-]+$#', $url)) { | |
| return $this->json(['ok' => false, 'error' => 'invalid url'], 422); | |
| } | |
| try { | |
| $result = $scaffolder->scaffold($url, $name); | |
| } catch (\Throwable $e) { | |
| return $this->json(['ok' => false, 'error' => $e->getMessage()], 422); | |
| } | |
| // The original 404'd URL is now (or will be on next request) live — | |
| // redirect there so the dev sees the new page immediately. | |
| $redirect = (string) ($result['url'] ?? '/'); | |
| if (!str_starts_with($redirect, '/')) { | |
| $redirect = '/' . ltrim($redirect, '/'); | |
| } | |
| header('Location: ' . $redirect, true, 303); | |
| return ''; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/silverengine/core/src/App/Controllers/ScaffoldController.php` around
lines 31 - 46, The controller currently only checks for non-empty $url/$name and
blindly redirects to $result['url']; update ScaffoldController to validate and
normalize scaffold targets by ensuring the provided $url is an internal path
(e.g., begins with '/' and contains no scheme/host or authority), reject or
sanitize any input containing "http(s)://", host, or back-reference segments
(..), normalize the path (remove ../, duplicate slashes, ensure single leading
slash) before passing to $scaffolder->scaffold and before emitting the redirect
for $result['url'], and when sending the Location header use the normalized,
internal-only path (and keep http_response_code(303) and exit) to prevent
open-redirects and path-traversal; reference $url, $name,
$scaffolder->scaffold(), $result['url'], header('Location: ...') when making
these changes.
Comment on lines
+32
to
39
| public function withFilter(int $filter, callable $cb): mixed | ||
| { | ||
| $old = self::getFilter(); | ||
| self::setFilter($filter); | ||
| $old = $this->getFilter(); | ||
| $this->setFilter($filter); | ||
| $rv = $cb(); | ||
| self::setFilter($old); | ||
| $this->setFilter($old); | ||
| return $rv; | ||
| } |
There was a problem hiding this comment.
withFilter is not exception-safe — filter is not restored if $cb throws.
If the callback throws an exception, the original filter is never restored, leaving the ErrorHandler in a modified state for subsequent operations.
Proposed fix using try/finally
public function withFilter(int $filter, callable $cb): mixed
{
$old = $this->getFilter();
$this->setFilter($filter);
- $rv = $cb();
- $this->setFilter($old);
- return $rv;
+ try {
+ return $cb();
+ } finally {
+ $this->setFilter($old);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public function withFilter(int $filter, callable $cb): mixed | |
| { | |
| $old = self::getFilter(); | |
| self::setFilter($filter); | |
| $old = $this->getFilter(); | |
| $this->setFilter($filter); | |
| $rv = $cb(); | |
| self::setFilter($old); | |
| $this->setFilter($old); | |
| return $rv; | |
| } | |
| public function withFilter(int $filter, callable $cb): mixed | |
| { | |
| $old = $this->getFilter(); | |
| $this->setFilter($filter); | |
| try { | |
| return $cb(); | |
| } finally { | |
| $this->setFilter($old); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/silverengine/core/src/Core/ErrorHandler.php` around lines 32 - 39,
The withFilter method currently sets a new filter, invokes the callback, and
then restores the old filter but does not handle exceptions — if $cb throws the
original filter remains lost; wrap the callback invocation in a try/finally
inside withFilter so that setFilter($old) is always executed (restore in
finally), ensure the callback's return value is returned and any exception is
rethrown naturally by not catching it in the try block. Use the existing method
name withFilter and the getFilter/setFilter helpers when implementing the
try/finally.
Comment on lines
+43
to
47
| private array $types = [ | ||
| 'int' => '/^[0-9]+$/', | ||
| 'string' => '/^[a-zA-Z]+/$', | ||
| 'hash' => 'md5', | ||
| ]; |
There was a problem hiding this comment.
Regex syntax error: extra $ after delimiter.
The string type regex has an invalid trailing $ after the closing delimiter, which will cause preg_match() to fail with a warning and return false.
🐛 Proposed fix
private array $types = [
'int' => '/^[0-9]+$/',
- 'string' => '/^[a-zA-Z]+/$',
+ 'string' => '/^[a-zA-Z]+$/',
'hash' => 'md5',
];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private array $types = [ | |
| 'int' => '/^[0-9]+$/', | |
| 'string' => '/^[a-zA-Z]+/$', | |
| 'hash' => 'md5', | |
| ]; | |
| private array $types = [ | |
| 'int' => '/^[0-9]+$/', | |
| 'string' => '/^[a-zA-Z]+$/', | |
| 'hash' => 'md5', | |
| ]; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/silverengine/core/src/Core/Route.php` around lines 43 - 47, The
'string' regex in the Route class types array is malformed (extra `$` after the
closing delimiter) causing preg_match warnings; update the 'string' entry in the
private array $types within class Route to a valid pattern that anchors the end
inside the delimiters (e.g. ensure the pattern ends with +$ before the closing
slash) so preg_match() works correctly.
Comment on lines
+57
to
62
| $loadRouteFile = static function (string $path, Route $route): void { | ||
| include_once $path; | ||
| }; | ||
| foreach (Env::get('routes', []) as $routeFile) { | ||
| $loadRouteFile(ROOT . $routeFile . '.php', $router); | ||
| } |
There was a problem hiding this comment.
Replace include_once with guarded include to avoid skipped route registration.
At Line 58, include_once can skip route file execution if the file was already loaded earlier in the same process; after Line 56 (reset()), that can leave route cache generation incomplete. Also, the path should be validated before include to prevent accidental out-of-root loads.
Suggested patch
- $loadRouteFile = static function (string $path, Route $route): void {
- include_once $path;
- };
+ $loadRouteFile = static function (string $path, Route $route): void {
+ $real = realpath($path);
+ $root = rtrim((string) realpath(ROOT), DIRECTORY_SEPARATOR) . DIRECTORY_SEPARATOR;
+
+ if ($real === false || !is_file($real) || !str_starts_with($real, $root)) {
+ throw new \RuntimeException("Invalid route file: {$path}");
+ }
+
+ include $real;
+ };
foreach (Env::get('routes', []) as $routeFile) {
- $loadRouteFile(ROOT . $routeFile . '.php', $router);
+ try {
+ $loadRouteFile(ROOT . $routeFile . '.php', $router);
+ } catch (\RuntimeException $e) {
+ $this->error($e->getMessage());
+ return;
+ }
}🧰 Tools
🪛 OpenGrep (1.21.0)
[ERROR] 58-58: Dynamic file path passed to include/require. This can lead to local or remote file inclusion. Use a fixed allowlist of paths.
(coderabbit.file-inclusion.php-dynamic-include)
🪛 PHPMD (2.15.0)
[warning] 57-57: Avoid unused parameters such as '$route'. (undefined)
(UnusedFormalParameter)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/silverengine/core/src/Engine/CLI.php` around lines 57 - 62, The
$loadRouteFile closure currently uses include_once which can skip execution if a
route file was previously loaded (breaking route registration after reset());
change it to use a plain include guarded by checks: validate the constructed
path before including by ensuring it is a real file (file_exists/is_readable)
and that its resolved realpath is under the ROOT directory to prevent
out-of-root loads, then perform include so the file always executes when
expected (reference the $loadRouteFile closure and the loop iterating
Env::get('routes')).
Comment on lines
+45
to
+50
| public function write(string $path, string $contents, bool $lock = false): int | ||
| { | ||
| return $this->intercept(__FUNCTION__, [$path, $contents, $lock], static function (string $p, string $c, bool $l): int { | ||
| $bytes = file_put_contents($p, $c, $l ? LOCK_EX : 0); | ||
| return $bytes === false ? 0 : $bytes; | ||
| }); |
There was a problem hiding this comment.
Disambiguate write failure from zero-byte success.
write()/append() currently collapse failure and valid zero-byte writes into the same 0 result, so callers cannot reliably detect I/O errors.
Proposed fix
public function write(string $path, string $contents, bool $lock = false): int
{
return $this->intercept(__FUNCTION__, [$path, $contents, $lock], static function (string $p, string $c, bool $l): int {
$bytes = file_put_contents($p, $c, $l ? LOCK_EX : 0);
- return $bytes === false ? 0 : $bytes;
+ if ($bytes === false) {
+ throw new Exception("FileSystem: cannot write '{$p}'.");
+ }
+ return $bytes;
});
}
public function append(string $path, string $contents): int
{
return $this->intercept(__FUNCTION__, [$path, $contents], static function (string $p, string $c): int {
$bytes = file_put_contents($p, $c, FILE_APPEND | LOCK_EX);
- return $bytes === false ? 0 : $bytes;
+ if ($bytes === false) {
+ throw new Exception("FileSystem: cannot append '{$p}'.");
+ }
+ return $bytes;
});
}Also applies to: 53-58
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/silverengine/core/src/FileSystem/LocalFileSystem.php` around lines
45 - 50, The write() (and append()) implementation currently returns 0 for both
a failed file_put_contents and a legitimate zero-byte write; change the behavior
to distinguish error vs zero-length success by detecting file_put_contents ===
false and throwing an exception (e.g., RuntimeException) with contextual message
rather than returning 0, while returning the integer byte count for successes
(including 0). Update the closures passed to intercept in LocalFileSystem::write
and LocalFileSystem::append to check for === false and throw, keeping the rest
of the intercept call/site unchanged so callers can catch the exception instead
of relying on ambiguous 0.
Comment on lines
+31
to
+32
| $url = '/' . ltrim($url, '/'); | ||
|
|
There was a problem hiding this comment.
Escape or validate URL before embedding in generated PHP code.
$url is inserted into a single-quoted PHP string without escaping; a URL containing ' will generate invalid PHP.
Proposed fix
$name = self::sanitiseName($name);
$url = '/' . ltrim($url, '/');
+if (!preg_match('#^/[A-Za-z0-9/_\-{}]*$#', $url)) {
+ throw new RuntimeException("Invalid scaffold URL: '{$url}'.");
+}
...
-$routeLine = "\$route->get('{$url}', {$name}Controller::class);";
+$safeUrl = str_replace("'", "\\'", $url);
+$routeLine = "\$route->get('{$safeUrl}', {$name}Controller::class);";Also applies to: 153-153
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/silverengine/core/src/Support/Scaffolder.php` around lines 31 - 32,
The generated PHP embeds $url into a single-quoted string without escaping, so
any single-quote in $url will break the output; update the Scaffolder code that
builds the URL (the assignment using $url = '/' . ltrim($url, '/'); and the
other occurrence around line 153) to either validate the URL (e.g.,
filter_var($url, FILTER_VALIDATE_URL) and reject/normalize invalid input) and
then escape single quotes before embedding (e.g., $safeUrl = str_replace("'",
"\\'", $url) or use var_export($url, true) / addslashes($url)) so the generated
single-quoted PHP string remains syntactically correct. Ensure you apply the
same change at both places where $url is embedded.
Comment on lines
+40
to
+43
| if (!$this->fs->isFile($controllerPath)) { | ||
| $this->fs->write($controllerPath, $this->controllerStub($name)); | ||
| $created[] = 'app/Controllers/' . $name . 'Controller.php'; | ||
| } |
There was a problem hiding this comment.
Check write outcomes before marking artifacts as created.
created[] is updated even if filesystem writes fail, which can report success while nothing was written.
Proposed fix
if (!$this->fs->isFile($controllerPath)) {
- $this->fs->write($controllerPath, $this->controllerStub($name));
+ if ($this->fs->write($controllerPath, $this->controllerStub($name)) <= 0) {
+ throw new RuntimeException("Failed to write controller: {$controllerPath}");
+ }
$created[] = 'app/Controllers/' . $name . 'Controller.php';
}
if (!$this->fs->isFile($vuePath)) {
$this->fs->mkdir(dirname($vuePath));
- $this->fs->write($vuePath, $this->vueStub($name, $url));
+ if ($this->fs->write($vuePath, $this->vueStub($name, $url)) <= 0) {
+ throw new RuntimeException("Failed to write page: {$vuePath}");
+ }
$created[] = 'app/Resources/js/Pages/' . $name . '.vue';
}
...
-$this->fs->write($path, $src);
+if ($this->fs->write($path, $src) <= 0) {
+ throw new RuntimeException("Failed to update routes file: {$path}");
+}
return true;Also applies to: 45-48, 172-173
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/silverengine/core/src/Support/Scaffolder.php` around lines 40 - 43,
The code currently appends entries to the created[] array immediately after
calling $this->fs->write (e.g., the controller creation block using
$this->fs->isFile, $this->fs->write and $this->controllerStub($name)), which can
record artifacts as created even if the write failed; update those blocks
(including the similar spots around lines 45-48 and 172-173) to check the return
value or throw status of $this->fs->write and only push the corresponding
'app/Controllers/...Controller.php' (and other artifact paths) into $created
when the write succeeded, otherwise handle/log the failure and do not mark the
artifact as created.
Comment on lines
+159
to
+167
| // Insert the `use` after the last existing `use ...;` line. | ||
| if (!str_contains($src, $useLine)) { | ||
| $src = preg_replace( | ||
| '/(^use [^\n]+;\n)(?!.*^use [^\n]+;\n)/ms', | ||
| "$1" . $useLine . "\n", | ||
| $src, | ||
| 1, | ||
| ) ?? $src; | ||
| } |
There was a problem hiding this comment.
Handle Web.php files that have no existing use statements.
If no use ...; exists, the regex replacement is a no-op and the route is appended without importing {$name}Controller, which can break route file loading.
Proposed fix
if (!str_contains($src, $useLine)) {
- $src = preg_replace(
+ $updated = preg_replace(
'/(^use [^\n]+;\n)(?!.*^use [^\n]+;\n)/ms',
"$1" . $useLine . "\n",
$src,
1,
- ) ?? $src;
+ );
+ if ($updated !== null && $updated !== $src) {
+ $src = $updated;
+ } else {
+ // Fallback: insert after namespace line.
+ $src = preg_replace('/^(namespace[^\n]+;\n)/m', "$1\n{$useLine}\n", $src, 1) ?? $src;
+ }
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/silverengine/core/src/Support/Scaffolder.php` around lines 159 -
167, The current insertion logic in Scaffolder.php checks for existing use
statements and uses preg_replace with a pattern that is a no-op when no use
lines exist, so $useLine never gets added (leading to missing imports for
{$name}Controller). Fix by detecting the absence of any "use ...;" lines (using
the existing str_contains($src, $useLine) check or a preg_match for '^use ')
and, when none exist, insert $useLine right after the opening PHP tag (<?php) or
at the top of the file before any routes definitions; update the code paths
around $src, $useLine and the preg_replace call so the fallback branch performs
a safe insert after the <?php header.
Comment on lines
+35
to
+39
| $tmp = '/tmp/hooked-hook-target.txt'; | ||
| @unlink($tmp); | ||
|
|
||
| $fs->write('/anywhere/hook-target.txt', 'payload'); | ||
|
|
There was a problem hiding this comment.
Use a unique temp target to avoid parallel-test collisions.
Line 35 uses a fixed /tmp filename; parallel workers can race on the same file and create flaky outcomes.
Suggested patch
- $tmp = '/tmp/hooked-hook-target.txt';
+ $token = uniqid('hook-target-', true) . '.txt';
+ $tmp = '/tmp/hooked-' . $token;
`@unlink`($tmp);
- $fs->write('/anywhere/hook-target.txt', 'payload');
+ $fs->write('/anywhere/' . $token, 'payload');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| $tmp = '/tmp/hooked-hook-target.txt'; | |
| @unlink($tmp); | |
| $fs->write('/anywhere/hook-target.txt', 'payload'); | |
| $token = uniqid('hook-target-', true) . '.txt'; | |
| $tmp = '/tmp/hooked-' . $token; | |
| `@unlink`($tmp); | |
| $fs->write('/anywhere/' . $token, 'payload'); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/Unit/Framework/Core/HookTest.php` around lines 35 - 39, The test uses a
fixed tmp path ($tmp = '/tmp/hooked-hook-target.txt') and a fixed fs write
target ('/anywhere/hook-target.txt') which can collide in parallel runs; change
the $tmp assignment to generate a unique temp filename (e.g., using
tempnam(sys_get_temp_dir(), 'hooked-') or uniqid) and update the corresponding
fs->write target to use that same unique path so each test worker operates on
its own file (adjust references to $tmp and the string literal
'/anywhere/hook-target.txt' in HookTest.php).
Nejcc
added a commit
that referenced
this pull request
May 27, 2026
refactor(core): convert framework statics to instance + container sin…
This was referenced May 27, 2026
Merged
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
1 participant
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.
…gletons
Drop static state from Route, Hook, Config, DI, ErrorHandler, ConnectionManager, TransactionManager, DebugTimer, RequestRecorder — each is now a regular class registered as a singleton on the app container and resolved through DI.
83/83 tests passing, full request pipeline + CLI optimize verified.
Summary by CodeRabbit
New Features
Bug Fixes
Chores
phpdotenvdependency; now uses native .env parsing