cURL instrumentation: support partially successful curl_setopt_array#316
Conversation
|
Thanks for opening your first pull request! If you haven't yet signed our Contributor License Agreement (CLA), then please do so that we can accept your contribution. A link should appear shortly in this PR if you have not already signed one. |
The committers listed above are authorized under a signed CLA. |
| if (curl_error($params[0])) { | ||
| foreach ($params[1] as $option => $value) { | ||
| if (!curl_setopt($params[0], $option, $value)) { | ||
| break; |
There was a problem hiding this comment.
If we continue here instead, would it permit setting the subsequent options too? (in case there is more than one error)
There was a problem hiding this comment.
yes it would, however I dont see a reason to do that since I think we should only be interested in recording options which would be used later (in the following curl handle execution).
trying to record all valid options might be useful (and could be done without continue too I think), but that is outside of scope of this PR.
There was a problem hiding this comment.
I think I agree with @vikmovcan here. We shouldn't set the options if curl itself wouldn't - so if curl_setopt_array wouldn't set them, neither should we.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #316 +/- ##
============================================
- Coverage 83.14% 79.60% -3.54%
- Complexity 669 729 +60
============================================
Files 61 69 +8
Lines 2676 2952 +276
============================================
+ Hits 2225 2350 +125
- Misses 451 602 +151
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 8 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
vikmovcan
changed the title
intuibase
left a comment
intuibase
left a comment
There was a problem hiding this comment.
Looks good, thanks for catching that.
|
released v0.0.2 |
4 participants
When
curl_setopt_arrayis partially successful we end up not recording curl options which were set successfully.In order to work around this, the fix tries to emulate the behaviour of how
curl_setopt_arrayworks, and iterates over the passed options usingcurl_opt, recording each valid one, until an invalid option is encountered (reference: https://github.com/php/php-src/blob/fb257ee83c405fecf449571bfcd1cc0fb4910336/ext/curl/interface.c#L2381).