[techmahedy]

Pull Request Checklist

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

Description

An end-to-end review of the HTTP request/response path (Request → body parsing → Router → Response/Factory → stream, redirect and error responses). It fixes real bugs, removes duplicated and dead code, and makes HEAD requests work on GET routes.

Bug fixes

Request

• Trusted proxies were ignored by ip(), ips() and isValidRequest(). They read static properties through $this->trustedHeaderSet, which evaluates to 0 and emits a notice plus a warning on every call. They now use the proxy-aware getClientIp()/getClientIps(). ips() returned []; it now returns the client IP first.
• scheme() ignored X-Forwarded-Proto. It now uses getScheme().
• getRouteParam() was a fatal error. It indexed ParameterBag as an array.
• input() with no key returned null instead of all input. Its internal cache also went stale after direct changes to the request bag. The cache is removed.
• JSON bodies:
  • an empty JSON body crashed with a 500 error; it is now treated as no body.
  • a scalar JSON body crashed with a 500 error; it is now ignored.
  • malformed JSON now returns 400 Bad Request. Before, it caused a PHP fatal and an empty 500, because the error handler called request() and re-parsed the broken body. createFromGlobals() now keeps a usable request and stores the failure; Application::handle() raises it as a BadRequestHttpException.
• URL-encoded bodies dropped query-string parameters, unlike JSON bodies. All methods now keep them, with body values winning on conflicts.

Response

• setExceptionError() stored the status code instead of the exception.
• text() set the status code without the status text, so text('x', 404) sent 404 OK.
• ResponseFactory::text() modified a shared container instance; it now creates a new response.
• Streamed and file responses overrode the unused setContent() instead of setBody(), so stream callbacks ran on HEAD/204/304 responses.
• prepare() called isHead('HEAD'), which takes no argument.
• The informational-response path called a misnamed method instead of the SAPI headers_send() function.
• Error views were never found when the framework isn't installed under vendor/doppar/framework (for example a path repository). The lookup is now package-relative and shared by Response and RequestAbortion.
• response()->json($queryBuilder) encoded the builder object itself rather than its results.

Routing

• HEAD requests to GET-only routes returned 404. All route lookups (callback, middleware, current route name) now resolve a HEAD request to an explicit Route::head() route if one matches, otherwise to the GET route. The handler and its middleware always come from the same route, so auth and similar middleware still apply. HEAD never falls back to POST, PUT, PATCH or DELETE.

Duplication and dead code removed

• JSON detection and serialization now live in one place, Response::shouldBeJson() and Response::toSerializable(). ResponseFactory and Router each had their own, slightly different copy.
• Deleted the unused BaseJsonResponse. DEFAULT_ENCODING_OPTIONS now has one source instead of three copies.
• Removed Symfony URI/base-URL preparation code that could never run: prepareRequestUri, prepareBaseUrl, preparePathInfo, getUrlencodedPrefix, isIisRewrite and initializeHeaders.
• Removed an unreachable branch in Application::handle() and a dead is_array() guard in Rule::sanitize().
• Smaller deduplications:
  • auth() now delegates to user().
  • hasFiles() reuses hasFile().
  • ip()/ips() and scheme() now delegate to the proxy-aware methods.
  • the redirect and response header loops use withHeaders().
• Rewrote the Response facade docblock in valid @method syntax. This removed 51 PHPStan baseline suppressions.

Breaking changes

• Behind a trusted proxy, ip() and scheme() return the real client values, and ips() lists the client IP first.
• Values are sent as JSON or as plain strings by the same rules everywhere. response($model) and other JsonSerializable values now produce JSON. An object that implements only Stringable is now sent as its string value.
• response()->redirect() returns a RedirectResponse with a standard redirect body.
• Form POSTs now include query-string parameters in all().
• Removed:
  • BaseJsonResponse
  • StreamedResponse::DEFAULT_ENCODING_OPTIONS
  • Response::headers_send()
  • the public $request->input property
  • setContent()/getContent() on stream and file responses
  • the unused $excludeKeys argument of failed()
• setException() now requires a Throwable or null.
• A HEAD request to a GET-only route now returns that route's response instead of 404.

Checklist

• Tests have been added or updated
• Documentation has been updated fix(http): make request/response handling consistent end to end docs#120
• Code follows the project coding standards
• All tests pass locally

[abdulmajidcse]

@techmahedy Tests and PHPStan pass, but I found these issues at ac2923f:

New in this PR

1. Request.php:249: array_merge($request, $data) renumbers and duplicates numeric keys. 10=a&20=b becomes ["a","b","a","b"]. array_replace() fixes it.
2. Response.php:1586: toSerializable() calls toArray() on models and skips a custom jsonSerialize(), which can expose hidden fields.
3. Router.php:1198: the HEAD memo goes stale when getOriginalClosure() reloads routes, so cached apps return 404 for HEAD closure routes.
4. RequestParser.php:16: when trusted Forwarded and X-Forwarded-For disagree, ip() throws a generic \Exception, then returns 0.0.0.0 on the next call.

Already in the base (not caused by this PR)
5. Router.php:1372: auth bypass. /admin/ reaches the /admin handler with no route middleware, because getCurrentRouteMiddleware() doesn't trim the trailing slash.
6. Request.php:1457: an invalid X-HTTP-Method-Override or _method throws from the constructor, outside the new try.

