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

Duplicated port in URI #28

Closed
ondrejbouda opened this issue May 27, 2019 · 3 comments
Closed

Duplicated port in URI #28

ondrejbouda opened this issue May 27, 2019 · 3 comments

Comments

@ondrejbouda
Copy link

Hi!

I am getting wrong URI when running on localhost with custom port:

<?php declare(strict_types = 1);

require __DIR__ . '/../vendor/autoload.php';

use Nyholm\Psr7\Factory\Psr17Factory;
use Nyholm\Psr7Server\ServerRequestCreator;

$psr17Factory = new Psr17Factory();

$creator = new ServerRequestCreator(
    $psr17Factory,
    $psr17Factory,
    $psr17Factory,
    $psr17Factory
);

$request = $creator->fromGlobals();

echo $request->getUri()->__toString();

GET http://localhost:8000/

http://localhost:8000:8000/

The problem is probably due to mechanism used to create URI in ServerRequestCreator::createUriFromArray(). URI host gets populated with $_SERVER['HTTP_HOST'], but this contains also the port (see for example here: https://stackoverflow.com/a/12046836 ).

I propose two solutions:

  1. strip the port from $_SERVER['HTTP_HOST']
  2. use $_SERVER['SERVER_NAME'] instead - I am not sure about the consequences

I may create a pull request with fix if you want.

@Zegnat
Copy link
Collaborator

Zegnat commented May 27, 2019

Could you create a test for this to show it failing? From a quick reread of the code, we should already be handling ports as part of $_SERVER['HTTP_HOST']. Specifically, if HTTP_HOST ends in a port, we split it away from the hostname and set that port as the actual port to use:

if (1 === \preg_match('/^(.+)\:(\d+)$/', $server['HTTP_HOST'], $matches)) {
$uri = $uri->withHost($matches[1])->withPort($matches[2]);

@ondrejbouda
Copy link
Author

Great, it seems to be resolved by pull request #26 . I was using version 0.3.0 without this fix. I will use master for now.

@stroebjo
Copy link

Can you release a new Version with this fix? Had this issue in a project using your library.

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

No branches or pull requests

3 participants