Skip to content

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

Merged
merged 2 commits into from
Jun 30, 2016
Merged

Allow CURL constant names #82

merged 2 commits into from
Jun 30, 2016

Conversation

Nyholm
Copy link
Member

@Nyholm Nyholm commented Jun 29, 2016

This will fix #71

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);
Copy link
Collaborator

@dbu dbu Jun 30, 2016

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 ;-)

Copy link
Member Author

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.

Copy link
Collaborator

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;
}

Copy link
Collaborator

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?

Copy link
Member

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.

Copy link
Member Author

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

Copy link
Member

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.

@Nyholm
Copy link
Member Author

Nyholm commented Jun 30, 2016

Thank you for the feedback. I've updated the PR accordingly.

@dbu dbu merged commit be6f9f5 into php-http:master Jun 30, 2016
@dbu
Copy link
Collaborator

dbu commented Jun 30, 2016

awesome, thanks!

@Nyholm Nyholm deleted the curl-constants branch June 30, 2016 12:22
@Nyholm
Copy link
Member Author

Nyholm commented Jun 30, 2016

Thank you for merging.

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.

Add support for string curl client params
3 participants