-
Notifications
You must be signed in to change notification settings - Fork 62
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
Executing multiple PUT requests makes curl emit warnings #99
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any chance to get a unit test covering the change?
Codecov Report
@@ Coverage Diff @@
## master #99 +/- ##
============================================
- Coverage 89% 88.83% -0.18%
Complexity 244 244
============================================
Files 15 15
Lines 782 779 -3
============================================
- Hits 696 692 -4
- Misses 86 87 +1
Continue to review full report at Codecov.
|
I've added a test and removed some previously needed workarounds that were required because CURL_OPTs were persisted between requests. With the introduction of curl_reset those problems are fixed. Is it worthwhile to also open a PR for the latest stable that fixes this problem? As I've currently got this patched locally to even have my code working |
Hmm I guess you accidentally removed the curl_reset() call? We cannot add the same patch on the latest stable release, because our 4.2 release supports php 5.4 and curl_reset() was added in 5.5 |
This fixes sabre-io#89 Executing multiple PUT requests makes curl emit warnings Since the php 7.0 is now required it is save to use curl_reset to fix previous issues with CURL_OPTS that were persisted between requests
I tried a different approach before as I did not notice that PHP 7 was now required so I removed the reset. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really like it! Lets see what others think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
As reported in issue #89
See also this related stack overflow article