Skip to content

[HttpKernel] Use constants for http statuses #22023

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

Closed
wants to merge 1 commit into from
Closed

[HttpKernel] Use constants for http statuses #22023

wants to merge 1 commit into from

Conversation

enleur
Copy link
Contributor

@enleur enleur commented Mar 16, 2017

Q A
Branch? master
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets no
License MIT
Doc PR no

@fabpot
Copy link
Member

fabpot commented Mar 16, 2017

This has been proposed several times in the past and it's been refused. There is no benefit for core, and core team members prefer int codes, which they find more readable. Thanks.

@Geolim4
Copy link

Geolim4 commented Feb 4, 2019

Hello @fabpot I'm not totally agreeing with you about benefit:

  • "Core member prefer int codes, which they find more readable"
    • Reading an HTTP status code constant is more descriptive than reading a primitive int,eger because you don't have to think about what integer code means when the constant name already tells you the purpose.
  • Consistency: HttpKernel depends on HttpFoundation for multiple reasons, this one would be one of the multiple reason why HttpKernel depends on HttpFoundation. Make use of the benefit of dependency as much as you can, I mean, if you plan to depend on another component, do it for real :)
  • Code reusability: Aside of using hard-coded integer, there's one of the Symfony catchword "Symfony is a set of reusable PHP components" that should be taken into consideration. The "reusable" word make all sense in this PR.
  • Refactoring: Unlikely risk, but not non-existing: If some day, for some reason, an HTTP code, came to change, the refactoring would be less complicated.

So, I'm encouraging you to rethink about pertinency of this PR which should be, IMO, re-evaluated :)

Cheers,

@jvasseur
Copy link
Contributor

jvasseur commented Feb 4, 2019

* "Core member prefer int codes, which they find more readable"
  
  * Reading an HTTP status code constant is more descriptive than reading a primitive int,eger because you don't have to think about what integer code means when the constant name already tells you the purpose.

For me it's the opposite, I'm used to read HTTP codes and don't necessary know the reason phrase associated. Especially for some that doesn't really explain what they do (302 and 303 for example: the names are not descriptive where the code at least show that they are redirects).

* Consistency: HttpKernel depends on HttpFoundation for multiple reasons, this one would be one of the multiple reason why  HttpKernel depends on HttpFoundation. Make use of the benefit of dependency as much as you can, I mean, if you plan to depend on another component, do it for real :)

* Code reusability: Aside of using hard-coded integer, there's one of the Symfony catchword "Symfony is a set of **reusable** PHP components" that should be taken into consideration. The "**reusable**" word make all sense in this PR.

I don't see the points here, forcing yourself to use more feature of a dependency you have when you don't need them seems like a bad design decision.

* Refactoring: Unlikely risk, but not non-existing: If some day, for some reason, an HTTP code, came to change, the refactoring would be less complicated.

Actually depending on the constants would increase the risks as the reason phrase (which the constants are base on) is much more likely to change than the status code.

Additionally, reason phrases are completely dropped from the spec in HTTP2 so we should treat them as something "legacy" and not use them more.

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

Successfully merging this pull request may close these issues.

5 participants