Skip to content
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

You cannot set a 303 redirect response using a result factory #9028

Closed
jameshalsall opened this issue Mar 27, 2017 · 11 comments
Closed

You cannot set a 303 redirect response using a result factory #9028

jameshalsall opened this issue Mar 27, 2017 · 11 comments
Assignees
Labels
Fixed in 2.2.x The issue has been fixed in 2.2 release line Fixed in 2.3.x The issue has been fixed in 2.3 release line Issue: Clear Description Gate 2 Passed. Manual verification of the issue description passed Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed Issue: Format is valid Gate 1 Passed. Automatic verification of issue format passed Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release up for grabs

Comments

@jameshalsall
Copy link

jameshalsall commented Mar 27, 2017

Preconditions

  1. Tested in Magento EE 2.1.4
  2. Chrome on Mac OS

Steps to reproduce

  1. Perform a 303 redirect from any controller (see code below)
  2. Send a request to that controller endpoint
  3. Examine the response code

Expected result

  1. A 303 response code should be returned

Actual result

  1. A 302 response code is used instead

Example code (use in any controller extending the default Action):

$result = $this->resultRedirectFactory->create();
$result->setHttpResponseCode(303);

return $result->setPath('checkout/cart');
@orlangur
Copy link
Contributor

orlangur commented Apr 1, 2017

Why need this? Please describe a complete use case.

@jameshalsall
Copy link
Author

Why do we need a 303 status code? Surely that question answers itself.

Use case:

  • I want to return a 303 response.

@orlangur
Copy link
Contributor

This is not a use case. For which purpose do you need such a status code?

There are a lot of codes with some $statusCode = $isPermanent ? 301 : 302 logic. To change this there should really be a reason.

@jameshalsall
Copy link
Author

What you are asking is "why do you want to set a 303 response code?" which falls outside the remit of discussion in my opinion. Magento should be flexible enough to allow a custom response code, especially when the public API ($result->setHttpResponseCode()) implies that is allowed.

If you must know the use case, I am using XHR to send a PUT request, and then I want to send a 303 response code so it follows the redirect using GET instead.

@orlangur
Copy link
Contributor

Thanks for your input,

I am using XHR to send a PUT request, and then I want to send a 303 response code so it follows the redirect using GET instead

That was exactly what I was asking for.

Magento should be flexible enough to allow a custom response code

Any additional flexibility means more maintenance efforts. But of course ANY limitation/unimplemented feature should be reasonable.

@orlangur
Copy link
Contributor

orlangur commented Sep 6, 2017

After getting familiar with implementation it looks like https://github.com/magento/magento2/blob/develop/lib/internal/Magento/Framework/Controller/ResultInterface.php#L13 is promising what you expect :)

In \Magento\Framework\Controller\AbstractResult::renderResult response code is set correctly first:

if (!empty($this->httpResponseCode)) {
    $response->setHttpResponseCode($this->httpResponseCode);
}

but then suddenly rewritten in render:

$response->setRedirect($this->url);

Fix seems to be as simple as

if (!empty($this->httpResponseCode)) {
    $response->setRedirect($this->url, $this->httpResponseCode);
} else {
    $response->setRedirect($this->url);
}

Anybody want to create a PR? \Magento\Framework\Controller\Test\Unit\Result\RedirectTest needs to be expanded of course.

@magento-engcom-team magento-engcom-team added G1 Passed Issue: Format is valid Gate 1 Passed. Automatic verification of issue format passed and removed G1 Passed labels Sep 7, 2017
@magento-engcom-team magento-engcom-team added the Issue: Clear Description Gate 2 Passed. Manual verification of the issue description passed label Oct 5, 2017
@magento-engcom-team
Copy link
Contributor

@jameshalsall, thank you for your report.
We've created internal ticket(s) MAGETWO-80996 to track progress on the issue.

@magento-engcom-team magento-engcom-team added 2.1.x Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release labels Oct 5, 2017
@gabrielqs-redstage
Copy link

I'm working on it #SQUASHTOBERFEST

@magento-team
Copy link
Contributor

Internal ticket to track issue progress: MAGETWO-81973

@okorshenko
Copy link
Contributor

The issue has been fixed in 2.2-develop branch and will be available with 2.2.2 release

@okorshenko okorshenko added the Fixed in 2.2.x The issue has been fixed in 2.2 release line label Nov 4, 2017
@magento-team
Copy link
Contributor

Hi @jameshalsall. Thank you for your report.
The issue has been fixed in magento-engcom/magento2ce#1286 by @magento-engcom-team in 2.3-develop branch
Related commit(s):

The fix will be available with the upcoming patch release.

@magento-team magento-team added the Fixed in 2.3.x The issue has been fixed in 2.3 release line label Jan 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fixed in 2.2.x The issue has been fixed in 2.2 release line Fixed in 2.3.x The issue has been fixed in 2.3 release line Issue: Clear Description Gate 2 Passed. Manual verification of the issue description passed Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed Issue: Format is valid Gate 1 Passed. Automatic verification of issue format passed Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release up for grabs
Projects
None yet
Development

No branches or pull requests

6 participants