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

Possible bug in UriResolver::resovle #433

Closed
Ahummeling opened this issue Aug 30, 2021 · 3 comments · Fixed by #439
Closed

Possible bug in UriResolver::resovle #433

Ahummeling opened this issue Aug 30, 2021 · 3 comments · Fixed by #439

Comments

@Ahummeling
Copy link

Ahummeling commented Aug 30, 2021

Note: I am writing in the context of the UriResolver

PHP version:7.3.20
guzzlehttp/psr7 version:1.8.2

Description
Given a $base with path: "/api/v1" (I.e. a base path with > 1 segments and no trailing slash). And a $rel with path: "screening-profile", this function (UriResolver::resolve) ends up with a $targetPath of /api/screening-profile. (So the /v1 segment is lost). If this is expected behavior, sorry for bothering, perhaps you can point me to where I could have found this out myself, or somewhere I can contribute to documentation.

How to reproduce
Instantiate a guzzle client with a base_uri that doesn't end in a '/' and make a request to some path that does not start with a '/'.

Possible Solution
storing $base->getPath() in a local variable, appending a '/' to this if it not already there, seems to create the expected behavior. I'm happy to submit a pull request if desired.

Additional context
If a pull request is desired, perhaps I could be pointed to any relevant code / tests / documentation that should be updated as well.
A "workaround" is very simple. Simply make sure to end the base_uri in a trailing '/'

PR Adding Testcase
#439

@Nyholm
Copy link
Member

Nyholm commented Sep 5, 2021

Thank you for a great issue.

I am pretty sure this is the intended behavior. See base_uri in the quickstart guide. There is a table explaining how how different inputs are handled.

Is there a test case for your exact scenario?

@GrahamCampbell
Copy link
Member

Having just read-read this I think @Nyholm is right that this is intended.

@Ahummeling
Copy link
Author

Thanks for pointing me to the documentation.
I've gone over the existing test cases and I believe that my exact scenario was not yet tested directly, so I've added the relevant testcase and submitted a pull request.

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 a pull request may close this issue.

3 participants