Skip to content

Commit d23d923

Browse files
serpentbladeclaude
andcommitted
fix(routing): close three byte-identity gaps in the URL generator (review)
A deep parity review (+ two adversarial audits) of the route() fast path against vanilla's full RouteUrlGenerator::to() found three issues. All proven against a vanilla oracle before/after, and regression-tested. 1. Empty-string param value (BYTE-IDENTITY): route('u', ['user' => '']) returned /api/users; vanilla treats '' as a *missing* named param (leaves {user} literal) and throws UrlGenerationException. Now defers on $value === ''. 2. Duplicate parameter names (BYTE-IDENTITY): {a}/{a} positional returned /api/5/6; vanilla fills the first {a} and throws on the second. greaseCompileEntry now returns false for routes with duplicate param names (never indexed). 3. forceRootUrl()/useOrigin() with a path, relative URL (BYTE-IDENTITY): the relative shortcut never consulted formatRoot(), dropping a forced root path ('/app/...' -> '/...'). The relative branch now derives from the full absolute URI and applies vanilla's exact root+base-path strip, so forced-root paths and subdirectory base paths are preserved (and the prior subdirectory defer is gone — broader coverage, all byte-identical). 4. Prewarm seed wiped under route:cache (PERF, not correctness): the framework's booted-phase cached-routes load rebinds 'routes' -> setRoutes() -> flush, which wiped a boot()-time URL-index seed. Seeding now runs on an app->booted() callback (registered after RouteServiceProvider's), so it lands after the route load and survives into requests. (Lazy fallback was already byte-identical; this restores the intended FPM/Octane prewarm.) Provider rebind, signed URLs, Octane persistence, command enumeration, and L10-13 container semantics all audited clean (no breaking changes). Tests: +3 regressions (empty/dup throw-like-vanilla, forced-root parity, seed-survives-rebind). 638 tests / 1810 assertions green; Pint + docs build clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 1ff30b2 commit d23d923

6 files changed

Lines changed: 147 additions & 20 deletions

File tree

CHANGELOG.md

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -17,20 +17,25 @@ All notable changes to `grease` are documented here. The format is based on
1717
**−93% per call**, and because the assembly cost is fixed while the model tiers shrink everything
1818
else, **~−26% of an already-greased API-Resource response** (−84.7% full-stack on a 500-row ×
1919
~5-link payload). Byte-identical to vanilla or it defers — domain routes, optional `{param?}` /
20-
scoped `{param:field}` bindings, route-level defaults, extra params (query string), signed /
21-
absolute-in-a-subdirectory URLs, and any `URL::defaults()` / `formatHostUsing()` /
22-
`formatPathUsing()` customization all fall through to `parent::`. Absolute (the `route()` default)
23-
and relative are both accelerated. Verified by `UrlGeneratorParityTest` (oracle = vanilla across
24-
absolute/relative, every defer case, special-char encoding, the missing-param exception, secure
25-
scheme, subdirectory apps, and prewarm-seed parity).
20+
scoped `{param:field}` bindings, route-level defaults, duplicate parameter names, extra params
21+
(query string), arity mismatches, empty-string / non-scalar values, signed URLs, and any
22+
`URL::defaults()` / `formatHostUsing()` / `formatPathUsing()` customization all fall through to
23+
`parent::`. Absolute (the `route()` default) and relative are both accelerated; the relative path
24+
derives from the full absolute URI and strips exactly what vanilla strips, so a `forceRootUrl()` /
25+
`useOrigin()` root path and a subdirectory base path are preserved. Verified by
26+
`UrlGeneratorParityTest` (oracle = vanilla across absolute/relative, every defer case,
27+
special-char encoding, the missing-param exception, secure scheme, subdirectory + forced-root
28+
apps, and prewarm-seed parity).
2629
- **`Grease\Routing\GreaseRoutingServiceProvider` now swaps the `url` singleton** for the greased
2730
generator (no `bootstrap/app.php` edit — unlike the kernel-injected router, `url` is resolved
2831
lazily, so a provider rebind is in time; the framework's session/key resolvers and `routes`
2932
rebinding survive it, so signed URLs and route-cache rebinding are unchanged).
3033
- **`grease:route-cache` now also writes an opcache-interned URL shape index** (`name =>
3134
[segments, params]`) alongside the middleware index, via the same `greaseCompileEntry()` the lazy
3235
path uses, so a pre-seeded entry is byte-identical to one compiled on first `route()`. The
33-
provider loads it under the same freshness check. The prewarm's payoff is FPM cold-build
36+
provider loads it under the same freshness check, on a `booted` callback — after the framework's
37+
cached-routes load (which rebinds `routes``setRoutes()` → flushes the index), so the seed
38+
survives into requests rather than being wiped. The prewarm's payoff is FPM cold-build
3439
elimination (sub-ms, scales with route count) and Octane build-once-ever — not a per-response
3540
render win (the lazy index self-warms on first call); the −26% comes from the assembly collapse,
3641
which lands cold or warm.

