guzzle / psr7

PSR-7 HTTP message library
MIT License
7.86k stars 2 forks source link

withPath overwrites current path #131

Closed gmponos closed 7 years ago

gmponos commented 7 years ago

I created a test case like the following:

<?php

require_once 'vendor/autoload.php';

$baseUrl = 'https://www.test.com/test';

/** @var \Psr\Http\Message\UriInterface $uri */
$uri = \GuzzleHttp\Psr7\uri_for($baseUrl);

echo $uri. "\n"; 
// Outputs: https://www.test.com/test
// Expecting: https://www.test.com/test
// Correct

echo $uri->withPath('/testme') . "\n"; 
// Outputs: https://www.test.com/testme
// Expecting: https://www.test.com/testme
// Correct

echo $uri->withPath('testme') . "\n";  
// Outputs: https://www.test.com/testme
// Expecting: https://www.test.com/test/testme
// Wrong?

Are the outputs above correct? I was expecting that without slash it will append the path.

According to the PHPDoc:

* If the path is intended to be domain-relative rather than path relative then
* it must begin with a slash ("/"). Paths not starting with a slash ("/")
* are assumed to be relative to some base path known to the application or
* consumer.

Maybe I am lost in translation or something but from the PHPDoc it's not clear to me if it should append it or not.

PS: same thing if the base URL is has a slash at the end like https://www.test.com/test/

Konafets commented 7 years ago

Are your output comment correct? I don't see a difference between those three.

prisis commented 7 years ago

Sorry, I misunderstood the question..

gmponos commented 7 years ago

@Konafets I have updated the description. Also I have written what I was expecting.

@prisis I am not sure I understood you.

sagikazarmark commented 7 years ago

To be honest the PSR-7 standard's wording is quite vague in some cases. In this specific case I don't see anything though that would suggest the behaviour you are expecting, at least not within the message representation layer.

In fact, the PSR states who should be responsible for this behaviour:

base path known to the application or consumer.

Maybe you could use the UriResolver for this task bundled with this library.

Tobion commented 7 years ago

The current behavior is correct and according to RFC 3986. The same logic applies to resolving relative links in websites. If you want https://www.test.com/test/testme as resulting URI, your base URI needs to be https://www.test.com/test/.

gmponos commented 7 years ago

@Tobion I am lost. You said that:

If you want https://www.test.com/test/testme as resulting URI, your base URI needs to be https://www.test.com/test/.

but in my description I wrote:

$baseUrl = 'https://www.test.com/test';
echo $uri->withPath('testme') . "\n";  
// Outputs: https://www.test.com/testme
// Expecting: https://www.test.com/test/testme
// Wrong?

If you meant that the problem is that in my example it misses the trailing slash I also wrote:

PS: same thing if the base URL is has a slash at the end like https://www.test.com/test/

Tobion commented 7 years ago

Sorry I was confused myself. What is correct is:

(new Uri('https://www.test.com/test')->withPath('testme') is not valid anymore since #94 (but not tagged yet) and will result in an exception. Because when an authority is present the path must start with a slash.

Previously this indeed magically added the leading slash as you described. But with some errata discussion on PSR7 this was changed.

What you are trying to do, is resolving a relative URI against a base URI. But this is not part of PSR7 (so not built into UriInterface). For this you can use UriResolver::resolve(). See https://github.com/guzzle/psr7/blob/master/src/UriResolver.php#L62 Then my above reasoning applies. You need a trailing slash if you want to have a path /test/testme after resolving, e.g. UriResolver::resolve(new Uri('https://www.test.com/test/'), new Uri('testme')) === 'https://www.test.com/test/testme'