Skip to content

fix: correct old input, validation rules, route lookups and error responses - #402

Merged
techmahedy merged 2 commits into
4.xfrom
fix/framework-bugs
Nov 5, 2026
Merged

techmahedy merged 2 commits into
4.xfrom
fix/framework-bugs

Conversation

@abdulmajidcse

Copy link
Copy Markdown
Member

Pull Request Checklist

Q A
Branch? 4.x
Bug fix? yes
New feature? no
Deprecations? no
Issues -
License MIT

Description

This fixes a set of unrelated bugs in forms, validation, routing and responses.

Forms

  • old() deleted all old input on its first call, so only the first field was restored, and array fields threw a TypeError. Every field can now be read for the rest of the request, and the input is not kept for the next one.

Validation

  • min, max and size counted bytes, so max:3 rejected আমি. They now count characters.
  • date accepted impossible dates such as 2026-02-30.
  • in and not_in compared strictly, so the JSON number 1 did not match "1".
  • alpha, alpha_num, alpha_dash, slug, digits, min_digits, max_digits, phone, uuid, int and numeric accepted a trailing newline.
  • required accepted whitespace-only values.

Routing

  • currentRouteAction() matched the route again, which forgot the dispatched route on closure routes in cached apps. Later name and middleware lookups then returned null.
  • currentRouteName() ignored the method, so POST /posts could report posts.index. Names now record their method, including in the route cache.
  • _method=ANY was accepted as a real method and led to a 404. It is now a 400.

Responses

  • A blank Referer made back() throw and ignored $fallback.
  • Web error pages dropped the exception's headers, such as Retry-After.
  • JsonErrorHandler used any exception code as the HTTP status, including invalid ones such as 2. It now uses only codes from 400 to 599, as WebErrorRenderer does, and otherwise returns 500.
  • A jsonSerialize() override that called $this->toArray() made relations skip their own jsonSerialize() in JSON responses.

Encryption

  • decryptLegacy() could return garbage from the wrong key when it happened to be valid UTF-8. It now prefers text without control characters. The old format has no MAC, so this makes the problem much rarer but cannot rule it out.

Behavior changes

  • old() now returns mixed instead of ?string, so array fields return arrays.
  • phone no longer accepts tabs or newlines inside the number; spaces still work.
  • required rejects whitespace-only values.
  • int and numeric reject values with surrounding whitespace.
  • A request using ANY as its method now gets a 400.

Known limits

  • A direct json_encode($model) in application code can still miss a relation's jsonSerialize() when the parent's override calls toArray(). Framework JSON responses are covered.

Testing

  • Added 50 regression test cases.
  • The full suite passes (5930 tests, no failures, 1121 skipped), and so does PHPStan.
  • Not tested in a full application.

Checklist

  • Tests have been added or updated
  • Documentation has been updated if necessary
  • Code follows the project coding standards
  • All tests pass locally

…ponses

- Restore every old() field and read array input
- Count characters in min, max and size, and reject impossible dates
- Match JSON numbers in in and not_in, and reject trailing newlines
- Reject whitespace-only values in required
- Keep the dispatched route in currentRouteAction()
- Use the dispatched method in currentRouteName()
- Treat _method=ANY as an invalid method
- Use the fallback in back() for a blank Referer
- Send exception headers on web error pages
- Use only HTTP error statuses from exception codes in JSON errors
- Use relations' jsonSerialize() in JSON responses when an override calls toArray()
- Prefer plain text when decrypting legacy ciphertext
@techmahedy techmahedy added the bug Something isn't working label Nov 5, 2026
@techmahedy

Copy link
Copy Markdown
Member

Thanks, most of this checks out. I'm requesting changes because two of the fixes don't fully cover the cases the description claims. I reproduced both against this branch.

Fix before merge

1. currentRouteName() still returns another method's name

The new test names both routes. When the POST route has no name, the fallback still returns the GET route's name, which is the bug this PR says it fixes:

$router->get('/posts', fn() => 'index')->name('posts.index');
$router->post('/posts', fn() => 'store');

// POST /posts
$router->currentRouteName(); // 'posts.index', expected null

