Add Connection:close header only when needed

[mircobabini]

Pull Request Type

• I have checked there is no other PR open for the same change.

This is a:

• Bug fix
• New feature
• Code quality improvement

Context

Fixes #656.

Quality assurance

• This change does NOT contain a breaking change (fix or feature that would cause existing functionality to change).
• I have commented my code, particularly in hard-to-understand areas.
• I have added unit tests to accompany this PR.
• The (new/existing) tests cover this PR 100%.
• I have (manually) tested this code to the best of my abilities.
• My code follows the style guidelines of this project.

[jrfnl]

👋🏻 Hi Mirco, thanks for opening the issue and pull request regarding this.

Could a meaningful unit test be added which can proof the issue exists (without the fix) and can safeguard the fix once it is in place ?

[mircobabini]

This is complicated. Btw even if I was able to create a unit test to proof the issue exists, it would work on the described environment only.

Is it still useful?

[mircobabini]

@jrfnl can you please introduce me to unit tests on this? How do you expect this to be implemented?

[jrfnl]

@jrfnl can you please introduce me to unit tests on this? How do you expect this to be implemented?

@mircobabini I'm not sure what you are asking ?

• if you need to know how to write tests: see the PHPUnit manual
• if you need to know how to write a test for this library, this repo has plenty of tests in the tests directory, have a look there for inspiration.
• if you need to know how to write tests for this specific case - please tell us what you are struggling to define for the test/be more specific about what you need help with. My mind reading skills are a bit rusty ;-)

[jrfnl]

@mircobabini Do you still want to continue with this PR or should one of us take over ?

[mircobabini]

@jrfnl

Do you still want to continue with this PR or should one of us take over ?

Thanks for the heads up. I'm unable to replicate this bc I don't have a Kaspersky license anymore.
Btw I think this is something you can work around it with the provided informations and/or some Kaspersky license if you are albe to get one.

If you can/want to take over, please go ahead. Thanks.

[mircobabini]

@jrfnl is this in your radar sooner or later?

[jrfnl]

@jrfnl is this in your radar sooner or later?

@mircobabini Well, yes, it is, but it is kind of hard to verify if something is the right fix and actually fixes something if there is no reproduction scenario....

[jrfnl]

@mircobabini We've discussed this today and will merge the PR, but the parse error (and CS errors) in the code will need to be fixed first.

[mircobabini]

@mircobabini We've discussed this today and will merge the PR, but the parse error (and CS errors) in the code will need to be fixed first.

Nice! I'm sorry I wasn't able to provide tests and a reproduction scenario. But glad to now it will be merged.
Just committed the proposed changes.

[jrfnl]

Thanks @mircobabini ! The patch will be included in the 2.1.0 release and if there will be a 2.0.x release, we may backport it as well.

[jrfnl]

FYI: The patch was included in the Requests 2.0.8 release and reverted in Requests 2.0.9 due to it causing problems with Curl 7.29.0 (and possibly others). See #838

I'll re-open the original issue.

[mircobabini]

@jrfnl thanks for sharing this update.

Since this issue is:

• related to Kaspersky which is a security software
• some old CURL versions (probably not secure)
• not so common in general

I'd say it's to be considered as a rare-case backward compatibility patch.
Probably not something we want to deal with - and not something we want to cause main issues to others.