fix the edge case of double leading slash when rebuilding request #167
Conversation
| if (isset($server['HTTP_HOST'])) { | ||
| $pathinfo = $server['HTTP_HOST'].$pathinfo; | ||
| } else { | ||
| $pathinfo = 'localhost'.$pathinfo; |
There was a problem hiding this comment.
do we want to do this? and if we do, should we do the same for the case without RequestContext being set at all?
There was a problem hiding this comment.
I hope the question is not for me. I cannot comment on the code because I do not understand it well.
I try to keep away from anything that comes into contact with preparePathInfo() and prepareBaseUrl() methods of Request, because it's a black magic.
There was a problem hiding this comment.
this is more a question to @lsmith77
There was a problem hiding this comment.
I am not deep enough into this PR to really tell either .. @Tobion might know best in general as he knows the entire routing topic really well ..
|
Hi @dbu . I tried the fix and it works for me. |
| if ($this->context->getBaseUrl()) { | ||
| $uri = $this->context->getBaseUrl().$uri; | ||
| $pathinfo = $this->context->getBaseUrl().$pathinfo; |
There was a problem hiding this comment.
pathinfo is the path without the base url according to CGI spec. so the variable naming reuse is confusing.
|
The query string handling seems to be missing (which is not that important because it's not used by the routing itself AFAIK, but for consistency and extensebility). |
|
thanks a lot for the feedback. indeed i do not need all that header
stuff. i just pushed an updated version.
The query string handling seems to be missing
i don't understand what you mean here. we just pass around the pathinfo
that was given to us to match. is that without query parameters? or are
the query parameters only handled after the route has been matched?
should i simply append context->getQueryString() to the uri?
|
|
Yes, the pathinfo does not contain the query string. Just add the query string at the end. See https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpFoundation/Request.php#L1067 |
|
Otherwise looks good now 👍 |
|
i think contrary to other parts, the query parameters are actually used in the as we discussed yesterday, it would be really nice to have a stand alone Request class cleanly separated from HTTP header and other black magic, with a separate RequestBuilder to build request instances from HTTP. i hope for symfony 4 :-) |
fix the edge case of double leading slash when rebuilding request
| } | ||
| $uri = $this->context->getScheme().'://'.$host.$uri.$this->context->getQueryString(); |
There was a problem hiding this comment.
this does not work. it's missing the ? before the query string. this is why i linked the exmaple implementation https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpFoundation/Request.php#L1067
There was a problem hiding this comment.
fix #166
@sustmi you are right. as it turns out, even specifying the host explicitly,
Request::create('//test')considers the path as a domain. i now rebuild the full URL, i do hope this now works.note that you will need to do setContext with the RequestContext for this to work. if there is no request context, we don't do anything more than before the fix.