Skip to content

fix curl error handler#97

Merged
nd4p90x merged 3 commits intosegmentio:masterfrom
krichprollsch:curl-error
Feb 2, 2021
Merged

fix curl error handler#97
nd4p90x merged 3 commits intosegmentio:masterfrom
krichprollsch:curl-error

Conversation

@krichprollsch
Copy link
Contributor

The method Consumer::handleError waits an error code and a string (see https://github.com/segmentio/analytics-php/blob/master/lib/Segment/Consumer.php#L93)

This PR fix the bad parameters call and return a real string content as error instead of the curl resource.
Moreover I removed the Consumer::executePost which was public (so it's a BC break) to use a loop for retry.

I also introduced a curl_error check to handle specific curl errors.

@f2prateek
Copy link
Contributor

hey @krichprollsch thans for the PR! It's not immediately clear to me what the issue was in the old codebase and how this addresses it - could you describe the goal in more detail?

@krichprollsch
Copy link
Contributor Author

krichprollsch commented Jan 22, 2018

The problems was in the way the handleError method was called before, here and here:

$this->handleError($ch, $httpResponse);
  • the order of the parameters is bad, the first parameter must be the error code and the second the message according to the method definition here
  • $ch is not a string, but a libcurl resource. I guess the original idea was to return the body of the request. You can get the body returned by curl_exec [1] (And I forgot to set the option CURLOPT_RETURNTRANSFER to curl, I will add it CURLOPT_RETURNTRANSFER is already set here).

To address the second point, I need to get the response code and the body of the request. The method executePost can't do that. So I refacto flushBatch to replace it by a loop with a retry in case of error.

}

$httpResponse = curl_getinfo($ch, CURLINFO_HTTP_CODE);
if ($httpResponse != 200) {
Copy link

Choose a reason for hiding this comment

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

!==

@smourph
Copy link

smourph commented Apr 10, 2018

Hi ! 👍 for this PR, the wrong order of parameters causes many mistakes to me 😞

About the choosen label: it is not an improvment, it is a real bug (no error message + wrong order of parameters on Consumer::handleError call)

@krichprollsch
Copy link
Contributor Author

you're welcome @smourph, happy to help you.
maybe @f2prateek could have another look on it?

@codecov-io
Copy link

Codecov Report

Merging #97 into master will decrease coverage by 0.65%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master     #97      +/-   ##
===========================================
- Coverage     86.05%   85.4%   -0.66%     
  Complexity      121     121              
===========================================
  Files             8       8              
  Lines           373     370       -3     
===========================================
- Hits            321     316       -5     
- Misses           52      54       +2
Impacted Files Coverage Δ Complexity Δ
lib/Segment/Consumer/LibCurl.php 93.93% <100%> (-0.51%) 8 <0> (ø)
lib/Segment.php 93.33% <0%> (-4.45%) 10% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d689e1c...24cee46. Read the comment docs.

@krichprollsch
Copy link
Contributor Author

krichprollsch commented May 4, 2018

Hello, I have updated my PR to solve the conflicts.
I have just seen #114 from @kevingilliard which did that work before.

Please @f2prateek can you merge one of these PR soon?
Moreover I think there are so many improvements which can be done in this client: namespaces, psr2 coding style (ty @kevingilliard for that), bumping php version, using psr/log, httplug...

@nd4p90x nd4p90x merged commit 08f6440 into segmentio:master Feb 2, 2021
@krichprollsch krichprollsch deleted the curl-error branch February 2, 2021 15:59
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.

5 participants