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

AddPathPlugin > Trim ending slash on path #128

Merged
merged 1 commit into from Dec 14, 2018
Merged

AddPathPlugin > Trim ending slash on path #128

merged 1 commit into from Dec 14, 2018

Conversation

ruudk
Copy link
Contributor

@ruudk ruudk commented Dec 11, 2018

Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
License MIT

What's in this PR?

Instead of throwing new \LogicException('URI path cannot end with a slash.'); when the URI path ends with a slash, this PR will rtrim it automatically.

Why?

Today was a day that we merged a PR that had successful CI builds but broke during deployment. The reason? The parameters set on the server ended with a /.

I don't really understand the reason for throwing exceptions this hard, while they can be recovered. Every time somebody converts a Guzzle client to HTTPlug I have to warn them that we first need to change the URI's everywhere to remove the ending slashes. And sometimes that breaks for no reason.

Checklist

  • Updated CHANGELOG.md

@ruudk
Copy link
Contributor Author

ruudk commented Dec 11, 2018

Pretty CI is failing because of:

Fatal error: Uncaught Error: Class 'SLLH\StyleCIBridge\ConfigBridge' not found in /tmp/code/.php_cs:13

Copy link
Contributor

@dbu dbu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i see. there are places where being less strict is a good thing. i am generally in favor of being strict because mistakes can indicate errors that we rather know about than silently do something unexpected. in this case however, i think we are indeed overly strict and see little harm in the rtrim. @soullivaneuh you originally contributed this plugin. do you see an issue with this change?

@gmponos
Copy link
Contributor

gmponos commented Dec 12, 2018

I think I have seen some discussions going around in symfony related to trailing slashes: https://github.com/symfony/symfony/issues?utf8=%E2%9C%93&q=trailing

not sure if this could help get the importance of this change or if the discussions are unrelated with the change here...

@dbu dbu merged commit b94e9bb into php-http:master Dec 14, 2018
@dbu
Copy link
Contributor

dbu commented Dec 14, 2018

thanks. if symfony routing decides to ignore trailing slashes, we can too :-)

i tagged 1.8.2 with this and also merged master to 2.x so we will have this in the new major version.

@ruudk ruudk deleted the remove-ending-slash branch December 14, 2018 22:35
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 this pull request may close these issues.

None yet

3 participants