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

feat: CURL option force_ip_resolve #9194

Open
wants to merge 5 commits into
base: 4.6
Choose a base branch
from

Conversation

ddevsr
Copy link
Collaborator

@ddevsr ddevsr commented Sep 20, 2024

Description
I think this needed for resolve only ipv4 or ipv6.

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@ddevsr ddevsr added enhancement PRs that improve existing functionalities 4.6 labels Sep 20, 2024
@ddevsr ddevsr changed the title feat: CURL option force_ip_resolve feat: CURL option force_ip_resolve Sep 20, 2024
phpstan-baseline.php Outdated Show resolved Hide resolved
@@ -649,6 +649,17 @@ protected function setCURLOptions(array $curlOptions = [], array $config = [])
$this->setHeader('Content-Length', (string) strlen($json));
}

// Resolve IP
if (array_key_exists('force_ip_resolve', $config) && $config['force_ip_resolve']) {
Copy link
Member

Choose a reason for hiding this comment

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

right side of &&, check for is_string then empty string

Comment on lines +653 to +661
if (array_key_exists('force_ip_resolve', $config) && $config['force_ip_resolve']) {
$protocolVersion = $config['force_ip_resolve'];

if ($protocolVersion === 'v4') {
$curlOptions[CURLOPT_IPRESOLVE] = CURL_IPRESOLVE_V4;
} elseif ($protocolVersion === 'v6') {
$curlOptions[CURLOPT_IPRESOLVE] = CURL_IPRESOLVE_V6;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

How about:

Suggested change
if (array_key_exists('force_ip_resolve', $config) && $config['force_ip_resolve']) {
$protocolVersion = $config['force_ip_resolve'];
if ($protocolVersion === 'v4') {
$curlOptions[CURLOPT_IPRESOLVE] = CURL_IPRESOLVE_V4;
} elseif ($protocolVersion === 'v6') {
$curlOptions[CURLOPT_IPRESOLVE] = CURL_IPRESOLVE_V6;
}
}
if (array_key_exists('force_ip_resolve', $config)) {
$curlOptions[CURLOPT_IPRESOLVE] = match($config['force_ip_resolve']) {
'v4' => CURL_IPRESOLVE_V4,
'v6' => CURL_IPRESOLVE_V6,
default => CURL_IPRESOLVE_WHATEVER
};
}

Copy link
Member

Choose a reason for hiding this comment

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

or throw for unknown value?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm... yes, that's probably even better than silently selecting the default value.

Copy link
Collaborator Author

@ddevsr ddevsr Sep 20, 2024

Choose a reason for hiding this comment

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

I think dont any insert option when no call, better throw

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This options same behaviour in version. If there no set declaration, we follow default of PHP Curl

Copy link
Member

Choose a reason for hiding this comment

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

What if I initialize this service with custom options? I can't change CURLOPT_IPRESOLVE to the default value later?

This seems like an issue - same story with version. There is no option to reset it to the default value during the instance lifecycle.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.6 enhancement PRs that improve existing functionalities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants