Skip to content

[Controller] Improved the hardcoded status for the redirection in the base Controller. #16514

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

Conversation

jcchavezs
Copy link

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

The default status code in the redirect and redirectToRoute methods are being hardcoded. Can we use the constant values from Symfony\Component\HttpFoundation\Response instead?

…fony\Bundle\FrameworkBundle\Controller\Controller
@stof
Copy link
Member

stof commented Nov 10, 2015

I don't see how this avoids hardcoding it. the value is still hardcoded.

and we agreed in the past that we would not use these constants internally in Symfony. Status code are more readable than seeing the constant name and having to remember which status code it represents (especially for common status codes)

@jcchavezs
Copy link
Author

@stof Yes but this is hardcoded in just one place. By the way I think it is much more readable HTTP_FOUND (which is text) than 302.

@jvasseur
Copy link
Contributor

I think most web developers know http status codes and not necessarily the corresponding names. I personally know what a 302 redirection is but don't know what is the corresponding name.

@javiereguiluz
Copy link
Member

In the past changes like this have been refused. Please refer to this discussion to know more about the reasons of doing it. Thanks!

@fabpot
Copy link
Member

fabpot commented Nov 11, 2015

Closing as it does add any value, the constant value won't change anyway, that's HTTP.

@fabpot fabpot closed this Nov 11, 2015
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.

6 participants