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.

Sign up for GitHub

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

Jump to bottom

Cleanup PHP 7 curl handlers #989

Merged
merged 1 commit into from Aug 18, 2020
Merged

Cleanup PHP 7 curl handlers #989

merged 1 commit into from Aug 18, 2020

Conversation

morrisonlevi
Copy link
Collaborator

@morrisonlevi morrisonlevi commented Aug 18, 2020 •
edited

Description

I took a peek to see how much code could be re-used if we tried to implement curl_multi_exec support. I think the hardest bits can all be reused, which is good news. I did notice a few things that could be cleaned up while looking, so that's what this PR is (no multi curl support yet, just cleanup).

The dd_tracer_inject_helper now uses the stackful goto cleanup strategy, which I think is at least better than what it was doing before. Next step: remove goto?

Readiness checklist

  • Changelog has been added to the release document.
  • Tests added for this feature/bug.

Reviewer checklist

  • Appropriate labels assigned.
  • Milestone is set.
  • Changelog has been added to the release document. For community contributors the reviewer is in charge of this task.

@morrisonlevi morrisonlevi added 🎉 new-integration A new integration c-extension Apply this label to issues and prs related to the C-extension labels Aug 18, 2020
@morrisonlevi morrisonlevi added this to the 0.48.0 milestone Aug 18, 2020
@morrisonlevi
Cleanup PHP 7 curl handlers
efdfec1 
dd_tracer_inject_helper now uses the stackful goto cleanup strategy,
which I think is at least better than what it was doing before.

Next step: remove goto?
SammyK
SammyK approved these changes Aug 18, 2020
View reviewed changes
Copy link
Contributor

@SammyK SammyK left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@morrisonlevi morrisonlevi marked this pull request as ready for review August 18, 2020 14:17
@morrisonlevi morrisonlevi merged commit 246e2ee into master Aug 18, 2020
@morrisonlevi morrisonlevi deleted the levi/build/curl_cleanup branch August 18, 2020 14:17
@roippi roippi mentioned this pull request Aug 31, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SammyK SammyK SammyK approved these changes

Assignees
No one assigned
Labels
c-extension Apply this label to issues and prs related to the C-extension 🎉 new-integration A new integration
Projects
None yet
Milestone
0.48.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@morrisonlevi @SammyK