Conversation
* Delete Core/Instances.php (38 LoC). Documented as the predecessor of Container; Container is its declared 'strict superset'. Only callers were 2 contract tests, migrated to instantiate Container directly. * Kill the broken Response::json(string $payload) — was wired backwards (json_decode instead of json_encode, malformed header) and never called. Delete the placeholder Response::xml() too (returned the input wrapped in spaces). * Add Response::json(mixed $body, int $code = 200, int $flags = …): string — single chokepoint for the (setCode → setHeader → json_encode) ritual. Already-encoded strings pass through verbatim. HeartbeatController and ScaffoldController now route their JSON paths through it. * Add is_class(mixed): bool global (Core/helpers.php). Replaces 9 copies of `is_string($x) && class_exists($x)` across Db.php (5 sites) and Heartbeat.php (4 sites, including the negated form). * ScaffoldController: stop calling header() / http_response_code() directly — go through $response. Drop the private json() helper now that Response::json() exists. Extract a small redirect() helper for the 4 303 paths that previously each set their own header+code.
…n/xml Restores XML as a first-class output format alongside JSON — the stubs I deleted earlier were genuinely broken (Response::xml returned its input wrapped in spaces) but the *idea* of supporting both for API clients is valid, so put it back properly. * Response::xml(string|array|SimpleXMLElement, code, rootElement) mirrors Response::json(). Pre-rendered strings pass through; SimpleXMLElement gets ->asXML(); arrays recursively serialize under a configurable root element. Numeric keys become <item index="N"> so the output stays well-formed; element names get sanitised. * Response::send() Accept negotiation now matches application/xml and text/xml in addition to the existing application/json / text/html / text/* / */*. * The body dispatch in send() gains an XML arm: strings pass through, SimpleXMLElement uses asXML(), RenderInterface emits its data(), arrays/objects serialize via Response::xml().
Two new test files locking in the surface added during the DRY pass:
* tests/Unit/Framework/Http/ResponseTest.php (11 tests)
- json(): array encode + headers, custom status code, string passthrough,
JSON_UNESCAPED_SLASHES flag respected
- xml(): array serialisation with root element, custom status code,
pre-rendered string passthrough, SimpleXMLElement input,
numeric-key indexed items, invalid-element-name sanitisation,
nested array roundtrip
* tests/Unit/Framework/Core/HelpersTest.php (7 tests)
- is_class(): true for existing class, false for unknown string,
non-string types, function names (functions aren't classes)
- env(): default fallback, true/false/null/empty sentinel coercion,
raw string passthrough
Full suite stays green: 101 tests, 218 assertions.
|
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:
📝 WalkthroughWalkthroughThis PR converts several static/shared APIs to container-resolved instances: Response gains JSON/XML builders, Validator becomes instance-based returning ValidationResult, View state moves to ViewRegistry, Scaffolder expands artifact types and removes env gating, Ghost engine and CLI are refactored, DB debug becomes instance-scoped, and the platform requirement is raised to PHP 8.5. ChangesHTTP Response, Validation, and View Architecture
Scaffolder Expansion and CLI Refactoring
Ghost Template Engine: Compiler and Template Refactoring
Facade Reorganization and Configuration Updates
Database Debug State, Query Builders, and Helper Refactoring
PHP 8.5 Platform Upgrade and Documentation
🎯 4 (Complex) | ⏱️ ~90 minutes Possibly Related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Three independent cleanups in one commit; together they remove 4 files, strip ~1 KB of legacy boilerplate from configs, and convert the Validator's silently-lying behaviour into loud-fail. * Delete Init/Image.php + Init/Silver.php + the (unrouted) SystemController that called them. The 'framework self-update' mechanism they implemented was hard-wired to filesystem paths that don't exist post-modernization (System/init/images/, URL.'/public/images.zip'). Nothing in the codebase references SystemController anymore. * Strip the 2015-2017 'SilverEngine Team' phpdoc block from Alias.php / App.php / Middlewares.php / Routes.php / lang.php. Pure noise — every file was reduced by ~205 bytes of identical boilerplate. * Delete Http\Validator::checkExist + checkUnique stubs. Both were literal 'return false;' — meaning rules like 'email' => 'required|unique' silently passed every value, giving callers false confidence the check was happening. They aren't. Without the stubs, attempting to use those rule names now throws (method-not-found), which is the correct outcome for an unimplemented validator. Issue #19 closed as obsolete by deletion. * Add tests/Unit/Framework/Http/ValidatorTest.php — locks in the rules that DO work (required, min, max, match), the chained-rule behaviour the .pass() / .get() helpers, AND the new contract that unimplemented rule names fail loud rather than silent-passing. Suite: 115 tests / 240 assertions / 1 pre-existing skip, all green.
BREAKING — the framework already uses PHP 8.5 natives (array_find, array_any) and would fatal on 8.4 at first request. Make the constraint honest and align all the user-facing surface. * composer.json (root + packages/silverengine/core) → "php": ">=8.5" * Heartbeat::MIN_PHP → '8.5.0' (the runtime self-check now demands what the code actually requires) * README, CLAUDE.md, About.vue, Welcome.vue, default.ghost.php — every '8.4+' callout updated to '8.5+' * public/docs/index.html — body text + the example heartbeat JSON snippet bumped for consistency * README: drop the now-stale 'legacy Instances registry preserved' paragraph (the class was deleted in eec1dae; Container kept the same method names, but Instances the class no longer exists)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/Http/Response.php`:
- Line 93: Response::xml() currently injects $rootElement directly into the
SimpleXMLElement constructor which can break XML; validate and sanitize
$rootElement before use (e.g., in Response::xml()) by: check against a safe
XML-name pattern (like /^[A-Za-z_][A-Za-z0-9\-\._]*$/), if it fails replace
invalid characters with '_' and ensure the first character is a letter or '_'
(prepend 'root_' if needed), then use the sanitized name when building the
SimpleXMLElement instead of the raw $rootElement.
- Line 65: Response::json() and the application/json branches in send()
currently cast json_encode() results to string, which hides encoding failures;
change both Response::json() and the json/array/object handling in send() to
check json_encode() return value !== false, and on failure throw a
RuntimeException (or return a 500 error) including json_last_error_msg() and
json_last_error() so callers get explicit errors instead of empty bodies; also
update Response::xml() to validate/sanitize the $rootElement (e.g., allow only
valid XML name characters or replace/escape invalid ones and ensure it’s
non-empty) before passing it to new SimpleXMLElement to prevent
injection/invalid-name errors.
🪄 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: 9d530e3c-cdc6-41f4-8dbd-8ba2f472e5d7
📒 Files selected for processing (20)
packages/silverengine/core/src/App/Controllers/HeartbeatController.phppackages/silverengine/core/src/App/Controllers/ScaffoldController.phppackages/silverengine/core/src/App/Controllers/SystemController.phppackages/silverengine/core/src/Config/Alias.phppackages/silverengine/core/src/Config/App.phppackages/silverengine/core/src/Config/Middlewares.phppackages/silverengine/core/src/Config/Routes.phppackages/silverengine/core/src/Config/lang.phppackages/silverengine/core/src/Core/Instances.phppackages/silverengine/core/src/Core/helpers.phppackages/silverengine/core/src/Database/Db.phppackages/silverengine/core/src/Http/Response.phppackages/silverengine/core/src/Http/Validator.phppackages/silverengine/core/src/Init/Image.phppackages/silverengine/core/src/Init/Silver.phppackages/silverengine/core/src/Support/Heartbeat.phptests/Unit/Framework/Core/ContainerContractTest.phptests/Unit/Framework/Core/HelpersTest.phptests/Unit/Framework/Http/ResponseTest.phptests/Unit/Framework/Http/ValidatorTest.php
💤 Files with no reviewable changes (10)
- packages/silverengine/core/src/Config/Middlewares.php
- packages/silverengine/core/src/Config/Alias.php
- packages/silverengine/core/src/Init/Silver.php
- packages/silverengine/core/src/Config/Routes.php
- packages/silverengine/core/src/Http/Validator.php
- packages/silverengine/core/src/Config/App.php
- packages/silverengine/core/src/Core/Instances.php
- packages/silverengine/core/src/Init/Image.php
- packages/silverengine/core/src/App/Controllers/SystemController.php
- packages/silverengine/core/src/Config/lang.php
| ): string { | ||
| $this->setCode($code); | ||
| $this->setHeader('Content-Type', 'application/json; charset=utf-8'); | ||
| return is_string($body) ? $body : (string) json_encode($body, $flags); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the file
ls -la packages/silverengine/core/src/Http/Response.php
# Print the relevant sections with line numbers (around json(), xml(), and send())
sed -n '1,220p' packages/silverengine/core/src/Http/Response.php | nl -ba | sed -n '1,220p'Repository: SilverEngine/Framework
Length of output: 225
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/silverengine/core/src/Http/Response.php"
echo "== $FILE =="
wc -l "$FILE"
echo "== json() and related around line 55-120 =="
sed -n '40,130p' "$FILE" | cat -n
echo "== xml() and any root element validation around line 80-170 =="
sed -n '80,190p' "$FILE" | cat -n
echo "== send() / content-type negotiation around line 130-280 =="
sed -n '120,280p' "$FILE" | cat -nRepository: SilverEngine/Framework
Length of output: 14724
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "->xml\(" packages/silverengine/core/src | head -n 50
rg -n "Response::xml\(" packages/silverengine | head -n 50
rg -n "function xml\(" packages/silverengine/core/src/Http/Response.php
rg -n "\$rootElement" packages/silverengine/core/src | head -n 50Repository: SilverEngine/Framework
Length of output: 471
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -- '->xml\(' packages/silverengine/core/src | head -n 50
rg -n -- 'Response::xml\(' packages/silverengine | head -n 50
rg -n -- '\$rootElement' packages/silverengine/core/src | head -n 50
# Also find xml() call sites that pass 3rd arg (root element)
rg -n -- '->xml\([^)]*,[^)]*,[^)]*\)' packages/silverengine/core/src | head -n 50
rg -n -- '->xml\([^)]*,[^)]*,[^)]*\)' packages/silverengine | head -n 50Repository: SilverEngine/Framework
Length of output: 331
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find all xml() method calls on Response instances
rg -n -- '->xml\(' packages | head -n 200
# Find xml() calls that pass 3 arguments (body, code, rootElement)
rg -n -- '->xml\([^,]+,[^,]+,[^)]+\)' packages | head -n 200
# Also check direct calls via $this->xml(...) with explicit third arg
rg -n -- '\$this->xml\([^,]+,[^,]+,[^)]+\)' packages | head -n 200
# Check for named argument usage that could set rootElement
rg -n -- '->xml\([^)]*rootElement\s*=>' packages | head -n 200Repository: SilverEngine/Framework
Length of output: 331
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/silverengine/core/src/Http/Response.php"
python3 - <<'PY'
import itertools
path = "packages/silverengine/core/src/Http/Response.php"
with open(path,'r',encoding='utf-8') as f:
lines = f.readlines()
targets = [62,63,64,65,66,67,68,69,70,85,90,93,94] # around json and near where root might appear
for i in targets:
if 1 <= i <= len(lines):
print(f"{i}: {lines[i-1].rstrip()}")
PYRepository: SilverEngine/Framework
Length of output: 671
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show all xml() call sites (Response instances / other helpers)
rg -n -- '->xml\(' packages/silverengine | head -n 200
rg -n -- '\$this->xml\(' packages/silverengine | head -n 200
rg -n -- 'Response::xml\(' packages/silverengine | head -n 200
# Try to catch 3-arg calls more robustly (works best for simple single-line calls)
rg -n -- '->xml\([^()]*,[^()]*,[^()]*\)' packages/silverengine | head -n 200
rg -n -- '\$this->xml\([^()]*,[^()]*,[^()]*\)' packages/silverengine | head -n 200Repository: SilverEngine/Framework
Length of output: 596
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Matches for '->xml(' =="
rg -n -- '->xml\(' packages | wc -l
rg -n -C 2 -- '->xml\(' packages | head -n 200
echo "== Matches for '\$this->xml(' =="
rg -n -- '\$this->xml\(' packages | wc -l
rg -n -C 2 -- '\$this->xml\(' packages | head -n 200
echo "== Matches for 'Response::xml(' =="
rg -n -- 'Response::xml\(' packages | wc -l
rg -n -C 2 -- 'Response::xml\(' packages | head -n 200Repository: SilverEngine/Framework
Length of output: 1568
Handle JSON encoding failures explicitly.
Response::json()currently castsjson_encode()to string:(string) json_encode(...), which can silently produce an empty body on encoding failure while still returning a successful response code.send()also contains uncheckedjson_encode()calls forapplication/json(and array/object cases), so failures can still result in empty output.Response::xml()interpolates$rootElementdirectly into theSimpleXMLElementconstructor; within this repo it’s only called with the default root element, but validate/sanitize$rootElementfor external callers.
Proposed fix
- return is_string($body) ? $body : (string) json_encode($body, $flags);
+ if (is_string($body)) {
+ return $body;
+ }
+ return json_encode($body, $flags | JSON_THROW_ON_ERROR);🤖 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/Http/Response.php` at line 65,
Response::json() and the application/json branches in send() currently cast
json_encode() results to string, which hides encoding failures; change both
Response::json() and the json/array/object handling in send() to check
json_encode() return value !== false, and on failure throw a RuntimeException
(or return a 500 error) including json_last_error_msg() and json_last_error() so
callers get explicit errors instead of empty bodies; also update Response::xml()
to validate/sanitize the $rootElement (e.g., allow only valid XML name
characters or replace/escape invalid ones and ensure it’s non-empty) before
passing it to new SimpleXMLElement to prevent injection/invalid-name errors.
| return (string) $body->asXML(); | ||
| } | ||
|
|
||
| $root = new \SimpleXMLElement('<?xml version="1.0" encoding="UTF-8"?><' . $rootElement . '/>'); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/silverengine/core/src/Http/Response.php"
echo "== File exists =="
ls -l "$FILE"
echo
echo "== Context around line 93 =="
nl -ba "$FILE" | sed -n '60,120p'
echo
echo "== Find all usages of \$rootElement in this file =="
rg -n "\$rootElement" "$FILE"
echo
echo "== Trace where rootElement is assigned in this file (near occurrences) =="
# Print a bit of context around each assignment
rg -n "\$rootElement\s*=" "$FILE" -n || true
# Better: show previous/next 30 lines around matches
for m in $(rg -n "\$rootElement" "$FILE" | cut -d: -f1 | sort -n | uniq); do
echo
echo "--- Around line $m ---"
nl -ba "$FILE" | sed -n "$((m-25)),$((m+25))p"
doneRepository: SilverEngine/Framework
Length of output: 274
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/silverengine/core/src/Http/Response.php"
echo "== Context around line 93 (numbered via awk) =="
awk 'NR>=60 && NR<=120 {printf "%6d\t%s\n", NR, $0}' "$FILE"
echo
echo "== Find all usages of \$rootElement in this file =="
rg -n "\$rootElement" "$FILE"
echo
echo "== Show assignments to \$rootElement =="
rg -n "\$rootElement\s*=" "$FILE" || true
echo
echo "== Show any parsing/validation of root element around usages =="
# Search nearby for common patterns
rg -n "(SimpleXMLElement|sanitize|preg_match|preg_replace|xml|rootElement)" "$FILE"Repository: SilverEngine/Framework
Length of output: 3015
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/silverengine/core/src/Http/Response.php"
echo "== Search for 'rootElement' (no $) =="
rg -n "rootElement" "$FILE" || true
echo
echo "== Search for 'function xml(' in Response.php =="
rg -n "function xml\(" "$FILE"
echo
echo "== Search for calls to ->xml(...) passing 3rd argument (root element) =="
rg -n "->xml\([^,]+,[^,]+,[^)]+\)" -S . || true
echo
echo "== Search for calls to xml(...) with 3 arguments (root element) =="
rg -n "\bxml\([^,]+,[^,]+,[^)]+\)" -S . || trueRepository: SilverEngine/Framework
Length of output: 1103
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/silverengine/core/src/Http/Response.php"
TEST="tests/Unit/Framework/Http/ResponseTest.php"
echo "== Response.php: xml() and arrayToXml() (lines 70-150) =="
awk 'NR>=70 && NR<=160 {printf "%6d\t%s\n", NR, $0}' "$FILE"
echo
echo "== ResponseTest.php: around line 62 =="
awk 'NR>=1 && NR<=120 {printf "%6d\t%s\n", NR, $0}' "$TEST"
echo
echo "== grep/rg for 'envelope' usage in tests =="
rg -n "envelope" "$TEST" || trueRepository: SilverEngine/Framework
Length of output: 9090
Sanitize $rootElement before using it as the XML root element name in Response::xml()
$rootElement is a Response::xml() parameter and is currently concatenated directly into SimpleXMLElement('<?xml...?><' . $rootElement . '/>'), so invalid characters or names can make the generated XML malformed at runtime.
Proposed fix
- $root = new \SimpleXMLElement('<?xml version="1.0" encoding="UTF-8"?><' . $rootElement . '/>');
+ $name = preg_replace('/[^A-Za-z0-9_\-]/', '_', $rootElement) ?: 'response';
+ if (!preg_match('/^[A-Za-z_]/', $name)) {
+ $name = '_' . $name;
+ }
+ $root = new \SimpleXMLElement('<?xml version="1.0" encoding="UTF-8"?><' . $name . '/>');📝 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
| $root = new \SimpleXMLElement('<?xml version="1.0" encoding="UTF-8"?><' . $rootElement . '/>'); | |
| $name = preg_replace('/[^A-Za-z0-9_\-]/', '_', $rootElement) ?: 'response'; | |
| if (!preg_match('/^[A-Za-z_]/', $name)) { | |
| $name = '_' . $name; | |
| } | |
| $root = new \SimpleXMLElement('<?xml version="1.0" encoding="UTF-8"?><' . $name . '/>'); |
🤖 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/Http/Response.php` at line 93, Response::xml()
currently injects $rootElement directly into the SimpleXMLElement constructor
which can break XML; validate and sanitize $rootElement before use (e.g., in
Response::xml()) by: check against a safe XML-name pattern (like
/^[A-Za-z_][A-Za-z0-9\-\._]*$/), if it fails replace invalid characters with '_'
and ensure the first character is a letter or '_' (prepend 'root_' if needed),
then use the sanitized name when building the SimpleXMLElement instead of the
raw $rootElement.
BREAKING — CLI 'g/d' commands now produce modern stubs (final, strict types, single-action __invoke controllers, Wisp-aware pages). Anything relying on the old multi-method (get/post/put/delete) controller stub or the ReflectORM-style model properties (selectable/hidden/etc) needs to update. Output paths are unchanged for the types CLI already had. Single source of truth: Silver\Support\Scaffolder owns every stub shape. The web /new UI and 'php silver g' both call Scaffolder::create() + ::remove() — no second implementation, no second set of stubs. * Scaffolder: add three more types (view / helper / facade) so the service covers CLI's full surface. view writes a Ghost template under app/Resources/views/, helper writes a final class under app/Helpers/, facade extends Silver\Support\Facade pointed at <Name>Service. * Scaffolder: drop guardEnv() — the local+debug gate was duplicated by the HTTP controllers (ScaffoldController, NewController) that wrap it. Removing it lets 'php silver g foo' work in any environment locally (the web endpoints stay gated by their controllers). * CLI: delete ~140 LoC of generate/deleteResources/resolvePaths/ generateFile/generateView/fixRoutes. Replaced by ~25 lines that delegate to Scaffolder + render per-file 'created/removed' progress. * CLI: help text bumped to list every type Scaffolder supports. * Delete legacy app/Templates/*.tpl (7 files). They contained stale pre-Wisp / pre-ReflectORM-removal templates that have been emitting broken code for the last modernization cycle. * tests/Unit/Framework/Support/ScaffolderTypesTest.php — locks in view/helper/facade stub shape + on-disk locations + the TYPES constant list. Suite: 120 tests / 262 assertions / 1 skipped, all green.
Studly-casing pipeline used to strtolower(every-part) then ucfirst — so
already-PascalCase inputs got their internal capitals erased:
Before: 'SmokeModel' -> 'Smokemodel.php' (CLI smoke test surfaced this)
After: 'SmokeModel' -> 'SmokeModel.php' ✓
Split on the lowercase->uppercase boundary too (plus the ALL_CAPS->Word
boundary so e.g. 'XMLParser' splits cleanly). Existing URL slug behaviour
('foo-bar-baz' -> 'FooBarBaz') and snake_case ('user_post' -> 'UserPost')
are unchanged.
+4 tests in ScaffolderTypesTest pinning the new behaviour.
Four small wins inside the Ghost template engine, found via a focused
DRY audit:
* Template::resolveView($alias, $subdir) — extracts the dotted-alias
-> 'app/Views/{subdir}{path}.ghost.{php,tpl}' fallback. Used to live
inline in parseMaster(), includeFile() and includeComponent(); all
three now go through the same helper. Single point to evolve view
lookup later (e.g. searching multiple roots).
* Compiler::freshDeps($cacheFile, $sourceFile) — single-pass replacement
for the prior 'isFresh() then depsFrom()' two-call pattern that opened
and parsed the same cache header file twice on every cache hit.
isFresh() kept as a one-line shim over freshDeps(); depsFrom() kept
with a @deprecated note for callers that only need deps.
* Template::filterControl($body, $tag) — table-drives the
#if/#foreach/#for filters that were three near-identical methods.
#if branch additionally handles #elseif/#else; the other tags just
open and close. Three methods (~20 LoC) -> one (~15 LoC).
* Delete Template::getRoute() — dead code, no callers in the codebase.
Also drops the now-unused 'use Silver\Core\Route' import.
DeferProp / LazyProp twins audited too; deliberately not merged.
Same shape, but they encode genuinely different Inertia protocols
(deferred = autoload after mount; lazy = only on explicit fetch).
A shared base would save ~4 LoC at the cost of hiding the protocol
distinction the class names + docblocks currently make explicit.
Suite: 124 tests, 272 assertions, 1 pre-existing skip. All green.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/Unit/Framework/Support/ScaffolderTypesTest.php (1)
38-54: ⚡ Quick winTest may leave artifacts on failure.
If any assertion before
remove()fails, the test file (app/Helpers/Smoke.php) will remain on disk. Consider usingtry/finallyortearDown()to ensure cleanup.♻️ Ensure cleanup with try/finally
public function testCreateHelperWritesFile(): void { $result = $this->scaffolder->create('helper', 'Smoke'); - $this->assertSame('helper', $result['type']); - $this->assertSame('Smoke', $result['name']); - $this->assertSame(['app/Helpers/Smoke.php'], $result['created']); - $path = ROOT . 'app/Helpers/Smoke.php'; - $this->assertFileExists($path); - $src = (string) file_get_contents($path); - $this->assertStringContainsString('namespace App\\Helpers', $src); - $this->assertStringContainsString('final class Smoke', $src); - $this->assertStringContainsString('declare(strict_types=1)', $src); - - $this->scaffolder->remove('helper', 'Smoke'); - $this->assertFileDoesNotExist($path); + try { + $this->assertSame('helper', $result['type']); + $this->assertSame('Smoke', $result['name']); + $this->assertSame(['app/Helpers/Smoke.php'], $result['created']); + + $this->assertFileExists($path); + $src = (string) file_get_contents($path); + $this->assertStringContainsString('namespace App\\Helpers', $src); + $this->assertStringContainsString('final class Smoke', $src); + $this->assertStringContainsString('declare(strict_types=1)', $src); + } finally { + `@unlink`($path); + } }The same pattern applies to
testCreateFacadeWritesFileandtestCreateViewWritesGhostTemplate.🤖 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/Support/ScaffolderTypesTest.php` around lines 38 - 54, The testCreateHelperWritesFile (and similarly testCreateFacadeWritesFile and testCreateViewWritesGhostTemplate) can leave app/Helpers/Smoke.php (and other scaffolded files) on disk if an assertion fails before $this->scaffolder->remove() is called; wrap the creation and assertions in a try/finally (or move cleanup into tearDown()) so $this->scaffolder->remove('helper','Smoke') always runs (use the same $path variable and the scaffolder->remove calls referenced in the test to perform cleanup), and apply the same pattern to the facade and view tests to guarantee files are deleted even on failure.packages/silverengine/core/src/Support/Scaffolder.php (1)
637-650: 💤 Low valueGenerated view template contains leading indentation.
The heredoc preserves the 12-space indentation, so the generated
.ghost.tplfile will have each line indented. While functional, this differs from other stubs which start content at column 0. Consider using a non-indented heredoc for consistency.♻️ Optional: Remove heredoc indentation
private function viewStub(string $name): string { $title = $name; - return <<<TPL - {{ extends('layouts.master') }} - - `#set`[content] - <h1>{$title}</h1> - <p>Welcome to <b>{$title}</b>. Edit this file under - <code>app/Resources/views/</code>.</p> - `#end` - - TPL; + return <<<TPL +{{ extends('layouts.master') }} + +#set[content] + <h1>{$title}</h1> + <p>Welcome to <b>{$title}</b>. Edit this file under + <code>app/Resources/views/</code>.</p> +#end + +TPL; }🤖 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 637 - 650, The heredoc in viewStub(string $name) preserves leading indentation causing generated templates to be indented; update viewStub to return a left-aligned heredoc (move the opening/lines so the template content starts at column 0 and the closing TPL marker is at column 0) or alternatively build the string and call trim() or preg_replace to strip the common leading whitespace from the returned string; ensure you reference the $title interpolation in the adjusted heredoc so the generated .ghost.tpl content matches other stubs (no leading 12-space indentation).
🤖 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.
Nitpick comments:
In `@packages/silverengine/core/src/Support/Scaffolder.php`:
- Around line 637-650: The heredoc in viewStub(string $name) preserves leading
indentation causing generated templates to be indented; update viewStub to
return a left-aligned heredoc (move the opening/lines so the template content
starts at column 0 and the closing TPL marker is at column 0) or alternatively
build the string and call trim() or preg_replace to strip the common leading
whitespace from the returned string; ensure you reference the $title
interpolation in the adjusted heredoc so the generated .ghost.tpl content
matches other stubs (no leading 12-space indentation).
In `@tests/Unit/Framework/Support/ScaffolderTypesTest.php`:
- Around line 38-54: The testCreateHelperWritesFile (and similarly
testCreateFacadeWritesFile and testCreateViewWritesGhostTemplate) can leave
app/Helpers/Smoke.php (and other scaffolded files) on disk if an assertion fails
before $this->scaffolder->remove() is called; wrap the creation and assertions
in a try/finally (or move cleanup into tearDown()) so
$this->scaffolder->remove('helper','Smoke') always runs (use the same $path
variable and the scaffolder->remove calls referenced in the test to perform
cleanup), and apply the same pattern to the facade and view tests to guarantee
files are deleted even on failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b3f2a5f8-e9aa-49b9-837f-9d5938e4f490
📒 Files selected for processing (18)
README.mdapp/Resources/js/Pages/About.vueapp/Resources/js/Pages/Welcome.vueapp/Templates/Controller.ghost.tplapp/Templates/Event.ghost.tplapp/Templates/Facade.ghost.tplapp/Templates/Helper.ghost.tplapp/Templates/Listeners.ghost.tplapp/Templates/Model.ghost.tplapp/Templates/View.ghost.tplcomposer.jsonpackages/silverengine/core/composer.jsonpackages/silverengine/core/src/App/Views/demo/default.ghost.phppackages/silverengine/core/src/Engine/CLI.phppackages/silverengine/core/src/Support/Heartbeat.phppackages/silverengine/core/src/Support/Scaffolder.phppublic/docs/index.htmltests/Unit/Framework/Support/ScaffolderTypesTest.php
💤 Files with no reviewable changes (7)
- app/Templates/View.ghost.tpl
- app/Templates/Facade.ghost.tpl
- app/Templates/Controller.ghost.tpl
- app/Templates/Event.ghost.tpl
- app/Templates/Model.ghost.tpl
- app/Templates/Helper.ghost.tpl
- app/Templates/Listeners.ghost.tpl
✅ Files skipped from review due to trivial changes (6)
- packages/silverengine/core/composer.json
- packages/silverengine/core/src/App/Views/demo/default.ghost.php
- app/Resources/js/Pages/About.vue
- public/docs/index.html
- app/Resources/js/Pages/Welcome.vue
- README.md
BREAKING — the validator API moves from `Validator::pass()/get()` shared
static state to a per-call immutable result. This was the single highest-
ROI de-static move in the audit because the old shape had a real bug:
Validator::check($formA, $rulesA); // sets static $errors
Validator::check($formB, $rulesB); // OVERWRITES it
Validator::pass(); // reads $formB's, not $formA's
Two checks in the same request would silently clobber each other.
The new shape:
$result = Validator::check($data, $rules);
if ($result->fails()) { $result->forField('email'); }
$result->passes();
$result->all();
$result->toArray();
Three pieces:
* Silver\Http\ValidationResult — new readonly value object holding the
errors map. passes() / fails() / forField() / hasField() / all() /
toArray() — every "is this OK?" question lives on the result, not on
a globally-mutable static.
* Silver\Http\Validator — rewritten as a small instance class. check()
returns a fresh ValidationResult per call. Per-rule methods now take
($value, $data, ...$args) so cross-field rules ('match') can compare
against sibling values without reaching into static $data.
* Silver\Core\Bootstrap\Facades\Validator — Laravel-style facade so
the call site `Validator::check(...)` keeps working. Behind __callStatic
it resolves the instance from the container — tests substitute via
Container::instance(\Silver\Http\Validator::class, $fake), the
framework gets one shared resolver, and the result is per-call regardless.
Tests rewritten end-to-end (15 cases, 26 assertions) including the
exact regression case the static state would have failed:
testTwoSequentialChecksDoNotClobberEachOther. Suite: 125 / 276 / 1 skipped.
Why a facade rather than full DI everywhere? The audit honestly mapped
which statics carry hidden state vs which are stateless value/util types
(see chat log). Validator was Category 3 — "hiding state behind static".
Categories 1, 2 and 4 (Env, Parts/, helpers, Wisp::el, Compiler dep
tracking) keep their statics; they don't have this bug class.
…pace
Two related moves in one commit.
A. Kill static state on View (the second de-static target)
View::$shared and View::$composers used to be private static arrays.
Two different parts of a request couldn't sensibly register composers
in isolation, and tests had to manually `View::flushShared()` between
cases to avoid leaks.
* Silver\Http\ViewRegistry — new instance class owning shared() /
share() / composer() / sharedFor() / reset(). Registered as a
container singleton in App::bindFrameworkDefaults().
* Silver\Http\View — static surface preserved (share / shared /
composer / flushShared / sharedFor) but now delegates to the
container-resolved registry. Every existing call site keeps working
unchanged; tests can swap a fresh registry via
Container::instance(ViewRegistry::class, new ViewRegistry()).
* Also register Silver\Http\Validator as a singleton — required for
the facade to dispatch consistently across requests.
B. Move facades from Core/Bootstrap/Facades/ → Support/Facades/
The five facades (FakeFactory, Log, Request, Response, Validator)
moved next to their base class Silver\Support\Facade. The old
location lived under 'Bootstrap' which never made sense for these
call sites.
* Namespace migration: Silver\Core\Bootstrap\Facades\* →
Silver\Support\Facades\* (matches naming the user asked for:
class name without the 'Facade' suffix, 'Facades' in the namespace).
* Two external callers updated: Core/ErrorHandler.php (Request
facade import) and tests/Unit/Framework/Http/ValidatorTest.php
(ValidatorFacade alias). Core/Config.php docblock reference too.
Suite: 125 / 276 / 1 skipped, all green. Routes 200.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/Http/Validator.php`:
- Around line 61-65: The checkRequired method incorrectly treats falsy-but-valid
values (0, "0", false) as missing due to a loose truthiness test; update
checkRequired to only fail when the value is actually absent/empty (e.g., null
or the empty string) instead of any falsy value. In function checkRequired
replace the ternary ($value ? false : 'KEY is required!') with an explicit check
such as if ($value === null || $value === '') return 'KEY is required!'; return
false; so that 0, "0", and false are accepted as present. Ensure you keep the
function signature and name (checkRequired) intact.
🪄 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: b69c7dfb-4dfb-436c-8e02-8e783fe0e1d4
📒 Files selected for processing (6)
packages/silverengine/core/src/Core/Bootstrap/Facades/Validator.phppackages/silverengine/core/src/Engine/Ghost/Compiler.phppackages/silverengine/core/src/Engine/Ghost/Template.phppackages/silverengine/core/src/Http/ValidationResult.phppackages/silverengine/core/src/Http/Validator.phptests/Unit/Framework/Http/ValidatorTest.php
✅ Files skipped from review due to trivial changes (1)
- packages/silverengine/core/src/Core/Bootstrap/Facades/Validator.php
Comment on lines
+61
to
65
| private function checkRequired(mixed $value, array $data): string|false | ||
| { | ||
| return empty(self::$errors); | ||
| unset($data); | ||
| return $value ? false : 'KEY is required!'; | ||
| } |
There was a problem hiding this comment.
checkRequired treats valid falsy values (like 0 or '0') as missing.
The truthiness check $value ? false : ... will incorrectly fail validation for legitimate values like integer 0, string '0', or false (if that's a valid input). Consider checking for actual absence/emptiness instead.
Proposed fix
private function checkRequired(mixed $value, array $data): string|false
{
unset($data);
- return $value ? false : 'KEY is required!';
+ return ($value !== null && $value !== '') ? false : 'KEY is required!';
}🧰 Tools
🪛 PHPMD (2.15.0)
[warning] 61-65: Avoid unused private methods such as 'checkRequired'. (undefined)
(UnusedPrivateMethod)
🤖 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/Http/Validator.php` around lines 61 - 65, The
checkRequired method incorrectly treats falsy-but-valid values (0, "0", false)
as missing due to a loose truthiness test; update checkRequired to only fail
when the value is actually absent/empty (e.g., null or the empty string) instead
of any falsy value. In function checkRequired replace the ternary ($value ?
false : 'KEY is required!') with an explicit check such as if ($value === null
|| $value === '') return 'KEY is required!'; return false; so that 0, "0", and
false are accepted as present. Ensure you keep the function signature and name
(checkRequired) intact.
… instance Last static-state slot on Db. The flag itself is tiny (one bool) but it's the only piece preventing Db from being honestly described as 'fully delegating'. * ConnectionManager gains setDebug()/isDebug() as instance methods. The flag lives on the container-resolved singleton, so a test that swaps ConnectionManager via Container::instance(...) gets a fresh debug state too — previously a leaky class-wide static. * Db::debugMode() delegates to the manager; Db::isDebug() reads from it (per-Query $this->debug still wins as the local override). * Db::exec() reads the flag via ConnectionManager instead of a self:: reference, so all paths agree on one source of truth. Suite: 125 tests, all green.
Pull two clear improvements out of the Query audit. The deeper bug
(broken paren() variable-shadowing + Parts construction mismatch on
the grouped-where code path) needs its own investigation; this commit
only takes the safe wins.
* Traits/QueryWH.php
- Add missing 'use Silver\Database\Parts\Paren;' — every Paren
reference was previously unresolvable.
- Rename callee in havingParen() / whereParen() from $this->parent()
(which doesn't exist) to $this->paren() (the method that does).
Pure typo fix; the grouped-where path still fails further down
in Parts construction, but at least the first hop is honest now.
- paren() rewritten to stop shadowing its first parameter
($cond = $this->$cond clobbered the slot name with the slot
value, making every subsequent $this->$cond = … write a no-op).
The slot name now stays in $condName.
- Drop unused 'use Silver\Database\Parts\Raw;'.
- Fix '\Query' instanceof check → proper 'Silver\Database\Query'
via 'use Silver\Database\Query;'.
* Query/Delete.php
- Drop QueryGroupBy trait + the two // FIXME-marked compile calls
(compileGroupBy, compileHaving). Standard DELETE has no GROUP BY
or HAVING clauses on any driver; emitting them would have produced
invalid SQL.
Suite: 125 / 276 / 1 skipped.
Triggered by /testerror — the old 500 page was honest but plain: bare
stack frames (vendor/Composer noise at the top), no code context per
frame, no syntax-aware highlighting on the error line, no environment
panel, light-mode only, no editor jumping.
The new view is still inline-CSS-only (must render when the asset build
is dead) but a lot more useful:
ErrorHandler (Core/ErrorHandler.php)
* codeAroundLines(file, line, pad) — returns [{n, text, hit}] rows
so the view can lay out a proper line-numbered table with the
hit line highlighted (was a flat string before).
* frameKind(file) — classifies each stack frame as
app | framework | vendor | internal by source-directory prefix.
* ideLink(file, line) — emits phpstorm:// or vscode:// (picked by
$IDE_PROTOCOL env, default phpstorm).
* normalizeFrames() now returns rich frame metadata per frame:
where, file, rel, line, kind, ide, and a 6-line source snippet.
* previousChain() — flattens $e->getPrevious() into a display list
so wrapped exceptions show their original cause.
* render() passes the new payload: rel_file, source, source_ide,
previous, env (php / name / debug / opcache / mem_peak).
500.ghost.php (App/Views/errors/500.ghost.php)
* Full rewrite. Dark-mode via prefers-color-scheme using CSS
custom properties throughout; no JS required.
* Header band with pulsing accent dot, exception class, message,
IDE-linked file:line, and right-side meta pills
(env / php / opcache / memory peak).
* 'Caused by' section walks getPrevious() and renders each wrapper
stage with class/message/location.
* Source viewer: numbered <ol>, the hit line gets a red rail +
tinted background. Header has an 'open in editor →' deep link.
* Stack trace: each frame is a <details>/summary. Kind chip
(app/framework/vendor/internal) on the left, where in the middle,
file:line on the right (also IDE-linked). Expanding a frame
reveals its 6-line source snippet inline. Vendor + internal
frames hidden by default behind a CSS-only toggle so the user
code stays at eye level.
* Two-column grid at the bottom: Request (method/URI/route/client/
query/input) + Environment (env/debug/PHP/opcache/memory).
* Production branch unchanged shape, just dark-mode aware.
ErrorPageTest.php
* The self-contained assertion was checking for the bare strings
'viteCss' and '/build/' — now false-positives because debug frame
snippets can legitimately include those tokens from source. Switch
to specific regex for the *actual* leak pattern:
<link href=".../build/..." / <script src=".../build/..."
plus an explicit check that the {{ viteCss() }} Ghost directive
was compiled away.
Suite: 125 / 277 / 1 skipped, all green.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/Http/ViewRegistry.php`:
- Around line 77-85: The anonymous callback passed to array_any in
ViewRegistry:: (the foreach over $this->composers) only declares one parameter
but array_any invokes the callback with (value, key); update the callback
signature to accept two parameters (e.g., static fn(string $pattern, $_): bool
=> ...) so it matches array_any's (value, key) invocation and avoid an
argument-count error, keeping the matching logic the same and leaving the rest
of the foreach (including the array_merge of composer['callback']) unchanged.
🪄 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: 3d5301f2-df23-4ff8-bd55-97c8170fab17
📒 Files selected for processing (18)
app/Controllers/TestErrorController.phpapp/Resources/js/Pages/TestError.vueapp/Routes/Web.phppackages/silverengine/core/src/Core/App.phppackages/silverengine/core/src/Core/Config.phppackages/silverengine/core/src/Core/ErrorHandler.phppackages/silverengine/core/src/Database/ConnectionManager.phppackages/silverengine/core/src/Database/Db.phppackages/silverengine/core/src/Database/Query/Delete.phppackages/silverengine/core/src/Database/Traits/QueryWH.phppackages/silverengine/core/src/Http/View.phppackages/silverengine/core/src/Http/ViewRegistry.phppackages/silverengine/core/src/Support/Facades/FakeFactory.phppackages/silverengine/core/src/Support/Facades/Log.phppackages/silverengine/core/src/Support/Facades/Request.phppackages/silverengine/core/src/Support/Facades/Response.phppackages/silverengine/core/src/Support/Facades/Validator.phptests/Unit/Framework/Http/ValidatorTest.php
✅ Files skipped from review due to trivial changes (2)
- packages/silverengine/core/src/Support/Facades/FakeFactory.php
- packages/silverengine/core/src/Core/Config.php
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/Unit/Framework/Http/ValidatorTest.php
Comment on lines
+77
to
+85
| foreach ($this->composers as $composer) { | ||
| $matches = array_any( | ||
| $composer['patterns'], | ||
| static fn (string $pattern): bool => $pattern === $name || fnmatch($pattern, $name), | ||
| ); | ||
| if ($matches) { | ||
| $data = array_merge($data, (array) ($composer['callback'])($name)); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
PHP 8.4 array_any function
💡 Result:
The array_any function is a standard library function introduced in PHP 8.4.0 that determines if at least one element in an array satisfies a given callback condition [1][2][3]. Key details include: Function Signature bool array_any(array $array, callable $callback) Behavior - The function iterates through the provided array and applies the callback to each element [1][3]. - The callback receives two arguments: the element value and the element key [1][2][3]. - It returns true immediately if the callback returns true for any element, at which point iteration stops [1][3]. - It returns false if the callback does not return true for any element [1][3]. - It returns false if the array is empty [3]. Usage Example $array = ['dog', 'cat', 'elephant']; // Checks if any animal name has more than 5 characters $result = array_any($array, fn(string $value) => strlen($value) > 5); var_dump($result); // Outputs: bool(true) Comparison with Other Functions - Unlike array_filter, which returns an array of elements that match a condition, array_any returns a boolean indicating only the existence of a match [2]. - The callback for array_any strictly requires two arguments (value and key), which can cause errors if used with existing built-in functions that expect a different number of arguments, such as is_integer [2].
Citations:
- 1: https://mirror.math.princeton.edu/pub/php/manual/en/function.array-any.php
- 2: https://www.php.net/manual/en/function.array-any.php
- 3: https://php.watch/versions/8.4/array_find-array_find_key-array_any-array_all
Fix callback signature for array_any()
array_any() is available in PHP 8.4+ (so PHP 8.5 is fine), but it calls the callback as (value, key). In packages/silverengine/core/src/Http/ViewRegistry.php (lines 77-85), the callback is static fn (string $pattern): bool => ... with only one parameter, which can cause an argument-count error at runtime—adjust it to accept both args (e.g., static fn (string $pattern, $_): bool => ...).
🤖 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/Http/ViewRegistry.php` around lines 77 - 85,
The anonymous callback passed to array_any in ViewRegistry:: (the foreach over
$this->composers) only declares one parameter but array_any invokes the callback
with (value, key); update the callback signature to accept two parameters (e.g.,
static fn(string $pattern, $_): bool => ...) so it matches array_any's (value,
key) invocation and avoid an argument-count error, keeping the matching logic
the same and leaving the rest of the foreach (including the array_merge of
composer['callback']) unchanged.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/Unit/Framework/Core/ErrorPageTest.php`:
- Around line 58-67: The two assertions in ErrorPageTest.php using
assertDoesNotMatchRegularExpression (the one checking
'#<link[^>]+href="[^"]*/build/#i' and the one checking
'#<script[^>]+src="[^"]*/build/#i') only match double-quoted attributes; update
both regexes to accept either single or double quotes around href/src (e.g.
match quote char and use a complementary class for the value) so they also catch
patterns like href='.../build/...' and src='.../build/...'.
🪄 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: d59210da-9d8c-429d-949b-5792e62e0a5a
📒 Files selected for processing (3)
packages/silverengine/core/src/App/Views/errors/500.ghost.phppackages/silverengine/core/src/Core/ErrorHandler.phptests/Unit/Framework/Core/ErrorPageTest.php
Comment on lines
+58
to
+67
| $this->assertDoesNotMatchRegularExpression( | ||
| '#<link[^>]+href="[^"]*/build/#i', | ||
| $html, | ||
| 'View must not <link> any Vite-built CSS.', | ||
| ); | ||
| $this->assertDoesNotMatchRegularExpression( | ||
| '#<script[^>]+src="[^"]*/build/#i', | ||
| $html, | ||
| 'View must not <script src> any Vite-built JS.', | ||
| ); |
There was a problem hiding this comment.
Broaden asset-tag regex to catch single-quoted attributes.
These assertions currently miss <link ... href='.../build/...'> and <script ... src='.../build/...'>, so a leak could slip through.
🔧 Suggested patch
$this->assertDoesNotMatchRegularExpression(
- '#<link[^>]+href="[^"]*/build/#i',
+ '#<link[^>]+href=["\'][^"\']*/build/#i',
$html,
'View must not <link> any Vite-built CSS.',
);
$this->assertDoesNotMatchRegularExpression(
- '#<script[^>]+src="[^"]*/build/#i',
+ '#<script[^>]+src=["\'][^"\']*/build/#i',
$html,
'View must not <script src> any Vite-built JS.',
);📝 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
| $this->assertDoesNotMatchRegularExpression( | |
| '#<link[^>]+href="[^"]*/build/#i', | |
| $html, | |
| 'View must not <link> any Vite-built CSS.', | |
| ); | |
| $this->assertDoesNotMatchRegularExpression( | |
| '#<script[^>]+src="[^"]*/build/#i', | |
| $html, | |
| 'View must not <script src> any Vite-built JS.', | |
| ); | |
| $this->assertDoesNotMatchRegularExpression( | |
| '#<link[^>]+href=["\'][^"\']*/build/#i', | |
| $html, | |
| 'View must not <link> any Vite-built CSS.', | |
| ); | |
| $this->assertDoesNotMatchRegularExpression( | |
| '#<script[^>]+src=["\'][^"\']*/build/#i', | |
| $html, | |
| 'View must not <script src> any Vite-built JS.', | |
| ); |
🤖 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/ErrorPageTest.php` around lines 58 - 67, The two
assertions in ErrorPageTest.php using assertDoesNotMatchRegularExpression (the
one checking '#<link[^>]+href="[^"]*/build/#i' and the one checking
'#<script[^>]+src="[^"]*/build/#i') only match double-quoted attributes; update
both regexes to accept either single or double quotes around href/src (e.g.
match quote char and use a complementary class for the value) so they also catch
patterns like href='.../build/...' and src='.../build/...'.
…ecordings Builds on e37c2d1 to make the error page genuinely useful instead of just informative. Page grew 35K → 117K, mostly content; load is still sub-10ms because everything is server-rendered, inline-CSS, no fonts. * Ghost: {!! $expr !!} added as Laravel-style raw-output syntax, alongside the existing {{{ }}} (which stays for back-compat). Six new GhostVars tests lock both shapes + the escaped {{ }} default + the @{{ }} skip marker. * PHP syntax highlighting on every source snippet — driven by native token_get_all, tokens wrapped in <span class="t-kw"> etc. Token colours inline in the view CSS for both light + prefers-color-scheme dark. Multi-line tokens (heredocs, docblocks) span correctly because the whole file is tokenised once then sliced by line. * Suggestions panel — pattern-matched hints for common error classes (ParseError → 'check unbalanced brackets', class-not-found → suggest composer dump-autoload, PDOException → point at heartbeat + .env, NotFoundException → suggest optimize:clear, SQLite path mismatch for the literal 'unable to open database file' string, etc.). Empty when nothing matches — section just doesn't render. * 'Copy AI prompt' button — primary purple-tinted button that copies a markdown-formatted prompt to the clipboard: error class/message, source snippet with the hit line arrow-pointed, app+framework stack frames (vendor noise stripped for signal), env block, and a one-line ask. Paste-ready for ChatGPT / Claude. - Plus secondary 'copy error' + 'copy location' + 'copy trace'. - JS uses navigator.clipboard with execCommand fallback for legacy browsers / non-HTTPS hosts. - All buttons gracefully no-op without JS — page still works. * 'View full file' disclosure under the source snippet — a <details> that expands to show the entire file syntax-highlighted (capped at 1000 lines for safety). Each line is anchor-targetable as #LN so you can deep-link straight to a specific line. * Recent recordings panel — last 5 requests from storage/debug/recordings/ shown as a compact grid (timestamp, method, path, status colour-coded by class, duration). Lets you see the request just before the failure without bouncing to /debug. * Visual polish: action-row grouping for copy buttons, primary purple-gradient button for the AI prompt, softer dark colours, cleaner section spacing, ico spans for emoji-style icons. * ErrorPageTest assertion tightened to a regex over the real leak pattern (link href=/build/ or script src=/build/) instead of a string match that false-positived on debug source snippets. Suite: 131 tests / 283 assertions / 1 pre-existing skip.
UI/UX pass over the advanced error page. Same data, more navigable.
* Sticky TOC at the top — Suggestions / Source / Stack / Request /
Recent jump links plus a small file-pill on the right showing the
hit location. Backdrop-blurred translucent background so the
underlying source stays partially visible while scrolling.
* Every section gets an explicit id (hints, source, stack, request,
recent) + scroll-margin-top so anchor jumps land below the sticky
TOC instead of underneath it. html { scroll-behavior: smooth } so
the jumps animate instead of teleporting.
* Recent recordings rows are now full-width clickable — wrap the row
contents in an <a href="/debug?tab=recordings&id=…"> with
display:contents so the layout stays intact. Hover state on rows.
* Section spacing bumped (2.25rem → 2.5rem), h2 .meta given inline-
flex + explicit gap so the copy-button next to 'Stack trace' sits
on the baseline cleanly instead of with the ad-hoc margin-left.
Same suite (131 / 283 / 1 skipped, all green); page is 120 KB now.
padding-bottom 1.5 → 2rem and margin-bottom 2 → 3.5rem so the first section (Suggestions or Source) sits clearly below the dividing line instead of crowding the header band.
Changed the TOC's margin from `-2rem -1.5rem 0` to `-2rem -1.5rem 2.5rem` so the header (eyebrow + class title + msg) sits clearly below the TOC bar instead of butting up against its bottom border.
Reduce header padding-bottom 2 → 1.5rem and margin-bottom 3.5 → 1.5rem. With section margin-top of 2.5rem collapsing against it, the gap above 'Suggestions' was stacking to ~3.5rem and looked airy. Now lands around 2.5rem which matches the rhythm between later sections.
Nine improvements from the self-audit of the new 500 page. All ship
behind no flag; backward-compatible.
REAL FIXES
* Secret redaction in the Request panel. requestContext() walks $_GET +
Request::all() and replaces values keyed by /password|token|secret|
api_key|authorization|bearer|session|cookie|csrf|otp|pin/i with
'[REDACTED]'. Login forms (and similar) no longer leak credentials
into the rendered HTML.
* First app-kind stack frame auto-opens. normalizeFrames() now sets
is_first_app on the topmost frame whose source lives under app/ —
the view honours it as the <details open> attribute. 90% of the
time it's exactly the frame you want to see first.
* Recording rows no longer link to a fake ?id= query param the
controller doesn't read. URL is now /debug?tab=recordings#{id} so
the user lands on the right tab; falling back to a fragment is a
no-op if the controller doesn't auto-scroll there.
* Single horizontal scrollbar for the source viewer. Lines used to
scroll independently per cell; .src-lines now overflow-x:auto and
the line-number cells use position:sticky;left:0 so the gutter
stays put while the code scrolls under it (IDE-style).
POLISH
* Mobile / narrow rules (<720px): TOC drops the file-pill, header
stacks vertically, action-row buttons compact, stack-frame
summaries wrap the file:line under the symbol instead of jamming
it on the same row.
* Caused-by chain gets a thin red guide line down the left edge,
with a small dot + connector for each previous exception — makes
the chain visually obvious instead of a flat list of cards.
* AI prompt now also includes the Request line (METHOD URI / route)
and a pointer to /heartbeat so the LLM has architectural context,
not just the immediate failure.
A11Y
* <nav class="toc"> gets aria-label="Error page sections".
* Line-number spans get aria-hidden="true" — purely decorative,
the line content is the meaning.
SCROLL-SPY
* TOC link of the section currently in view gets .active styling
(var(--fg) text + accent-coloured underline). Driven by ~15 lines
of IntersectionObserver; degrades silently if JS is off.
INTENTIONALLY SKIPPED (from the audit's 'skip' tier)
* Search/filter in stack trace, CSS code-splitting, git-blame
integration. All would add weight or break the
'page renders when assets are broken' guarantee.
Suite: 131 / 283 / 1 skipped, all green.
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.
Summary by CodeRabbit
New Features
Bug Fixes
Removals
Documentation