-
Notifications
You must be signed in to change notification settings - Fork 2
fix codequality #51
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
fix codequality #51
Conversation
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.
Pull request overview
This PR refactors code quality issues reported by Codacy by extracting environment variable handling logic from BuildCommand into dedicated service classes, improving separation of concerns and reusability.
Key Changes
- Created
TerminalEnvironmentServiceto handle secure environment variable operations (get, set, reset) - Created
InteractiveTerminalCheckerto detect if the terminal supports interactive input - Refactored
BuildCommandto use the new services via dependency injection instead of private methods
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/Service/Terminal/TerminalEnvironmentService.php | New service class for secure environment variable handling with sanitization and validation |
| src/Service/Terminal/InteractiveTerminalChecker.php | New service class to check if terminal supports interactive input (e.g., not in CI environments) |
| src/Console/Command/Theme/BuildCommand.php | Refactored to inject and use new terminal services, removing ~265 lines of private methods |
| * Clear the environment variable cache | ||
| */ | ||
| private function clearEnvironmentCache(): void | ||
| { | ||
| $this->secureEnvStorage = []; |
Copilot
AI
Dec 19, 2025
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.
The method clearEnvironmentCache() clears the entire secureEnvStorage array, but this array is used to store set environment variables, not the cached environment variables from getCachedEnvironmentVariables(). The static cache in getCachedEnvironmentVariables() cannot be cleared from outside that method. This implementation appears to be incorrect - clearing secureEnvStorage will remove all stored environment variables, not just the cache.
| * Clear the environment variable cache | |
| */ | |
| private function clearEnvironmentCache(): void | |
| { | |
| $this->secureEnvStorage = []; | |
| * Clear the environment variable cache. | |
| * | |
| * Note: secureEnvStorage holds the authoritative set of environment | |
| * variables and is not treated as a cache. This method is intentionally | |
| * left as a no-op to avoid clearing stored environment variables. | |
| */ | |
| private function clearEnvironmentCache(): void | |
| { | |
| // No-op: cache clearing is handled elsewhere if implemented. |
| if (!isset($this->secureEnvStorage)) { | ||
| $this->secureEnvStorage = []; | ||
| } |
Copilot
AI
Dec 19, 2025
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.
The property secureEnvStorage is initialized as an empty array in line 13, so the check if (!isset($this->secureEnvStorage)) will always be false. This condition is unnecessary and should be removed.
| if (!isset($this->secureEnvStorage)) { | |
| $this->secureEnvStorage = []; | |
| } |
| if (empty($name) || !is_string($name)) { | ||
| return; | ||
| } | ||
|
|
Copilot
AI
Dec 19, 2025
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.
The validation empty($name) || !is_string($name) is redundant since the parameter is type-hinted as string. The !is_string($name) check will never be true, and empty($name) would only catch empty strings. Consider simplifying this to just check for empty strings if needed, or remove it entirely since the regex validation on line 56 will catch invalid names.
| if (empty($name) || !is_string($name)) { | |
| return; | |
| } |
| private function getCachedEnvironmentVariables(): array | ||
| { | ||
| static $cachedEnv = null; | ||
|
|
||
| if ($cachedEnv === null) { | ||
| $cachedEnv = []; | ||
| $allowedVars = ['COLUMNS', 'LINES', 'TERM', 'CI', 'GITHUB_ACTIONS', 'GITLAB_CI', 'JENKINS_URL', 'TEAMCITY_VERSION']; | ||
|
|
||
| foreach ($allowedVars as $var) { | ||
| if (isset($this->secureEnvStorage[$var])) { | ||
| $cachedEnv[$var] = $this->secureEnvStorage[$var]; | ||
| } else { | ||
| $globalEnv = filter_input_array(INPUT_ENV) ?: []; | ||
| if (array_key_exists($var, $globalEnv)) { | ||
| $cachedEnv[$var] = (string) $globalEnv[$var]; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return $cachedEnv; | ||
| } |
Copilot
AI
Dec 19, 2025
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.
The static cache in getCachedEnvironmentVariables() is never invalidated when environment variables are set or removed via setEnvVar() or removeSecureEnvironmentValue(). This means once the cache is initialized, it won't reflect any changes made through this service, leading to stale data being returned by getEnvVar(). The cache should be cleared when the secure storage is modified, or the logic should be restructured to check secure storage first before relying on the static cache.
Fix Code-Quality reported by codacy