Redirection fails on multiple status values #15737 #15738

Merged
merged 10 commits into from May 15, 2017

Conversation

Projects
None yet
6 participants
@anibalsanchez
Contributor

anibalsanchez commented May 2, 2017

Pull Request for Issue #15737 .

Summary of Changes

Fixed JApplicationWeb to generate the redirection headers with the same headers API that it is called on respond().

Now, it is generating the headers adding them as a raw header. If the there's an extra Status (e.g. a 201 defined with setHeader), it replaces the 303.

Testing Instructions

Test that all redirections work Ok. For example, when items are saved.

This issue was discovered with an extension running on FOF2 (the controller defined a Status 201 and, in a second step, calls a redirection). So, any extension running FOF2 is also a good test (save items).

Expected result

All redirections work OK.

Actual result

When multiple statuses are defined, the 303 redirection is lost and the status defined with setHeader('Status'... overrides any previous header('HTTP 1.1 303 See other');

Documentation Changes Required

@wilsonge

This comment has been minimized.

Show comment
Hide comment
@wilsonge

wilsonge May 2, 2017

Contributor

Unit test failures need investigating. The HTTP/1.1 being repeated in the status code definitely looks unintended?

Contributor

wilsonge commented May 2, 2017

Unit test failures need investigating. The HTTP/1.1 being repeated in the status code definitely looks unintended?

@wilsonge

This comment has been minimized.

Show comment
Hide comment
@wilsonge

wilsonge May 2, 2017

Contributor

anibalsanchez#1 Adds a unit test for the bug fix being made

Contributor

wilsonge commented May 2, 2017

anibalsanchez#1 Adds a unit test for the bug fix being made

@wilsonge

This comment has been minimized.

Show comment
Hide comment
@wilsonge

wilsonge May 5, 2017

Contributor

@anibalsanchez you need to look at the unit test fails here

Contributor

wilsonge commented May 5, 2017

@anibalsanchez you need to look at the unit test fails here

@wilsonge wilsonge added this to the Joomla 3.7.1 milestone May 5, 2017

wilsonge added some commits May 5, 2017

@anibalsanchez

This comment has been minimized.

Show comment
Hide comment
@anibalsanchez

anibalsanchez May 5, 2017

Contributor

Hi @wilsonge , not sure why the unit test does not work. The change is a simple call to a different API to generate the same headers... so it shouldn't generate an invalid header.

Contributor

anibalsanchez commented May 5, 2017

Hi @wilsonge , not sure why the unit test does not work. The change is a simple call to a different API to generate the same headers... so it shouldn't generate an invalid header.

@anibalsanchez

This comment has been minimized.

Show comment
Hide comment
@anibalsanchez

anibalsanchez May 6, 2017

Contributor

Hi @wilsonge ,

I've just fixed the issues on the unit tests. Some notes:

  • 837d100 The translation of the 'status' to HTTP/1.1 was not sending the parameters required by JApplicationCmsTest/testRedirect.

  • 0c7acc6 In JApplicationWebTest/testRedirectWithExistingStatusCode, setHeader('status', 201) can also be setHeader('Status', 201), since setHeader is case-sensitive. So, the case difference produces different tests results. I have created testRedirectWithExistingStatusCode1 and testRedirectWithExistingStatusCode2 to check the results of status and Status.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/15738.
Contributor

anibalsanchez commented May 6, 2017

Hi @wilsonge ,

I've just fixed the issues on the unit tests. Some notes:

  • 837d100 The translation of the 'status' to HTTP/1.1 was not sending the parameters required by JApplicationCmsTest/testRedirect.

  • 0c7acc6 In JApplicationWebTest/testRedirectWithExistingStatusCode, setHeader('status', 201) can also be setHeader('Status', 201), since setHeader is case-sensitive. So, the case difference produces different tests results. I have created testRedirectWithExistingStatusCode1 and testRedirectWithExistingStatusCode2 to check the results of status and Status.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/15738.

@zero-24 zero-24 modified the milestones: Joomla 3.7.1, Joomla 3.7.2 May 12, 2017

@rdeutz

This comment has been minimized.

Show comment
Hide comment
@rdeutz

rdeutz May 14, 2017

Contributor

I have tested this item successfully on f826300


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/15738.

Contributor

rdeutz commented May 14, 2017

I have tested this item successfully on f826300


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/15738.

@@ -719,7 +719,7 @@ public function sendHeaders()
if ('status' == strtolower($header['name']))
{
// 'status' headers indicate an HTTP status, and need to be handled slightly differently
- $this->header('HTTP/1.1 ' . $header['value'], null, (int) $header['value']);

This comment has been minimized.

@wilsonge

wilsonge May 14, 2017

Contributor

Shouldn't the code param ((int) $header['value'] be preserved here?)

@wilsonge

wilsonge May 14, 2017

Contributor

Shouldn't the code param ((int) $header['value'] be preserved here?)

This comment has been minimized.

@rdeutz

rdeutz May 14, 2017

Contributor

agree (int) $header['value'] should be added

@rdeutz

rdeutz May 14, 2017

Contributor

agree (int) $header['value'] should be added

@@ -565,8 +565,8 @@ public function redirect($url, $status = 303)
}
// All other cases use the more efficient HTTP header for redirection.
- $this->header($this->responseMap[$status]);
- $this->header('Location: ' . $url);
+ $this->setHeader('Status', $status, true);

This comment has been minimized.

@zero-24

zero-24 May 14, 2017

Contributor

What about using $this->setHeader('Status', $this->responseMap[$status], true); than we would not need to change the unit tests.

@zero-24

zero-24 May 14, 2017

Contributor

What about using $this->setHeader('Status', $this->responseMap[$status], true); than we would not need to change the unit tests.

This comment has been minimized.

@zero-24

zero-24 May 14, 2017

Contributor

hmm i don't get it can you explain it more? As this is technical a B/C break what we are doing here now.

@zero-24

zero-24 May 14, 2017

Contributor

hmm i don't get it can you explain it more? As this is technical a B/C break what we are doing here now.

This comment has been minimized.

@wilsonge

wilsonge May 14, 2017

Contributor

Look at the output of the test on this line https://travis-ci.org/joomla/joomla-cms/jobs/227941705#L1050 Notice how HTTP 1.1 is duplicated

@wilsonge

wilsonge May 14, 2017

Contributor

Look at the output of the test on this line https://travis-ci.org/joomla/joomla-cms/jobs/227941705#L1050 Notice how HTTP 1.1 is duplicated

This comment has been minimized.

@zero-24

zero-24 May 15, 2017

Contributor

Is'nt than a str_replace a way to avoid the B/C break? Like replace HTTP/1.1 HTTP/1.1 with HTTP/1.1 and a deprecation notice for 4.0 which handle it different (e.g. remove the http/1.1 from here: https://github.com/joomla/joomla-cms/blob/staging/libraries/joomla/application/web.php#L85) or similiar)

@zero-24

zero-24 May 15, 2017

Contributor

Is'nt than a str_replace a way to avoid the B/C break? Like replace HTTP/1.1 HTTP/1.1 with HTTP/1.1 and a deprecation notice for 4.0 which handle it different (e.g. remove the http/1.1 from here: https://github.com/joomla/joomla-cms/blob/staging/libraries/joomla/application/web.php#L85) or similiar)

This comment has been minimized.

@anibalsanchez

anibalsanchez May 15, 2017

Contributor

I've just added the cast to int.

@anibalsanchez

anibalsanchez May 15, 2017

Contributor

I've just added the cast to int.

@rdeutz

This comment has been minimized.

Show comment
Hide comment
@rdeutz

rdeutz May 15, 2017

Contributor

Would be good to have more tests, if someone needs a component, this one http://babioon.com/en/component/ars/repository/babioon-event/babioon-event-3-0-0/babioon-event-3-0-0.html fails

Contributor

rdeutz commented May 15, 2017

Would be good to have more tests, if someone needs a component, this one http://babioon.com/en/component/ars/repository/babioon-event/babioon-event-3-0-0/babioon-event-3-0-0.html fails

@brianteeman

This comment has been minimized.

Show comment
Hide comment
@brianteeman

brianteeman May 15, 2017

Contributor

Tested using the component from @rdeutz
Before the PR saving an existing item gave a white page
After the PR the article saved as expected

Contributor

brianteeman commented May 15, 2017

Tested using the component from @rdeutz
Before the PR saving an existing item gave a white page
After the PR the article saved as expected

@brianteeman

This comment has been minimized.

Show comment
Hide comment
@brianteeman

brianteeman May 15, 2017

Contributor

I have tested this item successfully on f6fe521


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/15738.

Contributor

brianteeman commented May 15, 2017

I have tested this item successfully on f6fe521


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/15738.

@rdeutz rdeutz modified the milestones: Joomla 3.7.1, Joomla 3.7.2 May 15, 2017

@rdeutz rdeutz merged commit 6dfc4d9 into joomla:staging May 15, 2017

3 of 4 checks passed

JTracker/HumanTestResults Human Test Results: 1 Successful 0 Failed.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/drone/pr the build was successful
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rdeutz rdeutz removed their assignment May 15, 2017

brianteeman added a commit to brianteeman/joomla-cms that referenced this pull request May 16, 2017

Redirection fails on multiple status values #15737 (#15738)
* Redirection fails on multiple status values #15737

* Add test for issue #15738

* Status should just be a number

* Some test updates

* More fixes

* Correction to header status params to comply with the unit test testRedirect

* testRedirectWithExistingStatusCode1 and testRedirectWithExistingStatusCode2

* testRedirectWithExistingStatusCode2

* 'HTTP/1.1 ' . (int) $header['value']

@wilsonge wilsonge referenced this pull request in joomla-framework/application May 21, 2017

Merged

Port fixes to the redirect method from the CMS #72

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