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

Update CURLRequest.php #2049

Closed
wants to merge 1 commit into from

Conversation

nowackipawel
Copy link
Contributor

aa362d0

prvious fix caused eception if debug has not been specified
Type: ErrorException
Message: curl_setopt_array(): supplied argument is not a valid File-Handle resource
Filename: /home/-exchange/site/system/HTTP/CURLRequest.php

aa362d0

prvious fix caused eception if debug has not been specified
Type:        ErrorException
Message:     curl_setopt_array(): supplied argument is not a valid File-Handle resource
Filename:    /home/-exchange/site/system/HTTP/CURLRequest.php
@nowackipawel
Copy link
Contributor Author

@lonnieezell : I think test "testDebugOption" is written wrongly. "

CURLOPT_STDERR | An alternative location to output errors to instead of STDERR.

IMHO CURLOPT_STDERR should be set to file handle or should not exists.

@nowackipawel
Copy link
Contributor Author

nowackipawel commented Jun 17, 2019

@lonnieezell could you answer pls? Am I wrong or not my approach is correct?

$curl_options[CURLOPT_VERBOSE] = 1;
$curl_options[CURLOPT_STDERR] = fopen('php://output', 'w+');
}
else if(is_resource($config['debug']))
Copy link
Member

Choose a reason for hiding this comment

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

I'm confused. When is $config['debug'] going to be a resource? According to the docs it should only ever be a boolean. If true, it turns on verbose and sets the error output, otherwise it leaves verbose to whatever it was otherwise set at.

@lonnieezell
Copy link
Member

While it may crash if debug hasn't been specified, what you're changing it to doesn't work. See my comment for explanation.

@nowackipawel
Copy link
Contributor Author

nowackipawel commented Jun 20, 2019 via email

@MGatner
Copy link
Member

MGatner commented Aug 28, 2019

I'm encountering this on a fresh install. Even without doing any configuration:

$client = Services::curlrequest();
$response = $client->request('GET', 'https://example.com');

I get an exception:

An uncaught Exception was encountered

Type:        ErrorException
Message:     curl_setopt_array(): supplied argument is not a valid File-Handle resource
Filename:    .../vendor/codeigniter4/codeigniter4/system/HTTP/CURLRequest.php
Line Number: 808

I'll dig into it a bit.

@MGatner
Copy link
Member

MGatner commented Aug 28, 2019

Okay a couple issues going on here.


First issue, the class starts with some default values for $config:

protected $config = [
	'timeout'         => 0.0,
	'connect_timeout' => 150,
	'debug'           => false,
	'verify'          => true,
];

This means that in setCURLOptions() the following will always be true:

	if (isset($config['debug']))
	{
		$curl_options[CURLOPT_VERBOSE] = $config['debug'] === true ? 1 : 0;
		$curl_options[CURLOPT_STDERR]  = $config['debug'] === true ? fopen('php://output', 'w+') : $config['debug'];
	}

Because of this if no overriding value was passed for debug then $curl_options[CURLOPT_STDERR] will end up being boolean false, which causes the error in my last comment. The easy fix for this issue is just if ($config['debug']).


Second issue, if $curl_options[CURLOPT_STDERR] is set it has to be a stream resource (not bool, not path). From PHP.net:

value should be a stream resource (using fopen(), for example) for the following values of the option parameter:
...
CURLOPT_STDERR An alternative location to output errors to instead of STDERR

Consulting the CI4 User Manual on CURLRequest:

You can pass a filename as the value for debug to have the output written to a file:
$response->request('GET', 'http://example.com', ['debug' => '/usr/local/curl_log.txt']);

So @lonnieezell we need to decide if we still want to pass in file paths, and if so we need to make a resource from them, so then need to decide if we're going to check the path or just let that be an exception from somewhere else (File class maybe?).


I can make a new PR but this needs a little scrutiny first.

MGatner added a commit to MGatner/CodeIgniter4 that referenced this pull request Aug 28, 2019
NOTE: These should fail until codeigniter4#2049 is resolved.
@MGatner MGatner mentioned this pull request Aug 28, 2019
5 tasks
@lonnieezell
Copy link
Member

@MGatner yes, you're right. My apologies to @nowackipawel

We should allow a file name to be specified - so convert it to a resource behind the scenes I guess. I don't think it needs to check the path. If it doesn't exist it should create it. If it does exist, and is writable, then it would get overwritten. If it can't be written to (or outside safe_mode boundaries) then it should naturally throw an exception. I would think that exception would be clear enough about what happened and why.

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.

3 participants