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

Fix for #993, added ClientBuilder::includePortInHostHeader() #997

Merged
merged 4 commits into from
Feb 14, 2020
Merged

Fix for #993, added ClientBuilder::includePortInHostHeader() #997

merged 4 commits into from
Feb 14, 2020

Conversation

ezimuel
Copy link
Contributor

@ezimuel ezimuel commented Feb 14, 2020

This PR fixes #993 adding a ClientBuilder::includePortInHostHeader(bool $enable). Using this option you can include the port in the Host header.

For instance, this configuration will build the header Host: localhost:9200.

$host = 'localhost:9200';
$client = ClientBuilder::create()
    ->setHosts([$host])
    ->includePortInHostHeader(true)
    ->build();

By default (if you don't call includePortInHostHeader(true)), the port is not included for backward compatibility.

@ezimuel
Copy link
Contributor Author

ezimuel commented Feb 14, 2020

@larsnystrom may you review this PR? Thanks!

Copy link

@larsnystrom larsnystrom left a comment

Choose a reason for hiding this comment

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

I have tried this in the application I'm working on and it works! I made a couple of comments concerning how its implemented, but the change as it's implemented works for me now. 👍

@@ -168,6 +168,11 @@ public function __construct(
}
$port = $hostDetails['port'];

if (isset($connectionParams['client']['port_in_header']) && $connectionParams['client']['port_in_header']) {
if (!in_array((int) $port, [80,443])) {

Choose a reason for hiding this comment

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

I think it should be possible to have port 80 or 443 in the header as well. It is perfectly acceptable according to spec, so I don't see a need for special treatment.

Choose a reason for hiding this comment

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

Including the port is ok according to spec, and those who do not want the port in the Host header can just set the newly introduced option to false to disable it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the default ports because it's a common standard in many HTTP client. Moreover, I suspect this will fix also #782.

Choose a reason for hiding this comment

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

I don't feel super strongly about this, but my reasoning goes like this:

  1. The elasticsearch client adds the port to the Host header, as is allowed by the HTTP spec.
  2. Somebody will encounter [Header] Making the port optional in the header "Host" #548, for which the easy solution is to not append the port to the Host header (they could also fix the broken proxy server, but lets be realistic).
  3. They can accomplish this with includePortInHostHeader(false).

So this PR solves #782 even if you don't exclude port 80 and 443.

Anyway, you decide :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤔 Ok, you convinced me. I'll remove this standard port check from the PR. I'll investigate more on that for the next major version of elasticsearch-php. Thanks again for your precious feedback.

@@ -168,6 +168,11 @@ public function __construct(
}
$port = $hostDetails['port'];

if (isset($connectionParams['client']['port_in_header']) && $connectionParams['client']['port_in_header']) {
if (!in_array((int) $port, [80,443])) {
$host .= ":$port";

Choose a reason for hiding this comment

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

Instead of appending the port here in the constructor, I think you should append when a request is made in performRequest(). The setting is called include_port_in_host_header, so it makes sense to only include it in the header, and the header is only constructed in performRequest

Choose a reason for hiding this comment

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

(If you do it in performRequest() instead it is possible to get just the host part when you call getHost(). Those that need to get the port as well have a getPort() method for that. But the way its implemented right now makes it impossible to get the original host part back from the Connection object.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very good point! I'm refactoring it as you suggested. Thanks!

@ezimuel ezimuel merged commit e8f0f7b into elastic:master Feb 14, 2020
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.

Port is not properly set in requests, causing No alive nodes found in your cluster
2 participants