-
Notifications
You must be signed in to change notification settings - Fork 50
Allow CURL constant names #82
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
Conversation
foreach ($config as $key => $value) { | ||
// If the value starts with 'CURL' we assume it is a reference to a constant. | ||
if (strpos($key, 'CURL') === 0) { | ||
$contantValue = constant($key); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there something to check if a constant exists? imho an exception on nonexisting constant would be in order.
and s/contantValue/constantValue ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the constant does not exist constant() will return null.
Question: is it true that the only config parameters to the curl client are integers or constants named "CURL*"? If so, we should throw a exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
according to http://php.net/manual/en/function.curl-setopt.php it must be int. so either it already is an int here, or its a constant. maybe we could do
if (!is_int($key)) {
$constantValue = constant($key);
if (null === $constantValue) {
throw... $key is not an integer or a CURL constant.
}
unset...,
$config[$constantValue] = $value;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if we should force the constants to start with CURL - maybe there could be a reason to have some other constant that does not start with CURL.
is constant()
case sensitive?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Invalid constants (integers) will end up in an error upon setopt anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okey. Then I'm safe to assume that everything that comes in is either an Int or a name of a constant. If the constant is not defined I'll throw an exception. If the Int is invalid CURL will throw an exception
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the Int is invalid CURL will throw an exception
Setopt triggers an error IMO, but this is something that should be handled in the cURL client anyway.
Thank you for the feedback. I've updated the PR accordingly. |
awesome, thanks! |
Thank you for merging. |
This will fix #71