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

AddHostPlugin must also handle the port #29

Merged
merged 1 commit into from
Jul 26, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
# Change Log

## Unreleased

### Changed

- AddHostPlugin also sets the port if specified

## 1.2.0 - 2016-07-14

Expand Down
5 changes: 4 additions & 1 deletion spec/Plugin/AddHostPluginSpec.php
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,14 @@ function it_adds_domain(
) {
$host->getScheme()->shouldBeCalled()->willReturn('http://');
$host->getHost()->shouldBeCalled()->willReturn('example.com');
$host->getPort()->shouldBeCalled()->willReturn(8000);

$request->getUri()->shouldBeCalled()->willReturn($uri);
$request->withUri($uri)->shouldBeCalled()->willReturn($request);

$uri->withScheme('http://')->shouldBeCalled()->willReturn($uri);
$uri->withHost('example.com')->shouldBeCalled()->willReturn($uri);
$uri->withPort(8000)->shouldBeCalled()->willReturn($uri);
$uri->getHost()->shouldBeCalled()->willReturn('');

$this->beConstructedWith($host);
Expand All @@ -58,13 +60,14 @@ function it_replaces_domain(
) {
$host->getScheme()->shouldBeCalled()->willReturn('http://');
$host->getHost()->shouldBeCalled()->willReturn('example.com');
$host->getPort()->shouldBeCalled()->willReturn(8000);

$request->getUri()->shouldBeCalled()->willReturn($uri);
$request->withUri($uri)->shouldBeCalled()->willReturn($request);

$uri->withScheme('http://')->shouldBeCalled()->willReturn($uri);
$uri->withHost('example.com')->shouldBeCalled()->willReturn($uri);

$uri->withPort(8000)->shouldBeCalled()->willReturn($uri);

$this->beConstructedWith($host, ['replace' => true]);
$this->handleRequest($request, function () {}, function () {});
Expand Down
9 changes: 6 additions & 3 deletions src/Plugin/AddHostPlugin.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
use Symfony\Component\OptionsResolver\OptionsResolver;

/**
* Add schema and host to a request. Can be set to overwrite the schema and host if desired.
* Add schema, host and port to a request. Can be set to overwrite the schema and host if desired.
*
* @author Tobias Nyholm <[email protected]>
*/
Expand Down Expand Up @@ -52,8 +52,11 @@ public function __construct(UriInterface $host, array $config = [])
public function handleRequest(RequestInterface $request, callable $next, callable $first)
{
if ($this->replace || $request->getUri()->getHost() === '') {
$uri = $request->getUri()->withHost($this->host->getHost());
$uri = $uri->withScheme($this->host->getScheme());
$uri = $request->getUri()
->withHost($this->host->getHost())
->withScheme($this->host->getScheme())
->withPort($this->host->getPort())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

according to the PSR interface, getPort should return null if its the default port for this protocol, and withPort(null) must remove the port so that the default port is used. with a very poor uri implementation, this could lead to changing http://localhost/ to http://localhost:80/ which is a bit ugly but not wrong. i prefer that over complicated logic in here to detect the default port ourselves.

we must set the port to null if it is null, for the case of replace, as the original request could have specified a different port.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, we should assume that the PSR-7 implementation follows PSR7, even though it is weird at some places.

;

$request = $request->withUri($uri);
}
Expand Down