docs/guide/url-generator.md

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,14 +47,18 @@ result is provably the exact string vanilla would build. Anything else falls str
4747
- a route with a **domain** (it assembles a host),
4848
- an **optional** `{param?}` or **scoped** `{param:field}` binding (different replacement
4949
semantics),
50-
- a route carrying its own **`$defaults`**.
50+
- a route carrying its own **`$defaults`**,
51+
- a **duplicate parameter name** (`{a}/{a}`) — malformed; vanilla fills the first and throws on
52+
the second.
5153

5254
**Not fast-pathable** (decided per call):
5355

5456
- **extra parameters** — they become a query string,
5557
- an **arity mismatch** — too few is vanilla's `UrlGenerationException`, which must still throw,
5658
- a **non-scalar** value (after `UrlRoutable::getRouteKey()`) — `null`/`bool`/`float`/array have
5759
distinct vanilla semantics,
60+
- an **empty-string** value — vanilla treats it as a *missing* parameter (leaves `{name}` literal)
61+
and throws,
5862
- a value that would inject a literal `{…}` — vanilla treats that as a missing parameter and throws,
5963
- a **subdirectory app** for a *relative* URL (non-empty `Request::getBaseUrl()`),
6064
- any **`URL::defaults()`**, **`formatHostUsing()`**, or **`formatPathUsing()`** customization in

src/Routing/GreaseRoutingServiceProvider.php

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,16 @@ public function boot(): void
5656
}
5757

5858
$this->loadMiddlewareIndex();
59-
$this->loadUrlIndex();
59+
60+
// Seed the URL index on a `booted` callback, NOT here. A cached-routes load
61+
// (`RouteServiceProvider`'s own booted callback) re-binds `routes`, firing the
62+
// framework's `rebinding('routes')` → `UrlGenerator::setRoutes()`, which flushes the
63+
// index. That runs in the booted phase, strictly after every provider's `boot()` — so a
64+
// seed here would be wiped. Registered now (after `RouteServiceProvider::boot()` queued
65+
// its callback), this fires after the route load, so the seed survives into requests.
66+
// The middleware index has no such hazard: `setCompiledRoutes()` swaps the route
67+
// collection without touching the router's resolved-middleware cache.
68+
$this->app->booted(fn () => $this->loadUrlIndex());
6069
}
6170

6271
/**

src/Routing/UrlGenerator.php

Lines changed: 22 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -133,6 +133,12 @@ public static function greaseCompileEntry($route)
133133
$segments = preg_split('/\{[^}]+\}/', $uri);
134134
preg_match_all('/\{([^}]+)\}/', $uri, $matches);
135135

136+
// A duplicate parameter name is malformed: vanilla fills the first `{x}` and leaves the
137+
// second in place, throwing. Never index it — let vanilla own that behaviour.
138+
if (count($matches[1]) !== count(array_unique($matches[1]))) {
139+
return false;
140+
}
141+
136142
return ['segments' => $segments, 'params' => $matches[1]];
137143
}
138144

@@ -175,8 +181,10 @@ protected function greaseFastToRoute($route, array $entry, $parameters, bool $ab
175181
$value = $value->getRouteKey();
176182
}
177183

178-
if (! is_string($value) && ! is_int($value)) {
179-
return null; // null/bool/float/array have distinct vanilla semantics
184+
if ((! is_string($value) && ! is_int($value)) || $value === '') {
185+
// null/bool/float/array have distinct vanilla semantics; an empty string is a
186+
// *missing* named parameter to vanilla (it leaves `{name}` in place and throws).
187+
return null;
180188
}
181189

182190
$path .= $value.$segments[$i + 1];
@@ -189,19 +197,23 @@ protected function greaseFastToRoute($route, array $entry, $parameters, bool $ab
189197

190198
$dontEncode = $this->greaseDontEncode ??= $this->routeUrl()->dontEncode;
191199

192-
if ($absolute) {
193-
$scheme = $route->httpOnly() ? 'http://' : ($route->httpsOnly() ? 'https://' : $this->formatScheme());
200+
$scheme = $route->httpOnly() ? 'http://' : ($route->httpsOnly() ? 'https://' : $this->formatScheme());
201+
$uri = strtr(rawurlencode(trim($this->formatRoot($scheme).'/'.trim($path, '/'), '/')), $dontEncode);
194202

195-
return strtr(rawurlencode(trim($this->formatRoot($scheme).'/'.trim($path, '/'), '/')), $dontEncode);
203+
if ($absolute) {
204+
return $uri;
196205
}
197206

198-
// Relative: vanilla strips the root and the request base path. The simple form below is
199-
// byte-identical only at the document root; a subdirectory app defers.
200-
if ($this->request->getBaseUrl() !== '') {
201-
return null;
207+
// Relative: build the absolute URI (so a forced root path from forceRootUrl()/useOrigin()
208+
// is included), then strip scheme+host and the request base path — a verbatim copy of
209+
// RouteUrlGenerator::to()'s relative tail, byte-identical for every root shape.
210+
$uri = preg_replace('#^(//|[^/?])+#', '', $uri);
211+
212+
if ($base = $this->request->getBaseUrl()) {
213+
$uri = preg_replace('#^'.$base.'#i', '', $uri);
202214
}
203215

204-
return '/'.ltrim(strtr(rawurlencode($path), $dontEncode), '/');
216+
return '/'.ltrim($uri, '/');
205217
}
206218

207219
/**

tests/Routing/GreaseRoutingServiceProviderTest.php

Lines changed: 41 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,9 @@
88
use Illuminate\Contracts\Console\Kernel;
99
use Illuminate\Foundation\Application;
1010
use Illuminate\Foundation\Configuration\ApplicationBuilder;
11+
use Illuminate\Http\Request;
1112
use Orchestra\Testbench\TestCase as Orchestra;
13+
use ReflectionMethod;
1214
use ReflectionProperty;
1315

1416
/**
@@ -92,7 +94,7 @@ public function test_url_generator_is_greased_and_still_signs(): void
9294
$signed = $url->signedRoute('things.show', ['id' => 5]);
9395
$this->assertStringContainsString('signature=', $signed);
9496

95-
$request = \Illuminate\Http\Request::create($signed);
97+
$request = Request::create($signed);
9698
$this->assertTrue($url->hasValidSignature($request));
9799
}
98100

@@ -124,6 +126,44 @@ public function test_url_index_round_trips_and_seeds_the_generator(): void
124126
@unlink($routesCache);
125127
}
126128

129+
/**
130+
* The hazard the booted-callback seeding guards against: a cached-routes load re-binds
131+
* `routes`, firing the framework's `rebinding('routes')` → `UrlGenerator::setRoutes()`, which
132+
* flushes the URL index. Seeding in `boot()` would be wiped by that later booted-phase load;
133+
* the provider instead seeds on a `booted` callback (after the route load), so loading the
134+
* index AFTER a `setRoutes()` flush correctly repopulates it. Regression for a prewarm bug
135+
* caught in review.
136+
*/
137+
public function test_url_index_seed_survives_a_routes_rebind(): void
138+
{
139+
$url = $this->app->make('url');
140+
141+
$path = GreaseRoutingServiceProvider::urlIndexPath($this->app);
142+
$routesCache = $this->app->getCachedRoutesPath();
143+
@mkdir(dirname($path), 0777, true);
144+
file_put_contents($routesCache, '<?php return [];'.PHP_EOL);
145+
touch($routesCache, time() - 10);
146+
$entries = ['things.show' => ['segments' => ['things/', ''], 'params' => ['id']]];
147+
file_put_contents($path, '<?php return '.var_export($entries, true).';'.PHP_EOL);
148+
touch($path, time());
149+
150+
$indexProp = new ReflectionProperty(GreasedUrlGenerator::class, 'greaseRouteUrlIndex');
151+
152+
// A boot()-time seed would be wiped here: the compiled-routes load rebinds 'routes'.
153+
$url->useGreaseRouteUrlIndex($entries);
154+
$url->setRoutes($this->app['router']->getRoutes());
155+
$this->assertSame([], $indexProp->getValue($url), 'sanity: a routes rebind flushes the index');
156+
157+
// The provider seeds on app->booted(), which fires AFTER that load — replay its body.
158+
(new ReflectionMethod(GreaseRoutingServiceProvider::class, 'loadUrlIndex'))
159+
->invoke(new GreaseRoutingServiceProvider($this->app));
160+
161+
$this->assertArrayHasKey('things.show', $indexProp->getValue($url), 'seed must land after the route load');
162+
163+
@unlink($path);
164+
@unlink($routesCache);
165+
}
166+
127167
public function test_index_freshness_guard(): void
128168
{
129169
$dir = sys_get_temp_dir().'/grease_rt_'.getmypid();

tests/Routing/UrlGeneratorParityTest.php

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ class UrlGeneratorParityTest extends TestCase
3131
'root' => '/', // root
3232
'posts.optional' => 'api/posts/{post}/{slug?}', // optional → defer
3333
'posts.scoped' => 'api/posts/{post:slug}', // scoped binding → defer
34+
'posts.dup' => 'api/dup/{a}/{a}', // duplicate param name → defer (vanilla throws)
3435
'admin.dash' => 'dashboard', // for a domain variant below
3536
];
3637

