refactor(HTTP): split setCURLOptions into category helper methods to reduce complexity#10339
Open
gr8man wants to merge 2 commits into
Open
refactor(HTTP): split setCURLOptions into category helper methods to reduce complexity#10339gr8man wants to merge 2 commits into
gr8man wants to merge 2 commits into
Conversation
gr8man
force-pushed
the
refactor/setcurloptions
branch
from
0ea2fd3 to
2c5d21c
Compare
michalsn
reviewed
Jun 25, 2026
michalsn
left a comment
michalsn
left a comment
Member
There was a problem hiding this comment.
I like the general idea, this will make the codebase better. But...
applyPayloadOptions() and applyMiscOptions() are trying to do too many things. I would reorganize these methods into four groups:
applyBodyOptions() // form_params, multipart, json
applyResponseOptions() // decode_content, http_errors
applyProtocolOptions() // HTTP version
applyClientOptions() // cookie, user_agentI would prefer testing the observable behavior through setCURLOptions() or the public request API rather than testing every private helper directly. That keeps the tests stable if these internal groups are reorganized later.
| private function applyAuthOptions(array $curlOptions, array $config): array | ||
| { | ||
| // Auth Headers | ||
| if (isset($config['auth']) && $config['auth'] !== []) { |
Member
There was a problem hiding this comment.
Suggested change
| if (isset($config['auth']) && $config['auth'] !== []) { | |
| if (isset($config['auth']) && is_array($config['auth']) && count($config['auth']) >= 2) { |
| { | ||
| // Certificate | ||
| if (! empty($config['cert'])) { | ||
| if (isset($config['cert']) && $config['cert'] !== '' && $config['cert'] !== []) { |
Member
There was a problem hiding this comment.
Suggested change
| if (isset($config['cert']) && $config['cert'] !== '' && $config['cert'] !== []) { | |
| $cert = $config['cert'] ?? null; | |
| if ((bool) $cert) { |
| { | ||
| // Debug | ||
| if ($config['debug']) { | ||
| if (isset($config['debug']) && $config['debug'] !== false && $config['debug'] !== '') { |
Member
There was a problem hiding this comment.
Suggested change
| if (isset($config['debug']) && $config['debug'] !== false && $config['debug'] !== '') { | |
| if ((bool) ($config['debug'] ?? false)) { |
| private function applyPayloadOptions(array $curlOptions, array $config): array | ||
| { | ||
| // Decode Content | ||
| if (isset($config['decode_content']) && $config['decode_content'] !== false) { |
Member
There was a problem hiding this comment.
Suggested change
| if (isset($config['decode_content']) && $config['decode_content'] !== false) { | |
| if ((bool) ($config['decode_content'] ?? false)) { |
|
|
||
| // version | ||
| if (! empty($config['version'])) { | ||
| if (isset($config['version']) && $config['version'] !== '') { |
Member
There was a problem hiding this comment.
Suggested change
| if (isset($config['version']) && $config['version'] !== '') { | |
| $version = $config['version'] ?? null; | |
| if ((bool) $version) { |
Comment on lines
-42
to
-43
| parent::setUp(); | ||
|
|
Member
There was a problem hiding this comment.
Is this a mistake? Why are you removing this?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR refactors the massive
setCURLOptions()method inCodeIgniter\HTTP\CURLRequest(which was a 180+ line method with extremely high cyclomatic complexity) by splitting it into 8 domain-specific private helper methods. This change adheres to the Single Responsibility Principle (SRP) and the Open-Closed Principle (OCP), making the options-building logic significantly cleaner, easier to read, and simpler to maintain without introducing any backward compatibility (BC) breaks.Additionally, static analysis (PHPStan Level 6 with strict rules) and test validation have been integrated for all newly created helper methods.
Proposed Changes
system/HTTP/CURLRequest.php:setCURLOptions()into:applyAuthOptions()applySslOptions()applyProxyOptions()applyDebugOptions()applyRedirectOptions()applyConnectionOptions()applyPayloadOptions()applyMiscOptions()empty()checks with strict comparison operators (isset(),!== '', etc.) inside the new helper methods to comply with strict PHPStan rules.missingType.iterableValuewarnings).tests/system/HTTP/CURLRequestTest.php:testApply*Direct) usinggetPrivateMethodInvokerfor all 8 helper methods to ensure maximum test coverage.assertSametoassertEqualsWithDeltato satisfy Rector rules.utils/phpstan-baseline/empty.notAllowed.neon:empty()constructs forCURLRequestto match the new strict refactoring.Checklist
OK (100 tests, 269 assertions)).[OK] No errors).php-cs-fixer).