Skip to content

[FrameworkBundle] Add support to 307/308 HTTP status codes in RedirectController #26213

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 4 commits into from

Conversation

ZipoKing
Copy link
Contributor

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

With this PR RedirectController will allow to create redirections with use of 307/308 HTTP status codes together with 301/302. Related RFC documents:

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for working on this; see comments

*
* @throws HttpException In case the route name is empty
*/
public function redirectAction(Request $request, string $route, bool $permanent = false, $ignoreAttributes = false): Response
public function redirectAction(Request $request, string $route, bool $permanent = false, $ignoreAttributes = false, bool $keepRequestMethod = false): Response
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for other reviewers: the class is final, so this is not a BC break

{
if ('' == $route) {
throw new HttpException($permanent ? 410 : 404);
throw new HttpException($permanent ? Response::HTTP_GONE : Response::HTTP_NOT_FOUND);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we prefer explicit numbers; please revert, and use numbers everywhere in the PR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nicolas-grekas Change reverted

@fabpot
Copy link
Member

fabpot commented Feb 19, 2018

Thank you @ZipoKing.

@fabpot fabpot closed this Feb 19, 2018
fabpot added a commit that referenced this pull request Feb 19, 2018
…odes in RedirectController (ZipoKing)

This PR was squashed before being merged into the 4.1-dev branch (closes #26213).

Discussion
----------

[FrameworkBundle] Add support to 307/308 HTTP status codes in RedirectController

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

With this PR `RedirectController` will allow to create redirections with use of 307/308 HTTP status codes together with 301/302. Related RFC documents:
* https://tools.ietf.org/html/rfc7231
* https://tools.ietf.org/html/rfc7538

Commits
-------

64fb5a5 [FrameworkBundle] Add support to 307/308 HTTP status codes in RedirectController
@stof
Copy link
Member

stof commented Feb 19, 2018

this needs a documentation PR though

@ZipoKing
Copy link
Contributor Author

@stof will prepare some docs PR later today

javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Mar 5, 2018
…7/308 HTTP status codes (ZipoKing, javiereguiluz)

This PR was merged into the master branch.

Discussion
----------

[FrameworkBundle] PR #26213 Document redirections with 307/308 HTTP status codes

This documents changes introduced with pull request symfony/symfony#26213

Commits
-------

9468bf9 Reword and added an example
c8ce65d Changes after @Simperfit  review
b3984d0 [PR #26213 Document redirections with 307/308 HTTP status codes
@fabpot fabpot mentioned this pull request May 7, 2018
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