php-http / client-common

Common HTTP Client implementations and tools for HTTPlug
http://httplug.io
MIT License
1.01k stars 53 forks source link

RedirectPlugin: Default to empty path when Location doesn't specify any #218

Closed ostrolucky closed 1 year ago

ostrolucky commented 1 year ago
Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Related tickets fixes #217
Documentation
License MIT
dbu commented 1 year ago

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?

ostrolucky commented 1 year ago

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

dbu commented 1 year ago

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.

dbu commented 1 year ago

@ostrolucky do you remove the failing phpspec tests and also fix the thing about the port?

ostrolucky commented 1 year ago

Yes I just didn't have time to continue since my last struggle, but I have unstaged changes

ostrolucky commented 1 year ago

Should be addressed now.

dbu commented 1 year ago

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 commented 1 year ago

rebased

dbu commented 1 year ago

yay, thanks!