Smaller
7. Response.php:646: there is no 400.odo.php, so malformed JSON falls through to abort() instead of a clean 400.
8. RequestParser.php:27: the ips() docblock says the client is first, but the list is most-trusted first.
9. WebErrorRenderer.php:239: still hardcodes vendor/doppar/framework; it should use resolveErrorView().
10. Request.php:430: isValidRequest() now has no callers, so it should be removed or corrected.

[techmahedy]

Thanks @abdulmajidcse, great catches. All addressed in the next push:

1. Body parsing and merge() now use array_replace(), so numeric keys are kept.
2. JSON output defers to jsonSerialize(), including for models inside a Collection (Collection::jsonSerialize() had the same gap).
   3 + 5. The router now matches a request once and remembers the dispatched route. Middleware and route-name lookups use that route's key, which fixes the stale HEAD memo, the trailing-slash auth bypass (including parameterized and domain-restricted routes), and lost middleware on closure routes in cached apps.
3. Conflicting forwarding headers throw ConflictingHeadersException (400); later lookups return the connecting address instead of 0.0.0.0.
4. An invalid method or override is recorded instead of thrown from the constructor, and handle() returns 400. getBodyParseError() is renamed to getMalformedRequestError().
5. Added a bundled 400.odo.php, with $message escaped.
6. Docblock corrected: ips() is ordered most-trusted first, and the originating client is last.
7. WebErrorRenderer uses Response::resolveErrorView().
8. Removed isValidRequest().

There are 18 new regression tests; each fails on ac2923f. The full suite and PHPStan pass.

[abdulmajidcse]

@techmahedy Thanks, most of the fixes check out. Still open at 8d4283c:

Fix before merge

1. Response::dispatchHttpException(): now that 400.odo.php exists, malformed JSON on /api routes gets the HTML page instead of a JSON error. It renders the view without checking for API or AJAX requests.
2. Request::getMethod(): _method[]=PUT still throws a TypeError from strtoupper(), because validateMethodOverride() only catches BadRequestHttpException. It should return a 400.

Worth fixing here
3. Model::toArray(): related models still skip a custom jsonSerialize(), so $user->posts can expose fields the override hides.
4. Request::getMethod() (also in the base): GET /posts/1?_method=DELETE reaches the DELETE handler. Overrides should only apply to real POST requests.

Separately, EncryptionTest::test_legacy_ciphertext_is_not_decrypted_to_garbage_by_the_other_key is flaky. This PR doesn't cause it, and it doesn't need to block the merge.

[techmahedy]

Thanks @abdulmajidcse, all four are fixed in the next push:

1. dispatchHttpException() now returns the abort()-style JSON error for API/AJAX requests, even when an HTML view exists for the status.
2. A non-string _method (e.g. _method[]=PUT) is now a 400 "Invalid HTTP method override." instead of a TypeError.
3. While JSON-serializing, loaded relations go through their own jsonSerialize(). A model's toArray() override still shapes its JSON, and a direct toArray() call is unchanged.
4. Method overrides now apply only to real POST requests, so GET /posts/1?_method=DELETE reaches the GET route. I updated RequestTest::testGetMethod, which asserted the old behavior.

There are 11 new tests; the 7 covering these fixes fail on 8d4283c.

[abdulmajidcse]

@techmahedy One more at 309ef40: Router::any() (Router.php:618) still reads $request->_method directly:

• _method[]=PUT throws a TypeError in addRoute(). Routes load during boot, so this should happen before the 400 check in handle().
• GET /any?_method=DELETE no longer matches: the route registers under DELETE, but getMethod() now returns GET.

Using $method = $request->getMethod(); fixes both.

[techmahedy]

Thanks @abdulmajidcse, fixed as suggested: any() now registers under $request->getMethod(). This also covers a third case: a lowercase _method=put was registered as put and never matched.

Checked in a real app against 309ef40:

• POST _method[]=PUT: was a 500 while loading routes, now a 400
• GET ?_method=DELETE: was a 404, now handled as GET
• POST _method=put: was a 404, now handled as PUT

There are 3 new router tests, which fail on 309ef40.

[abdulmajidcse]

@techmahedy The any() fix looks good. Two regressions from the base are left at 1bdafda:

1. Request::parseRequestBody(): query params are now merged into the request bag, so POST /m?_method=DELETE becomes DELETE, and ?_method=garbage records a 400. Overrides should only come from the body:

   $query = $this->query->all();
   unset($query['_method']);
   
   $request = $this->request->all() + $query;

2. ResponseFactory::make(): make($response, 503, ['Retry-After' => '10']) now returns 200 with no header, because an existing response is returned as is:

   if ($content instanceof Response) {
       if ($status !== 200) {
           $content->setStatusCode($status);
       }
   
       return $content->withHeaders($headers);
   }

With both applied on a local copy, the suite and PHPStan pass. Neither behavior has a test yet, so please add regression tests.

[techmahedy]

Thanks @abdulmajidcse, both applied as suggested:

1. parseRequestBody() no longer merges a query-string _method into the input, so POST /m?_method=DELETE stays POST and ?_method=garbage is no longer a 400. A body _method still works, and the raw value stays readable via $request->query->get('_method').
2. make() applies the passed status (when it isn't 200) and headers to an existing response.

[abdulmajidcse]

@techmahedy Thanks, the latest fixes look good. We can merge this now.