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

fixes #2049 - debug option #2204

Closed

Conversation

michalsn
Copy link
Member

@michalsn michalsn commented Sep 9, 2019

Description
CURLRequest class with the default debug value reports an error:

curl_setopt_array(): supplied argument is not a valid File-Handle resource

This pull request fixes this issue and ensures to create resource if we specify a path to the log file. I believe that tests for this are already started in #2168.

Checklist:

  • Securely signed commits
  • Component(s) with PHPdocs
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@MGatner
Copy link
Member

MGatner commented Sep 10, 2019

I think @xbotkaj was right and $config[‘debug’] will always be set so we should check if ($config... instead of isset.

Also can you merge my tests from #2168? Let me know if you want help with a cross-user-fork merge.

@michalsn
Copy link
Member Author

Yes, I removed the isset option. I also merged your tests - thanks.

Crap... seems like merge portion wasn't signed 😞 oh well...

@MGatner
Copy link
Member

MGatner commented Sep 14, 2019

This is looking good. If there is any way you can setup signing on the device you did the merge from then GitHub will verify it retroactively. Unless @lonnieezell wants to override the merge block.

@MGatner MGatner requested a review from lonnieezell September 14, 2019 18:17
@michalsn michalsn force-pushed the feature/curl_debug_fix branch from 2d7ebd9 to c407a15 Compare September 14, 2019 19:28
@michalsn
Copy link
Member Author

Well... I made this (not signed) merge on the same device. I tried to fix it somehow yesterday but from what I can see I failed. So, idk... if you can't merge this just close this pull request, copy my code (it's just 3 lines actually) and add it to your branch with tests @MGatner - I don't mind.

@MGatner
Copy link
Member

MGatner commented Sep 15, 2019

:D thanks! I can probably merge your commits and keep you as the author and just do a verified merge myself. I’ll take a look later tonight - we already got another CURL debug report so I’m eager to get this in.

@MGatner
Copy link
Member

MGatner commented Sep 16, 2019

@michalsn I added your commits to #2168 and assuming it passes we should be all set. Feel free to close this PR if you are satisfied.

@michalsn
Copy link
Member Author

Ok, thanks @MGatner.

@michalsn michalsn closed this Sep 16, 2019
@michalsn michalsn deleted the feature/curl_debug_fix branch September 17, 2019 15:23
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.

2 participants