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

Pass url into curlinfo hook for effective url #364

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

whikloj
Copy link

@whikloj whikloj commented Feb 23, 2022

Context

Issue #363

When checking for errors in a curl_multi I pull the effective url to log with the error.

$url = curl_getinfo($curl_handles[$x], CURLINFO_EFFECTIVE_URL);

Which currently fails with php-vcr

BadMethodCallException: Not implemented: CURLINFO_EFFECTIVE_URL (1048577)

IMHO it really should just return the request url parameter from the source YAML.

What has been done

  • Added a nullable string parameter to the getCurlOptionFromResponse to pass in the URL (or null)
  • Get the url from the self::$requests array of VCR\Request objects

How to test

  • Before if you did a command like above you get the above error.
  • Now you should get the url provided in the request -> url parameter

Todo

  • [ ]
  • [ ]

Notes

  • I'm very new to php-vcr, so this might go against the pattern you are trying to work to. If so any guidance would be appreciated.

@whikloj
Copy link
Author

whikloj commented Feb 23, 2022

I'm using the 1.5 version to support some older versions of PHP. If this PR seems acceptable, would you be open to another patch release of that version?

@specialtactics
Copy link
Contributor

Hi, Would invite you to make this PR on our fork, if not we will likely do so ourselves at some point:
https://github.com/CoverGenius/php-vcr

Tjeerd pushed a commit to ibuildingsnl/php-vcr that referenced this pull request Aug 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants