Only include the port number in the Host header when it differs from default

[dd32]

Based on @amandato in https://core.trac.wordpress.org/ticket/37991

[codecov-io]

Current coverage is 92.22% (diff: 100%)

Merging #238 into master will not change coverage

@@             master       #238   diff @@
==========================================
  Files            21         21          
  Lines          1762       1762          
  Methods         156        156          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits           1625       1625          
  Misses          137        137          
  Partials          0          0          

Powered by Codecov. Last update fb5b517...9b402cb

[angelomandato]

Thanks for applying my suggestion. One detail that was missed is that you also need to check if the scheme is present. It is possible that the scheme was not specified and it will not be set. either !empty($url_parts['scheme']) or isset($url_parts['scheme']) is necessary here.

The current logic if the scheme was foo://example.com:81 it will not set the port in the "Hosts: example.com".

logic currently in place that fails this foo://example.com:81/ ::

            if (( 'http' === strtolower($url_parts['scheme']) && $url_parts['port'] !== 80 ) || ( 'https' === strtolower($url_parts['scheme']) && $url_parts['port'] !== 443 )) {
                $out .= ':' . $url_parts['port'];
            }

Logic that passes foo://example.com:81/ ::

        if( !empty($url_parts['scheme']) && strtolower($url_parts['scheme']) === 'http' && $url_parts['port'] !== 80 ) {
            $out .= ':' . $url_parts['port']; // Handle custom http scheme port
        }
        else if( !empty($url_parts['scheme']) && strtolower($url_parts['scheme']) === 'https' && $url_parts['port'] !== 443 ) {
            $out .= ':' . $url_parts['port']; // Handle custom https scheme port
        }
        else if( empty($url_parts['scheme']) || (strtolower($url_parts['scheme']) !== 'http' && strtolower( $url_parts['scheme']) !== 'https') ) {
            $out .= ':' . $url_parts['port']; // specify port when scheme is unknown
        }

Note this is a slight change to my original patch.

fsockopen.php.patch.txt

[angelomandato]

Sorry my last reply did not address the situation where the scheme was not specified.

Please see this patch for a better example how to write this logic, it catches all cases, if the protocol is specified, not specified and if a custom port is set or not.

Update added strtolower() in switch line to handle when scheme may have capital letters.

fsockopen.php.patch.txt

[angelomandato]

Here are some test cases:

http://example.com/ (Host: example.com)
http://example.com:443/ (Host: example.com:443)
http://example.com:123/ (Host: example.com:123)
https://example.com/ (Host: example.com)
https://example.com:80/ (Host: example.com:80)
https://example.com:400/ (Host: example.com:400)
foo://example.com/ (Host: example.com)
foo://example.com:443/ (Host: example.com:443)
foo://example.com:999/ (Host: example.com:999)
//example.com/  (Host: example.com)
//example.com:443/ (Host: example.com:443) - this one is up for comment if port should be specified if SSL is used. I believe the port should be specified, we cannot predict why a custom/blank scheme would be using 443.
//example.com:999/ (Host: example.com:999)

[dd32]

@amandato Thanks for expanding on your concerns.
Neither of them are an issue however, Requests both normalises and rejects incomplete URLs, a provided url MUST have a scheme and it MUST match either http:// or https:// to be able to reach this section of code.

Thanks for the example test cases too, I'm yet to actually write any Requests unit tests, but I'll see if I can work out how to test these.

[rmccue]

Based on RFC 7230, it seems like we shouldn't do this at all?

If the target URI includes an authority component, then a
client MUST send a field-value for Host that is identical to that
authority component, excluding any userinfo subcomponent and its "@"
delimiter (Section 2.7.1).

That said, seems like a no-brainer to do it if we're already doing 80.

[angelomandato]

Awesome!

Yes that same RFC states "If the connection's incoming TCP port number
differs from the default port for the effective request URI's
scheme, ...". Since https default port is 443, you should not specify it.

On the server side, when the port is included the $_SERVER['HTTP_HOST'] then includes the port, which can confuse some PHP code for certain applications that do not expect the port for HTTP or HTTPS when 80/443 is used. There are special cases, such as https on port 80 and http on port 443, then the ports should be specified.

I understand that it is a good idea NOT to support other scheme's. The drawback though your library does not support those URLs where the scheme still uses port 80 as default, such as feed:// or podcast://. podcast:// does not have a RFC though, another reason not to directly support it. But there are other scheme's such as feed:// that have been proposed they are just almost never used. For your reference: http://www.iana.org/assignments/uri-schemes/uri-schemes.xhtml I would keep things the way they are, just providing additional info.

Thanks for a great library!