Skip to content

Conversation

@clue
Copy link
Member

@clue clue commented Jul 4, 2017

This means we no longer have to maintain a list of HTTP reason phrases
ourselves, but instead rely on those already present in our HTTP message
abstraction.

This means we no longer have to maintain a list of HTTP reason phrases
ourselves, but instead rely on those already present in our HTTP message
abstraction.
@clue clue added this to the v0.7.2 milestone Jul 4, 2017
@WyriHaximus WyriHaximus requested review from WyriHaximus and jsor July 4, 2017 09:40
$body->seek(0, SEEK_END);
$body->write(': ' . $reason);
}

Copy link
Member

Choose a reason for hiding this comment

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

Any particular reason you don't construct $message as before?

$message = 'Error ' . $code;

$reason = $response->getReasonPhrase();
if ($reason !== '') {
    $message .= ': ' . $reason
}

Copy link
Member Author

Choose a reason for hiding this comment

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

The $message is used only as a constructor parameter for the response body and the default reason phrase is only available after calling the constructor, hence this assignment.

Copy link
Member

Choose a reason for hiding this comment

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

Oh yes, missed that 😊

$reason = $response->getReasonPhrase();
if ($reason !== '') {
$body = $response->getBody();
$body->seek(0, SEEK_END);
Copy link

Choose a reason for hiding this comment

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

Why does it need that seek?

Copy link
Member Author

Choose a reason for hiding this comment

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

This makes sure it seeks to the end so that we append the reason phrase to the existing body.

@WyriHaximus WyriHaximus merged commit 2dda18c into reactphp:master Jul 4, 2017
@clue clue deleted the reasons branch July 4, 2017 12:22
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.

4 participants