@@ -136,6 +137,39 @@ public function test_missing_required_parameter_throws_like_vanilla(): void
136137
$greased->route('posts.show', [], true);
137138
}
138139

140+
/**
141+
* Values vanilla treats as a *missing* parameter (an empty string, which it leaves as a
142+
* literal `{name}`) and malformed duplicate-name routes must throw `UrlGenerationException`
143+
* just like vanilla — the fast path must NOT silently build a URL. Regression for two
144+
* byte-identity divergences caught in review.
145+
*/
146+
public function test_empty_string_and_duplicate_param_throw_like_vanilla(): void
147+
{
148+
[$vanilla, $greased] = $this->generators();
149+
150+
$cases = [
151+
'empty named value' => ['posts.show', ['post' => '']],
152+
'empty mid-segment' => ['posts.comments', ['post' => '', 'comment' => 5]],
153+
'duplicate param positional' => ['posts.dup', [5, 6]],
154+
];
155+
156+
foreach ($cases as $label => [$name, $params]) {
157+
$this->assertTrue($this->throws($vanilla, $name, $params), "sanity: vanilla throws for $label");
158+
$this->assertTrue($this->throws($greased, $name, $params), "greased must throw like vanilla for $label");
159+
}
160+
}
161+
162+
private function throws(VanillaUrlGenerator $url, string $name, $params): bool
163+
{
164+
try {
165+
$url->route($name, $params);
166+
167+
return false;
168+
} catch (UrlGenerationException) {
169+
return true;
170+
}
171+
}
172+
139173
public function test_prewarmed_index_matches_lazy(): void
140174
{
141175
[$vanilla, $greased] = $this->generators();
@@ -214,4 +248,27 @@ public function test_subdirectory_app_relative_matches(): void
214248
'subdir relative'
215249
);
216250
}
251+
252+
/**
253+
* forceRootUrl()/useOrigin() inject a root that may carry a *path* component
254+
* (`https://example.com/app`) which a relative URL must keep — `/app/...`. Regression for a
255+
* byte-identity divergence caught in review (the relative fast path now derives from the full
256+
* absolute URI and strips exactly what vanilla strips, so the forced root path survives).
257+
*/
258+
public function test_forced_root_url_with_path_matches(): void
259+
{
260+
foreach (['https://example.com/app', 'https://cdn.example.com'] as $root) {
261+
[$vanilla, $greased] = $this->generators();
262+
$vanilla->forceRootUrl($root);
263+
$greased->forceRootUrl($root);
264+
265+
foreach ([true, false] as $absolute) {
266+
$this->assertSame(
267+
$vanilla->route('posts.show', ['post' => 5], $absolute),
268+
$greased->route('posts.show', ['post' => 5], $absolute),
269+
"$root absolute=".var_export($absolute, true)
270+
);
271+
}
272+
}
273+
}
217274
}

0 commit comments

Comments
 (0)