Skip to content

Using Response constants instead of plain integers #31394

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
gabiudrescu opened this issue May 6, 2019 · 6 comments
Closed

Using Response constants instead of plain integers #31394

gabiudrescu opened this issue May 6, 2019 · 6 comments

Comments

@gabiudrescu
Copy link
Contributor

Symfony version(s) affected: all

Description
Our (new) marketing team asked a pretty simple question: why are we using HTTP 302 redirect codes on almost all our redirects.

So I dived in the codebase and realized that we don't use Response::HTTP_FOUND or 302 as integer at all. Puzzled, I started checking controller by controller and found out that we rely mostly on redirectToRoute(https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/Controller/ControllerTrait.php#L112) from Framework Bundle.

Which internally is using a hardcoded integer instead of the constant from Response class (https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpFoundation/Response.php#L37)

I wonder if there is any reason for using plain integers instead of constants.

Possible Solution
I would suggest replacing all hardcoded HTTP response codes with constants from Response class from HttpFoundation.

@stof
Copy link
Member

stof commented May 6, 2019

Well, in the core, we already rejecting multiple time using the constants rather than the HTTP codes themselves, as we find the code more readable (for people knowing their meaning in HTTP) than having to remember what each constant name represent as actual HTTP code.

@stof
Copy link
Member

stof commented May 6, 2019

Btw, I don't see how using the constant would change anything for you in that case. The question you wanted to answer even asked for 302, not for HTTP_FOUND. So even your marketing team thinks in integers for HTTP codes, not in names.

@javiereguiluz
Copy link
Member

@gabiudrescu for reference, this was proposed several times in the past, but it was always rejected for the reasons given by @stof:

#11401
#22023
#29812
...

@gabiudrescu
Copy link
Contributor Author

gabiudrescu commented May 6, 2019

thank you, guys, for your fast response.

when I searched before for any other open issues with similar keywords, I couldn't find all the links mentioned above by @javiereguiluz. though, I must admit, I didn't check the closed ones.

@stof I told the story of how I ended up opening this issue to give some context. basically, what I wanted to tell is that we started from searching our codebase from the constants from Response class and couldn't find them being used at all.

searching for 301 or 302 in our codebase also retrieved a couple of results that were unrelated and then I started searching controller by controller. that's how I discovered that in vendor integers are used instead of constants - answering me the question "why aren't no 302 redirects based on the constant, either in src or in vendor".

I can relate with your decision of using integers instead of constants for brevity and because you got used with the codes. But my suggestion to use constants instead was because I thought it would be easier for a developer to search where is a redirect code used by using the IDE function to find usages of a certain constant.

That way, I could have answered to the question: where in our codebase we are using 301 redirects and where we are using 302 much easier - instead of relying on a integer search.

But, the issue is very small and without much gain - if there is a consensus over using integers instead of constants for HTTP codes, fine by me - I will close the issue.

@stof
Copy link
Member

stof commented May 6, 2019

well, in your own codebase, you can still use constants. That's why they have been added even though there are not used in core: giving choice to each project.
But until now, there is indeed a consensus in the core team over using integers for HTTP codes.

@gabiudrescu
Copy link
Contributor Author

cool.

thanks for the feedback.

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

No branches or pull requests

4 participants