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

Conversation

dbu
Copy link
Contributor

@dbu dbu commented Jul 26, 2016

Q A
Bug fix? yes
New feature? no
BC breaks? only if you relied on the bug
Deprecations? no
Related tickets -
Documentation -
License MIT

What's in this PR?

The AddHostPlugin did not look at the port and thus the port was either the default or in the case of overriding an existing host the port specified in the request.

Why?

It makes more sense to overwrite the port along with the domain and protocol.

Example Usage

$uriFactory = new \Http\Message\UriFactory\GuzzleUriFactory();
new AddHostPlugin($uriFactory->createUri('http://localhost:8000'));

Checklist

  • Updated CHANGELOG.md to describe BC breaks / deprecations | new feature | bugfix
  • Documentation pull request created (if not simply a bugfix): Document AddHostPlugin documentation#121 there is no doc atm.

$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.

@Nyholm
Copy link
Member

Nyholm commented Jul 26, 2016

Your example usage is invalid. The first parameter should be an URL interface.

One could also argue that this feature belongs in a AddPortPlugin. But I think this is good.

@dbu
Copy link
Contributor Author

dbu commented Jul 26, 2016

fixed the example.

if we would do an AddPortPlugin we would also need to split the scheme into an AddSchemePlugin to be consistent, and we could not distinguish between "not set" and "default port" so it would be very tricky.

imo scheme, host and port belong together in this situation.

build failure is the same as on master, only for lowest-version.

@Nyholm Nyholm merged commit fa291ea into master Jul 26, 2016
@dbu dbu deleted the bugfix/hostplugin-handle-port branch July 26, 2016 08:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants