Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce Http status code response helper #53691

Closed
wants to merge 1 commit into from

Conversation

jasonmccreary
Copy link
Contributor

@jasonmccreary jasonmccreary commented Nov 27, 2024

Anytime I work on APIs I alway feel it would be smooth to quickly return a status code from a controller action.

public function store(Request $request)
{
    // ...

    return 201;
}

However, this has underlying challenges - breaking change with existing apps and potentially unexpected side-effects of returning a raw integer.

So, in the spirit of the to_route() helper, I propose status. Similarly this code reads very well, "return status 201". I've added a fluent API for named status.

Examples

// passing integer
return status(201);

// fluent API
return status()->created();

The fluent API method names are the snake case names of the Response constants.

Additional Examples

return status()->ok();
return status()->unauthorized();
return status()->methodNotAllowed();
return status()->iAmATeapot();

@jasonmccreary
Copy link
Contributor Author

jasonmccreary commented Nov 27, 2024

During my livestream, I did do benchmarks on this as I was curious the anonymous class. However, the difference between the fluent API and passing an integer were negligible (~.002ms difference). Memory was also negligible. PHP has heavily optimized anonymous classes by storing them in memory. So it is not "rebuilt" on every call.

@andrey-helldar
Copy link
Contributor

There are not enough doc-blocks to let the IDE know which hints to output in the fluent method.

@Wirone
Copy link

Wirone commented Nov 28, 2024

Why would you need a helper function with fluent API if self-explanatory constants are already available, readable and 100% supported by IDE? I personally don't see much value in this.

But even if we consider it as a shortcut for creating an empty response, then:

  • status() alone is not intuitive, it does not describe the intent well. It should be at least http_status(), but it also is not that great.
  • return type object for null code is totally not helpful. It does not provide any help for IDEs, because there's only magic under the hood.

@davorminchorov
Copy link

davorminchorov commented Nov 28, 2024

The Response class has a ton of HTTP status codes defined as constants which you can use.

return Response::HTTP_CREATED;

@tontonsb
Copy link
Contributor

I think we can already almost do the integer version via return response(status: 201);, right?

@jasonmccreary this and the #53663 would've been good applications for an enum. return Status::created and there's no mistaking it for a response body.

@davorminchorov doing that would return a response with body 204 not one with status 204.

@jasonmccreary
Copy link
Contributor Author

@tontonsb, yes, underneath it uses response()->noContent(...). Just a shorthand, much like to_route() is for redirect()->route(...).

Enums would be nice, but present the same challenge of fundamentally altering how the framework handles return values. As such, that would need to be targeted at 12.x.

@tontonsb
Copy link
Contributor

@jasonmccreary I'm not sure about needing to target 12.x.

If you create a new enum and add a case among these containing something like if ($response instanceof Status) $response = response(status: $response->value);, how could it conflict with anything existing?

@jasonmccreary
Copy link
Contributor Author

@tontonsb, I toyed around with this idea and don't think that's the only path. But test it out and open a PR if you like that approach better. 👍

@x7ryan
Copy link

x7ryan commented Nov 28, 2024

I get you want a shorthand but this is just shortening response(code: 401); or response('', 401); to status(401);.

As for the fluent methods (which I absolutely like the idea of btw), IMO it makes more sense to add to the response helper, like response()->ok();, response()->NotFound(); ect.

Just my 2 cents.

@jasonmccreary
Copy link
Contributor Author

jasonmccreary commented Nov 28, 2024

@x7ryan, yeah, that would be another approach. noContent is kind of the only "status method" on Response. As noted, this was kind of the "least invasive" approach. If the Laravel Team is open to returning enums or expanding Response, I'm all for it. 👍

We'll see where this lands...

@shaedrich
Copy link
Contributor

shaedrich commented Nov 28, 2024

  • status() alone is not intuitive, it does not describe the intent well. It should be at least http_status(), but it also is not that great.

response_status() would fit more with existing framework nomenclature, I'd say 🤔

@jasonmccreary this and the #53663 would've been good applications for an enum. return Status::created and there's no mistaking it for a response body.

Even though, I'd name the enum HttpStatusCode or even HttpResponseStatusCode, I absolutely love the idea. This would definitely solve the problem discussed in #53663 (comment)

Enums would be nice, but present the same challenge of fundamentally altering how the framework handles return values. As such, that would need to be targeted at 12.x.

This could also be done gradually in two PRs: One introducing the enum itself in 11.x as "free to use by devs" and one using it in the framework itself in 12.x as @tontonsb suggested

potentially unexpected side-effects of returning a raw integer

If the number handling of #53663 was to be reverted in favor of this PR, this still wouldn't be perfect, yet a huge improvement 👍🏻

noContent is kind of the only "status method" on Response.

Good catch! That's weird tbh. I would have expected this to be part of a trait/concern among other known status code methods 🤔

What's even weirder, is that its status code can be changed to any other number:

/**
* Create a new "no content" response.
*
* @param int $status
* @param array $headers
* @return \Illuminate\Http\Response
*/
public function noContent($status = 204, array $headers = [])
{
return $this->make('', $status, $headers);
}

@tontonsb
Copy link
Contributor

I think it's fair to add few words on why enums (that I brought up earlier myself) might not be appropriate for status codes, but would still be fine here.

Enums are intended for closed sets of values. By accepting an enum we say "here's the list of cases we handle. and nothing else". But status codes are extensible (both by RFC and in practice), e.g. 292 is a valid code that must be understood as a successful response.

But the feature under discussion is a convenience one. By allowing return Http::created and handling it in the router as a status code instead of response contents, we are not preventing return response(status: 277); when someone needs it.

Meanwhile the "appropriate" solution for status codes would be a class hierarchy where Created extends Successful and you can return new Status\Created(); as well as add your own subclasses. But the syntax is messier. Unless you add helper methods on top :D

Good catch! That's weird tbh. I would have expected this to be part of a trait/concern among other known status code methods

@shaedrich what else would you need? The framework chooses between 200 and 201 automagically. You have noContent() for 204. You have abort(4xx) for all the denies, as well as throws for 4xx and 5xx. The only code I've been specifying manually is 303.

@shaedrich
Copy link
Contributor

Good catch! That's weird tbh. I would have expected this to be part of a trait/concern among other known status code methods

@shaedrich what else would you need? The framework chooses between 200 and 201 automagically. You have noContent() for 204. You have abort(4xx) for all the denies, as well as throws for 4xx and 5xx. The only code I've been specifying manually is 303.

There'd be 202 as well, for example, though it's way less frequently used than 200.

@taylorotwell
Copy link
Member

I think I'm ok holding off on this since response with named arguments is not too bad:

return response(status: 204);

@shaedrich
Copy link
Contributor

shaedrich commented Nov 29, 2024

Well, the docs state:

Named Arguments

Named arguments are not covered by Laravel's backwards compatibility guidelines. We may choose to rename function arguments when necessary in order to improve the Laravel codebase. Therefore, using named arguments when calling Laravel methods should be done cautiously and with the understanding that the parameter names may change in the future.

So, giving the advise to use named arguments kind of goes against this rule

@andrey-helldar
Copy link
Contributor

So, giving the advise to use named arguments kind of goes against this rule

Not contradictory in my opinion. Just a notice that in major versions of Laravel they can be renamed if the changed behavior of the logic requires it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants