RedirectPlugin: Default to empty path when Location doesn't specify any #218
Conversation
ostrolucky
commented
Sep 10, 2022
ostrolucky
commented
| Q | A |
|---|---|
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Related tickets | fixes #217 |
| Documentation | |
| License | MIT |
| @@ -244,22 +249,6 @@ private function createUri(ResponseInterface $redirectResponse, RequestInterface | |||
| $uri = $uri->withPort($parsedLocation['port']); | |||
There was a problem hiding this comment.
i am actually not sure about this one either. if i was on http://localhost:8080/foo and i get redirect a header https://php-http.org/bar i would not expect to stay on port 8080... the only case where the port should be kept is when there is also no host in the redirection... wdyt?
There was a problem hiding this comment.
I agree. Will change
|
thanks for looking into this! i think you are right that we need to also reset the path if none is provided, and i like how you compact the code by chaining the calls. can you please check the test failure? is this some mocking going wrong? could we use an actual URI implementation instead of mocking, if thats the cause for the issue? |
|
Wow those phpspec tests are insane. Can I just delete those phpspec tests I have trouble with, since I replaced it with integration-like new phpunit test? Otherwise it would take me entire day to fix it, as phpspcec is makint it very difficult to find what needs to be changed. Edit: I'll try to give it more chance and perhaps replace some parts of mocks with real stuff like you suggested first |
|
i guess the problem is that the Uri object is mocked instead of using an actual implementation, and it is missing mocking those calls. but i agree to remove the failing cases in favor of the phpunit tests, i also find them a lot more readable. please add a phpdoc to the top of the RedirectPluginSpec that mentions the phpunit test and vice versa. |
|
@ostrolucky do you remove the failing phpspec tests and also fix the thing about the port? |
|
Yes I just didn't have time to continue since my last struggle, but I have unstaged changes |
|
Should be addressed now. |
ostrolucky
force-pushed
the
fix-redirect-plugin-path-handling
branch
2 times, most recently
from
860464a to
5bb0f5a
Compare
|
i fixed the build issue in #220, can you please rebase this branch on master? i don't expect problems with only one symfony version, but it just feels too yolo to merge a PR with failing builds :-) |
ostrolucky
force-pushed
the
fix-redirect-plugin-path-handling
branch
from
5bb0f5a to
c06f154
Compare
|
rebased |
|
yay, thanks! |
