use HTTP_HOST instead of SERVER_NAME

[bAndie91]

Enchanté!

could you please consider changing SERVER_NAME to HTTP_HOST? SERVER_NAME usually contains internally used name (single-element hostname, or other address see https://tools.ietf.org/html/rfc3875#section-4.1.14), but here we need the user-facing hostname which is in her URL bar in the browser. IMO it's HTTP_HOST which is the most appropriative variable for this.

Merci.

[tontof]

I'm not sure, but I guess it can not be modified directly as proposed: HTTP_HOST: header from the current request, if there is one.
https://secure.php.net/manual/en/reserved.variables.server.php As it can be empty, the modification should take into account this, and if exists the value is different from .SERVER_NAME as it seems to contain the server port. https://stackoverflow.com/a/12046836/195835

Maybe something like this?

if (!isset($_SERVER["HTTP_HOST"]) && !isset($_SERVER["SERVER_NAME"])) {
  return $scriptname;
}
$host = $_SERVER["HTTP_HOST"];
if (empty($host)) {
  $host = $_SERVER["SERVER_NAME"] . $serverport ;
}
return 'http' . ($https ? 's' : '') . '://'. $host . $scriptname;

[bAndie91]

the port number – yes you are right, I forgot. we should consider port number in HTTP_HOST.

empty HTTP_HOST – i think no, I can not imagine a http request without HTTP_HOST. http clients, browsers always set Host header, and webserver passes it to php, if not, then there are much serious misconfiguration.

maybe remove SERVER_PORT from calculation and rely just on HTTP_HOST.

[tontof]

You mean something like this?

if (!isset($_SERVER["HTTP_HOST"])) {
  return $scriptname;
}
return 'http' . ($https ? 's' : '') . '://'. $_SERVER["HTTP_HOST"] . $scriptname;

Actually this code is inspired from shaarli and it also uses SERVER_NAME https://github.com/shaarli/Shaarli/blob/ff3b5dc5542ec150f0d9b447394364a15e9156d0/application/http/HttpUtils.php#L291 https://github.com/shaarli/Shaarli/blob/ff3b5dc5542ec150f0d9b447394364a15e9156d0/application/http/HttpUtils.php#L340 I guess it may be more useful to use its code as it also takes into account proxy.

[bAndie91]

yeah, detecting user-facing URL in server-side properly is a pretty tricky businness. IMO it's forward- and reverse proxies' responsibility to set request HTTP headers properly. Application developers only need to look up which header means this and that from the standards. give me some time to lookup the standards. Btw, do we really need the fully qualified URL at all?

[tontof]

I don't know exactly if it's linked to this problem https://github.com/tontof/kriss_feed/issues/241#issuecomment-23611458 but people with custom configuration had problem with current config so I added a BASE_URL global variable to solve this (which is a bad idea, but was working) Maybe you point out the original problem to solve. I don't need this as it works for me with all proposed code. With my new code inspired from zend-http I use HTTP_HOST first then SERVER_NAME then SERVER_ADDR https://github.com/KrISS/mvvm/blob/master/Kriss/Core/Request/Request.php#L170

[bAndie91]

sorry for the delay. yeah, defining a BASE_URL is an other common approach to let backend code know about the user-facing URL and let it construct right absolute paths and redirects. I'm Ok with adding BASE_URL.

in the code you referred, i noticed that Request->setHost takes HTTP_HOST first, then other sources, then Request->getHost is used only to get the hostname part and not the port number, but HTTP_HOST usually already contains the port number if it's not the standard 80/443.

[tontof]

No problem. Indeed, the port number is in HTTP_HOST but to be generic the Request->setHost does not consider port as it may not be defined if HTTP_HOST is not defined, as it will use SERVER_NAME or SERVER_ADDR instead.

[doc75]

I am trying to create a docker container with kriss_feed (with php-fpm behind a reverse proxy done with NGINX). Being behind a reverse proxy, SERVER_NAME is not set in my case, therefore HTTP_POST is interesting ;-)

@tontof and @bAndie91, what is the conclusion of your discussion on this PR ? Any chance to have it updated and merged ? I can help and push a PR, but I want before to be sure to understand your discussion. Thanks.

[bAndie91]

@doc75 hi, sorry I don't actively develop kriss_feed. we agreed on the design, and tontof added BASE_URL too, so you might set BASE_URL directly, or set SERVER_NAME to $http_host by factcgi_param if neccessary. cheers.

[tontof]

@doc75 you may propose a PR but I don't know exactly how it can be modified. I don't know exactly how docker works but I think @bAndie91 answers to your question.

[doc75]

@bAndie91, thanks a lot for your answer. Indeed the fastcgi_param is doing the trick (using the line: fastcgi_param SERVER_NAME $host;) @tontof, thanks but I will not push a PR as it works with the above solution ;-)

You might close this PR to avoid further question.