Skip to content

Conversation

@kenjis
Copy link
Member

@kenjis kenjis commented Apr 20, 2023

Description
With this PR, if you specify status code for redirect, the code is surely used in the response.
$routes->addRedirect() specifies 302 by default. So 302 will be used by default.

  • fix: specified code may not be used in redirect (Should use the specified code)
  • fix: default value to null for $code in RedirectResponse::route() (For consistency)
  • fix: HEAD/OPTION request changes to GET because of 303
  • change the default code to 302 for GET request (No need to use 307)

Ref #3057

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@kenjis kenjis marked this pull request as draft April 20, 2023 13:04
@kenjis kenjis added bug Verified issues on the current code behavior or pull requests that will fix them breaking change Pull requests that may break existing functionalities labels Apr 20, 2023
@kenjis kenjis force-pushed the fix-redirect-status-code branch from 3ddc545 to ae8ac2c Compare April 20, 2023 13:17
@iRedds
Copy link
Collaborator

iRedds commented Apr 20, 2023

  1. The default redirect code should be 302.
  2. There should be no code substitution inside the handler.

301 and 302 for a redirect without data (GET -> GET). GET, HEAD, (OPTIONS)?
308 and 307 for data redirect (POST -> POST). POST, PUT, PATCH
303 for redirect after request processing (POST -> GET)

I hope you fix it, because the POST -> POST redirect in the current implementation will replace 307 or 308 with 303.

@kenjis
Copy link
Member Author

kenjis commented Apr 20, 2023

There should be no code substitution inside the handler.

Why?
It is done since v3.
https://github.com/bcit-ci/CodeIgniter/blob/63d037565dd782795021dcbd3b1ca6f8c8a509e4/system/helpers/url_helper.php#L522

@iRedds iRedds closed this Apr 20, 2023
@iRedds iRedds reopened this Apr 20, 2023
@kenjis
Copy link
Member Author

kenjis commented Apr 20, 2023

I hope you fix it, because the POST -> POST redirect in the current implementation will replace 307 or 308 with 303.

I think it will be fixed by this PR.
See 4da5f0d#diff-e3e8e9e1b70160a0f569050d2f248d3df024213a3bd66909a3d0308024b60638

@kenjis kenjis added the docs needed Pull requests needing documentation write-ups and/or revisions. label Apr 20, 2023
Comment on lines 519 to 521
$code = ($_SERVER['REQUEST_METHOD'] !== 'GET')
? 303 // reference: https://en.wikipedia.org/wiki/Post/Redirect/Get
: 307;
Copy link
Collaborator

Choose a reason for hiding this comment

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

For the HEAD and OPTIONS methods, there will be a 303 code, that is, a redirect with the GET method.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@iRedds
Copy link
Collaborator

iRedds commented Apr 20, 2023

Why? It is done since v3. https://github.com/bcit-ci/CodeIgniter/blob/63d037565dd782795021dcbd3b1ca6f8c8a509e4/system/helpers/url_helper.php#L522

There is a lot written in CI 3. But that doesn't mean it's right.
Don't get too attached to things. Learn to let go(с) Ivan Vanko.

@kenjis
Copy link
Member Author

kenjis commented Apr 20, 2023

There is a lot written in CI 3. But that doesn't mean it's right.

Right or wrong is just a direction. If you want to go to the North Pole, it is right to go north.
It is not right to go south.

Yes, if there is a code in CI3, it does not means the code is right code.
But if we change the code, apps might break.
So more careful consideration is needed to make the change, and
we need to show the reason why.

I just want you to show why your opinion is right.

@iRedds
Copy link
Collaborator

iRedds commented Apr 21, 2023

I will repeat what I said above.

  1. The 303 response code implies that a POST request was sent, processed, and the processing result can be obtained at the specified URL via a GET request.
  2. Response code 307 implies that the resource was found but temporarily moved to the specified URL, so you need to use the same method with which the request was sent and resend the request body.

Regarding code CI3.
The code assumes that if the response code is not passed as an argument, then any non-GET request is a POST request (given the link to the wiki). That is, it is processed and the result can be obtained from the specified link through the GET method.

The HEAD and OPTIONS methods are not GET, which means that they will have a 303 response code, but this makes no sense when redirecting using the GET method.

For a GET request, it makes no sense to use 307 (308) response codes, since a GET request does not imply a request body. It makes more sense to use 302 (301).

Conclusion.
We can set the default value of the code argument to 303. And remove the condition for defining the response code.

For non-GET methods, the behavior will not change. Since they are already redirected to 303.
For the GET method, the behavior will not change. Since the body of the GET request is not provided.

@kenjis kenjis force-pushed the fix-redirect-status-code branch from ae8ac2c to f5bbe1a Compare April 21, 2023 10:30
@michalsn
Copy link
Member

Maybe we could add https://developer.mozilla.org/en-US/docs/Web/HTTP/Redirections as a reference for redirect() method? I think this could be useful regarding the rules we're following.

IMO this is a good change.

@kenjis kenjis force-pushed the fix-redirect-status-code branch from d79bacd to 31bdd79 Compare April 25, 2023 02:39
@kenjis kenjis removed the docs needed Pull requests needing documentation write-ups and/or revisions. label Apr 25, 2023
@kenjis
Copy link
Member Author

kenjis commented Apr 25, 2023

Added docs.

@kenjis kenjis requested review from iRedds and michalsn April 25, 2023 02:40
@kenjis kenjis marked this pull request as ready for review April 25, 2023 02:40
@kenjis kenjis force-pushed the fix-redirect-status-code branch from 31bdd79 to 1c3a3b7 Compare April 26, 2023 04:29
@kenjis
Copy link
Member Author

kenjis commented Apr 26, 2023

Rebased to resolve conflicts, and tweaks the added note (the last 3 commits).

@kenjis kenjis requested a review from michalsn April 26, 2023 04:32
@kenjis kenjis merged commit a3a35df into codeigniter4:develop Apr 26, 2023
@kenjis kenjis deleted the fix-redirect-status-code branch April 26, 2023 06:32
@kenjis
Copy link
Member Author

kenjis commented Apr 26, 2023

@michalsn Thank you.

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

Labels

breaking change Pull requests that may break existing functionalities bug Verified issues on the current code behavior or pull requests that will fix them

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants