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

Fix #1790 proxy should not end with / #1792

Closed
wants to merge 1 commit into from

Conversation

remicollet
Copy link

@remicollet remicollet commented Mar 30, 2017

@sgolemon can you please have a look, related to fix from https://bugs.php.net/74216

Perhaps better to ignore this ending / in php side to avoid BC

But, probably, fixing here make sense in all case

@remicollet
Copy link
Author

See php/php-src#2443

@remicollet
Copy link
Author

Can we get some feedback for project owner about the issue ? (ping @sagikazarmark)

RC time is only 2 weeks, and I'd like to see some consensus on this before 7.1.4

(this PR is only there to point where the failure is, perhaps not perfect, but discussion really needed)

@mtdowling
Copy link
Member

This seems reasonable to me. @sagikazarmark what do you think?

@remicollet
Copy link
Author

So, seems to be an agreement to not fix in PHP side, as bad data should not be accepted.

Question is: should we only fix the test, or also the library (add the trim) to be error tolerant.

@sagikazarmark
Copy link
Member

I see the problem and I think this is rather a potential quick fix, but not a solution to it.

I guess the question here is what a valid value is for a proxy. The slash at the end is clearly not, at least not for me. I would say allowing it is a bug, which should be fixed by either validating the value and throwing exceptions, or parsing it as a URI and silently reconstructing the value using only schema, host and port segments. Both sound risky from a BC break point of view, so maybe we should just merge this and properly fix in a follow up issue.

BTW this fix should be added as a 6.2.x patch (that's the current stable release line).

@sagikazarmark
Copy link
Member

@remicollet I am going to accept #1811 for now as it contains more PHP 7.2 related test fixes. For now I decided to solve it only in the tests and add a fix later if necessary.

Thanks for reporting the issue.

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