feat: migrate to php8.5 Uri\Rfc3986\Uri

[rrr63]

No description provided.

[techmahedy]

@rrr63 thanks for this PR

[techmahedy]

Thanks for the PR. Moving off parse_url() to Uri\Rfc3986\Uri is a reasonable
direction, since the repo already requires php: ^8.5. The full suite passes on your branch (3554 tests, 0 failures; 672 skipped for unrelated reasons).

There is one regression that will break real traffic. It is easy to fix, but the tests
don't catch it, because every new test uses well-formed URLs. Details, reproductions and
suggested fixes are below.

---

1. Summary of the problem

parse_url() is lenient. It accepts almost any string and returns a best-effort result.

Uri\Rfc3986\Uri::parse() is strict RFC 3986. For any string that is not a valid URI
reference it returns null. Several of the changed call sites turn that null into a
silent wrong value ('/', an unchanged URL, or an un-merged query) instead of handling it.

Characters that are technically invalid in RFC 3986 but sent by browsers, curl and HTTP
clients every day include:

Character / pattern	Example	Why it appears in practice
[ ] in the query	?filter[status]=active, ?ids[]=1&ids[]=2	PHP-style array params. Browsers do not percent-encode them.
Space	/search?q=hello world	Hand-typed URLs, some clients and proxies
{ } | ^ `	?x={1}, /a|b	JSON-in-query, legacy links
Raw UTF-8	/café	Non-ASCII paths sent unencoded by some clients

Verified with PHP 8.5.10:

input                 Uri::parse   parse_url path
/users?a[]=1&b=2      NULL         /users
/users?x={1}          NULL         /users
/search?q=a b         NULL         /search
/foo bar              NULL         /foo bar
/café                 NULL         /café
/a|b                  NULL         /a|b

---

Run against the real classes (Request::getPath(), RedirectResponse::ensureScheme(),
Paginator::appendQueryParameters()).

2.1 Request::getPath() (routing path)

REQUEST_URI	Base (fd645bf)	PR (60b0e44)
/users?a[]=1&b=2	/users	/
/users?filter[status]=active	/users	/
/search?q=hello world	/search	/
/café	/café	/
/users?x={1}	/users	/
/users/1?page=2	/users/1	/users/1 (unchanged)

2.2 RedirectResponse::ensureScheme() with $secure = true

Input	Base	PR
http://example.com/a?x=1	https://example.com/a?x=1	https://example.com/a?x=1
http://example.com/a?x[]=1	https://example.com/a?x[]=1	http://example.com/a?x[]=1 (still http)
http://example.com/a b	https://example.com/a b	http://example.com/a b (still http)

2.3 Paginator::appendQueryParameters() (adding ['page'=>3,'sort'=>'id'])

Input URL	Base	PR
http://x.test/items?page=2	…/items?page=2&sort=id	same
http://x.test/items?filter[status]=a&page=2	…/items?page=2&sort=id&filter%5Bstatus%5D=a	…/items?filter[status]=a&page=2&page=3&sort=id (duplicate page, no merge, no re-encoding)
http://x.test/items?q=a b&page=2	…/items?page=2&sort=id&q=a+b	…/items?q=a b&page=2&page=3&sort=id (duplicate page, raw space in link)

---

3. Required changes

3.1 [BLOCKER] Request::getPath() — src/Phaseolies/Http/Request.php:633

$uri = Uri::parse($this->server->get("REQUEST_URI", "/"));
return urldecode($uri?->getRawPath() ?? '/');

Problem: null falls back to '/'. A request to /users?filter[status]=active is routed
as /, so the user sees the home page instead of /users, with no error or log. This is
a silent misroute on the hottest code path in the framework.

Why the strict parser is the wrong tool here: REQUEST_URI is a request-target that the web
server has already accepted. It is not a URL we are building or validating. It should be
split, not validated.

Requested fix (preferred, smallest diff): keep parse_url() for REQUEST_URI.

public function getPath(): string
{
    return urldecode(
        parse_url($this->server->get("REQUEST_URI", "/"), PHP_URL_PATH) ?? '/'
    );
}

Note the ?? '/'. In the original code parse_url() can return null (for example a
query-only target) or false (for a seriously malformed target), and urldecode(null) is
deprecated in 8.1+. Adding the coalesce is a small robustness improvement that stays
behaviour-compatible for every input that worked before.

If you want to avoid parse_url entirely, a hand-rolled split must also preserve the
authority-form behaviour (//evil.com/x → /x). A plain strcspn($raw, '?#') would return
//evil.com/x and differ from today's behaviour. Please only go this route with tests that
cover that case.

3.2 [BLOCKER] RouteLauncher::register() — src/Phaseolies/Launchers/RouteLauncher.php:17

$uri = Uri::parse(request()->server->get("REQUEST_URI", "/"));
$path = urldecode($uri?->getRawPath() ?? '/');

Same root cause and same fix as 3.1. Please revert to parse_url(..., PHP_URL_PATH) ?? '/'.

The two sites also compute the same value. If you touch this, consider replacing the
duplicate with request()->getPath() so a fix only has to be made once.

3.3 [BLOCKER] Request::prepareRequestUri() — src/Phaseolies/Http/Request.php:1175

$uriComponents = Uri::parse($requestUri);
if ($uriComponents !== null) { ... }   // else: $requestUri left as the full "http://host/path?x[]=1"

Problem: this branch handles absolute-form request targets (HTTP proxy style,
GET http://host/path?x=1). If the target contains [, ], a space or similar, parse()
returns null and the full http://host/... string is stored as REQUEST_URI. Everything
downstream (routing, getRequestUri(), getUri()) then sees a value that is not a path.

Your test testPrepareRequestUriRetainsInvalidProxyUri currently locks in that behaviour
('not a valid uri' in → same out). Please reconsider whether that is the contract you want.
The old code returned the parsed path and query for any URL parse_url could split.

Requested fix: either restore parse_url() here (preferred, same reasoning as 3.1), or
fall back to parse_url() when Uri::parse() returns null:

$parts = Uri::parse($requestUri);
if ($parts !== null) {
    $requestUri = $parts->getRawPath();
    if ($parts->getRawQuery() !== null) {
        $requestUri .= '?' . $parts->getRawQuery();
    }
} elseif (($p = parse_url($requestUri)) !== false) {
    $requestUri = ($p['path'] ?? '') . (isset($p['query']) ? '?' . $p['query'] : '');
}

Also please update testPrepareRequestUriRetainsInvalidProxyUri so it only asserts for
input that is genuinely unparsable by both parsers, and add a proxy-form case with [] in the query.

3.4 [MAJOR] Paginator::appendQueryParameters() — src/Phaseolies/Utilities/Paginator.php:346-354

if ($parsedUrl === null) {
    $separator = str_contains($url, '?') ? '&' : '?';
    return $url . $separator . http_build_query($queryParams);
}

Problem: the fallback skips the merge, which is the whole point of the method.
With ?filter[status]=a&page=2 (a very common pagination-with-filters URL) you get a
link with two page params (page=2&page=3). Which one wins is server-dependent
(PHP takes the last, other consumers may take the first). Pagination links and
filter state can be wrong. Values are also no longer normalised by http_build_query.

Requested fix: on null, fall back to parse_url() and run the same merge:

protected function appendQueryParameters(?string $url, array $queryParams): string
{
    if ($url === null || $url === '') {
        return '';
    }

    $parsed = Uri::parse($url);

    if ($parsed !== null) {
        $scheme   = $parsed->getRawScheme() !== null ? $parsed->getRawScheme() . '://' : '';
        $host     = $parsed->getRawHost() ?? '';
        $port     = $parsed->getPort() !== null ? ':' . $parsed->getPort() : '';
        $path     = $parsed->getRawPath();
        $rawQuery = $parsed->getRawQuery();
    } else {
        $p = parse_url($url);

        if ($p === false) {
            // Truly unparsable (e.g. "http://[invalid"): keep the existing safe fallback.
            return $url . (str_contains($url, '?') ? '&' : '?') . http_build_query($queryParams);
        }

        $scheme   = isset($p['scheme']) ? $p['scheme'] . '://' : '';
        $host     = $p['host'] ?? '';
        $port     = isset($p['port']) ? ':' . $p['port'] : '';
        $path     = $p['path'] ?? '';
        $rawQuery = $p['query'] ?? null;
    }

    $existing = [];
    if ($rawQuery !== null) {
        parse_str($rawQuery, $existing);
    }

    return $scheme . $host . $port . $path . '?' . http_build_query(array_merge($queryParams, $existing));
}

I ran this against the four cases in section 2.3 and it reproduces the base-commit output
(?page=2&sort=id&filter%5Bstatus%5D=a, ?page=2&sort=id&q=a+b) and keeps your
http://[invalid?page=3&sort=id fallback. Your testAppendQueryParametersFallsBackForInvalidUri
still passes.

Unrelated nit in the same method (optional): the comment says "New ones take precedence"
but array_merge($queryParams, $existingParams) lets existing params win, which is why
page=2 beats page=3 in the table above. That behaviour predates this PR (your own new
test expects filter=old), so it is not blocking. Please just fix the comment or the intent.

3.5 [MAJOR] RedirectResponse::ensureScheme() — src/Phaseolies/Http/Response/RedirectResponse.php:143-148

$parsedUrl = Uri::parse($url);
if ($parsedUrl !== null) {
    $url = $parsedUrl->withScheme($scheme)->toRawString();
}

Problem: for a URL that parse() rejects (for example ?x[]=1), the URL is returned
unchanged, so ->secure() / to($url, ..., secure: true) silently does not force
HTTPS. That is a security-relevant downgrade: the caller asked for https and gets http
with no error. The old code always rewrote the scheme.

Requested fix: on null, rewrite the scheme textually, but leave truly unparsable
input alone so your testToMethodPreservesInvalidAbsoluteUrlWhenForcingScheme still passes:

$parsedUrl = Uri::parse($url);
$scheme    = $secure ? 'https' : 'http';

if ($parsedUrl !== null) {
    $url = $parsedUrl->withScheme($scheme)->toRawString();
} elseif (parse_url($url) !== false) {
    $url = preg_replace('#^[a-z][a-z0-9+.\-]*://#i', $scheme . '://', $url, 1) ?? $url;
}

Checked: http://example.com/a?x[]=1 → https://example.com/a?x[]=1,
http://example.com/a b → https://example.com/a b, and http://[invalid is untouched.

3.6 [MINOR] CacheLauncher::createRedisAdapter() — src/Phaseolies/Launchers/CacheLauncher.php:75-81

I tested the DSNs below and the parsed host, port, password and database match the old
code, so the common cases are fine:

redis://:s3cret@127.0.0.1:6380/2   → 127.0.0.1, 6380, s3cret, db 2
redis://user:p%40ss@host/1         → host, (default port), p%40ss, db 1
redis://[::1]:6379                 → [::1], 6379

Two small things:

1. getHost() returns "" (not null) for a host-less URI such as unix:///var/run/redis.sock,
   so ?? '127.0.0.1' does not kick in and connect('', 6379) is attempted. Old code
   would have used 127.0.0.1. Neither is correct for a unix socket, but the failure mode
   changed. Consider $host = ($parsed?->getHost()) ?: '127.0.0.1';.
2. getPassword() returns the raw, still percent-encoded value (p%40ss), the same as
   parse_url. It is not a regression, but it is a known gotcha: consider rawurldecode() if you
   want DSN passwords with @/: to work.

If Redis DSNs with a leading : or credentials containing reserved characters are supported,
please add a test.

---

4. Tests to add

Every new test in the PR uses well-formed URLs, plus one deliberately broken one
(http://[invalid). The failing class of input (URLs that are almost valid) is
untested. Please add the following (adapt names to your style):

tests/Requests/RequestTest.php

#[DataProvider('lenientRequestTargets')]
public function testGetPathToleratesLenientRequestTargets(string $uri, string $expected): void
{
    $this->request->server->set('REQUEST_URI', $uri);
    $this->assertSame($expected, $this->request->getPath());
}

public static function lenientRequestTargets(): array
{
    return [
        'bracket array query'  => ['/users?a[]=1&b=2',            '/users'],
        'bracket nested query' => ['/users?filter[status]=active', '/users'],
        'raw space in query'   => ['/search?q=hello world',        '/search'],
        'raw utf-8 path'       => ['/café?x=1',                    '/café'],
        'braces in query'      => ['/users?x={1}',                 '/users'],
        'pipe in path'         => ['/a|b',                         '/a|b'],
        'authority-form'       => ['//evil.com/x',                 '/x'],
        'query-only'           => ['?a=1',                         '/'],
    ];
}

Add a proxy-form case to prepareRequestUri: http://proxy.example/users?a[]=1 must yield /users?a[]=1.

tests/PaginatorTest.php

public function testAppendQueryParametersMergesWhenQueryContainsBrackets(): void
{
    $m = new \ReflectionMethod($this->paginator, 'appendQueryParameters');
    $this->assertSame(
        'http://x.test/items?page=2&sort=id&filter%5Bstatus%5D=a',
        $m->invoke($this->paginator, 'http://x.test/items?filter[status]=a&page=2', ['page' => 3, 'sort' => 'id'])
    );
}
// Also: 'http://x.test/items?q=a b&page=2'  → '...?page=2&sort=id&q=a+b'   (no duplicate page)

tests/RedirectResponseTest.php

public function testSecureTrueStillUpgradesUrlsWithBracketsAndSpaces(): void
{
    $this->redirect->to('http://example.com/a?x[]=1', 302, [], true);
    $this->assertSame('https://example.com/a?x[]=1', $this->redirect->headers->get('Location'));
}
// Also: 'http://example.com/a b' → 'https://example.com/a b'

tests/Support/ViteManagerTest.php: no change needed.

Please also add at least one end-to-end style test that boots the router (or calls
RouteLauncher::register()), sends /users?filter[status]=x, and asserts that it resolves
to the /users route rather than /.

---

5. Files reviewed with no changes requested

• src/Phaseolies/Auth/Security/InteractsWithTwoFactorAuth.php:53. It parses the
  config-controlled app.url. Falling back to '' for the issuer is safe. It is fine.
• src/Phaseolies/Support/ViteManager.php:132. It parses the config or hot-file URL, and
  the null and empty-host guard is correct. getPort() returns ?int, so dropping the
  (int) cast is correct.

---

6. Guidance for the rest of this migration

A rule of thumb for deciding which parser each site should use:

Input origin	Use
Untrusted, wire-level request targets (REQUEST_URI, proxy URLs, Referer, user-supplied redirect URLs)	Keep lenient parsing (parse_url), or Uri::parse with an explicit, tested fallback. Never map null to a plausible-looking value like '/'.
Developer-controlled config (app.url, Redis DSN, Vite hot URL)	Uri::parse is fine. Null means misconfiguration, so failing loudly is better than silently defaulting.

The bug pattern to avoid is Uri::parse($untrusted)?->x ?? <valid-looking default>. It turns
"this input is weird" into "this is the home page".

---

7. Checklist for re-review

• 3.1 Request::getPath() no longer returns / for /users?a[]=1
• 3.2 RouteLauncher::register() fixed the same way (or delegates to request()->getPath())
• 3.3 prepareRequestUri() proxy branch handles [] and spaces, and the test is updated
• 3.4 Paginator fallback merges instead of appending, and there is no duplicate page
• 3.5 RedirectResponse::secure still forces https for []/space URLs
• 3.6 Redis host-less fallback (optional but recommended)
• Section 4 tests added and passing (vendor/bin/phpunit)
• Full suite still green

Happy to re-review as soon as these are in. The change is small and mostly a matter of
restoring parse_url() at the three request-target sites and adding a fallback at the other
two.

[techmahedy]

looks good now

[techmahedy]

@rrr63 I think Uri\Rfc3986\Uri is appropriate for application-level URLs and DSNs, but replacing parse_url() in request/REQUEST_URI handling isn't necessary and may introduce behavioral differences.

[rrr63]

@rrr63 I think Uri\Rfc3986\Uri is appropriate for application-level URLs and DSNs, but replacing parse_url() in request/REQUEST_URI handling isn't necessary and may introduce behavioral differences.

Ok i changed it to draft until we test it well

[techmahedy]

@rrr63 I think Uri\Rfc3986\Uri is appropriate for application-level URLs and DSNs, but replacing parse_url() in request/REQUEST_URI handling isn't necessary and may introduce behavioral differences.

Ok i changed it to draft until we test it well

thanks @rrr63