Suggested fix: use the fallback only for names with no recorded method, such as a route cache written before this change.

That has a knock-on effect. An attribute route with methods: ['GET', 'POST'] calls addRouteNameToAttributesRouting() once per method, so $namedRouteMethods[$name] keeps only the last one. Once the fallback is restricted, the other methods would lose the name, so this needs to store a list of methods per name, e.g. array<string, string[]>. Please also save that list in the route cache.

2. Streamed JSON responses still skip the relation's jsonSerialize()

The description says framework JSON responses are covered, but StreamedJsonResponse::sendContent() calls json_encode() directly. With this PR's own fixtures:

$user = new OverridingUser(); // jsonSerialize() calls $this->toArray()
// relations: ['post' => new HiddenFieldsPost()]

(new StreamedJsonResponse(['user' => $user]))->sendContent();
// {"user":{"post":{"secret":"hidden"}}}
// expected {"user":{"post":{"only":"public"}}}

This exposes exactly the fields the relation's override hides. Wrapping the encoding in sendContent() with Model::serializingJson() should fix it. Please add a test next to testJsonResponseUsesRelationOverrideWhenTheParentCallsToArray().

Please fix in this PR too

3. ->where() on one route moves another method's route name

rekeyCurrentRoute() rewrites every named route with the same path, whatever its method:

$router->get('/posts/{id}', fn() => 'show')->name('posts.show');
$router->post('/posts/{id}', fn() => 'update')->where(['id' => '\d+']);

// GET /posts/5
$router->currentRouteName(); // null, expected 'posts.show'

This bug already exists on 4.x, but $namedRouteMethods now makes it a small fix: only rekey names recorded for the current method.

4. JsonErrorHandler drops the exception's headers

The new HttpException branch takes the status code but not getHeaders(), so a Retry-After is lost on this path. That's the same problem this PR fixes for web error pages in Response::dispatchHttpException(). Suggested fix:

$response = $renderer->render($exception, $this->resolveStatusCode($exception));

if ($exception instanceof HttpException) {
    $response->withHeaders($exception->getHeaders());
}

Looks good

  • ANY is no longer a valid method, so both a real ANY request and _method=ANY get a 400.
  • old() goes through the session's peek data, so every field stays readable for the rest of the request and array fields return arrays.
  • Validation: mb_strlen() for min, max and size; date_parse() warnings for impossible dates; \z anchors; whitespace checks for int, numeric and required; and numbers converted to strings for in and not_in.
  • back() uses the fallback for a blank Referer.
  • JsonErrorHandler uses only codes from 400 to 599.
  • currentRouteAction() reuses the dispatched route.
  • The legacy-decryption change only adds a preferred candidate, so it can't reject a value that decrypted before.

- currentRouteName() no longer returns another method's name; names keep
  their path per method, including in the route cache
- where() only moves the name of the route it constrains
- StreamedJsonResponse uses a relation's jsonSerialize() when encoding models
- JsonErrorHandler keeps an HttpException's headers

@abdulmajidcse abdulmajidcse left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@techmahedy All four are done in 8c6f8cd:

  1. currentRouteName() no longer falls back to another method's name, and an attribute route keeps its name for every method. I stored name => [method => path] instead of a list of methods, because a list still gave the wrong name when a name was reused on another path. It's saved in the route cache.
  2. StreamedJsonResponse now uses the relation's jsonSerialize(). I wrapped only the model encoding, not all of sendContent(), so toArray() inside a generator still returns plain data.
  3. ->where() only moves the name of the route it constrains.
  4. JsonErrorHandler keeps the HttpException headers.

The full suite and PHPStan pass locally.

Not changed, as they weren't in your review: is() still ignores the method, and WebErrorHandler doesn't apply the exception's headers.

@techmahedy techmahedy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @abdulmajidcse, all four are fixed in 8c6f8cd, and the tests cover each case from the review, plus attribute routes, older route caches and HEAD requests. It also merges cleanly with #401.

@techmahedy
techmahedy merged commit 56c8c7a into 4.x Nov 5, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants