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

Add Connection:close header only when needed #657

Merged
merged 3 commits into from Aug 17, 2023

Conversation

mircobabini
Copy link
Contributor

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.

Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

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

👋🏻 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 ?

src/Transport/Curl.php Outdated Show resolved Hide resolved
@mircobabini
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@jrfnl
Copy link
Member

jrfnl commented Feb 27, 2022

@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
Copy link
Member

jrfnl commented Apr 24, 2023

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

@jrfnl jrfnl added this to the 2.1.0 milestone Apr 24, 2023
@mircobabini
Copy link
Contributor Author

mircobabini commented May 13, 2023

@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
Copy link
Contributor Author

@jrfnl is this in your radar sooner or later?

@jrfnl
Copy link
Member

jrfnl commented Aug 7, 2023

@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
Copy link
Member

jrfnl commented Aug 14, 2023

@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.

src/Transport/Curl.php Outdated Show resolved Hide resolved
@mircobabini
Copy link
Contributor Author

@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 jrfnl merged commit c7e62be into WordPress:develop Aug 17, 2023
17 checks passed
@jrfnl
Copy link
Member

jrfnl commented Aug 17, 2023

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 jrfnl mentioned this pull request Aug 17, 2023
6 tasks
@jrfnl jrfnl modified the milestones: 2.1.0, 2.0.x Next Sep 11, 2023
laszlof added a commit to laszlof/Requests that referenced this pull request Nov 8, 2023
@laszlof laszlof mentioned this pull request Nov 8, 2023
13 tasks
@jrfnl
Copy link
Member

jrfnl commented Nov 8, 2023

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
Copy link
Contributor Author

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cURL incompatibility with Kaspersky (kesl)
2 participants