Skip to content
This repository was archived by the owner on Jan 30, 2020. It is now read-only.

Handle 100 continue responses proposal #131

Closed
wants to merge 2 commits into from

Conversation

t1gor
Copy link

@t1gor t1gor commented Oct 10, 2017

Attempt to fix a 100 Continue responses issue.

@t1gor t1gor mentioned this pull request Oct 10, 2017
@t1gor
Copy link
Author

t1gor commented Oct 11, 2017

The build fails with an error irrelent to the change:

Parse error: syntax error, unexpected ':', expecting ';' or '{' in /home/travis/build/zendframework/zend-http/vendor/doctrine/instantiator/src/Doctrine/Instantiator/Instantiator.php on line 95

src/Response.php Outdated
} catch (RuntimeException $e) {
array_shift($lines); // skip next empty line
$firstLine = array_shift($lines); // and try again
$response->parseStatusLine($firstLine);
Copy link
Member

Choose a reason for hiding this comment

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

Make sure next line is empty before skipping.
On a quick glance at the specification I do not see anything preventing sending more headers with 100 status code before proceeding with actual response.

Also your extracted method does too much. Extract status line parsing logic but leave the rest out of it.
I would do something like this:

list($version, $status, $reason) = $this->parseStatusLine($firstLine);
if (static::STATUS_CODE_100 === $status) {
    $next = array_shift($lines);
    if (!empty($next)) {
        // throw exception, probably
    }
    $next = array_shift($lines);
    list($version, $status, $reason) = $this->parseStatusLine($next);
}

$response->version = $version;
$response->setStatusCode($status);
$response->setReasonPhrase($reason);

Copy link
Author

Choose a reason for hiding this comment

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

Changed to work without exceptions. Rebased and fixed tests.

@Xerkus Xerkus changed the base branch from master to develop October 30, 2017 09:38
Copy link
Member

@Xerkus Xerkus left a comment

Choose a reason for hiding this comment

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

Please revert all changes to tests.
They are using deprecated and removed methods.

weierophinney added a commit that referenced this pull request Apr 26, 2018
weierophinney added a commit that referenced this pull request Apr 26, 2018
@weierophinney
Copy link
Member

I have cherry-picked the relevant commits, ensuring the changes to tests are not propagated. Merged to master for next release.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants