Skip to content

Commit fe8d901

Browse files
Copilotdermatz
andcommitted
🔧 refactor: address code review suggestions for robustness and standards compliance
- Replace manual path extraction with dirname() for better reliability - Add CSS value sanitization to prevent syntax issues from untrusted tokens - Use str_contains() instead of strpos() for modern PHP 8.3+ - Improve vendor theme path fallback logic with multi-level handling - Add finally blocks to restore working directory in watch mode - Add phpcs:ignore comment for shell_exec TTY detection Co-authored-by: dermatz <6103201+dermatz@users.noreply.github.com>
1 parent c5ff72e commit fe8d901

5 files changed

Lines changed: 54 additions & 11 deletions

File tree

‎src/Service/EnvironmentService.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ public function isInteractiveTerminal(): bool
3535
}
3636

3737
// Additional check: try to detect if running in a proper TTY
38+
// phpcs:ignore Magento2.Security.InsecureFunction -- Safe static 'stty -g' usage for TTY detection with error redirection; no user input
3839
$sttyOutput = shell_exec('stty -g 2>/dev/null');
3940
return !empty($sttyOutput);
4041
}

‎src/Service/HyvaTokens/ConfigReader.php‎

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,7 @@ public function getOutputPath(string $themePath): string
119119
*/
120120
public function isVendorTheme(string $themePath): bool
121121
{
122-
return strpos($themePath, '/vendor/') !== false;
122+
return str_contains($themePath, '/vendor/');
123123
}
124124

125125
/**
@@ -131,12 +131,32 @@ public function isVendorTheme(string $themePath): bool
131131
private function getVendorThemeOutputPath(string $themePath): string
132132
{
133133
// Extract theme identifier from path
134-
// e.g., /path/to/vendor/hyva-themes/magento2-default-theme -> hyva-themes/magento2-default-theme
134+
// e.g., /path/to/vendor/hyva-themes/magento2-default-theme
135+
// -> hyva-themes/magento2-default-theme
135136
preg_match('#/vendor/([^/]+/[^/]+)#', $themePath, $matches);
136-
$themeIdentifier = $matches[1] ?? 'unknown';
137+
138+
if (isset($matches[1])) {
139+
$themeIdentifier = $matches[1];
140+
} else {
141+
// Fallback: derive identifier from the last two path segments
142+
$normalizedPath = str_replace('\\', '/', rtrim($themePath, '/'));
143+
$pathSegments = explode('/', $normalizedPath);
144+
$segmentCount = count($pathSegments);
145+
146+
if ($segmentCount >= 2) {
147+
$themeIdentifier = $pathSegments[$segmentCount - 2]
148+
. '/' . $pathSegments[$segmentCount - 1];
149+
} else {
150+
// Ensure a unique, deterministic identifier as a last resort
151+
$themeIdentifier = 'theme_' . md5($themePath);
152+
}
153+
}
137154

138155
$varPath = $this->directoryList->getPath(DirectoryList::VAR_DIR);
139-
return $varPath . '/view_preprocessed/hyva-tokens/' . $themeIdentifier . '/hyva-tokens.css';
156+
return $varPath
157+
. '/view_preprocessed/hyva-tokens/'
158+
. $themeIdentifier
159+
. '/hyva-tokens.css';
140160
}
141161

142162
/**

‎src/Service/HyvaTokens/CssGenerator.php‎

Lines changed: 25 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -29,14 +29,36 @@ public function generate(array $tokens, string $cssSelector): string
2929

3030
foreach ($tokens as $name => $value) {
3131
$cssVarName = '--' . $name;
32-
$css .= " {$cssVarName}: {$value};\n";
32+
// Sanitize value to prevent CSS syntax issues
33+
$sanitizedValue = $this->sanitizeCssValue($value);
34+
$css .= " {$cssVarName}: {$sanitizedValue};\n";
3335
}
3436

3537
$css .= "}\n";
3638

3739
return $css;
3840
}
3941

42+
/**
43+
* Sanitize CSS value to prevent syntax issues
44+
*
45+
* @param string $value
46+
* @return string
47+
*/
48+
private function sanitizeCssValue(string $value): string
49+
{
50+
// Remove newlines and control characters
51+
$value = preg_replace('/[\r\n\t\x00-\x1F\x7F]/', '', $value);
52+
53+
// Remove potentially problematic characters
54+
// Allow: alphanumeric, spaces, parentheses, commas, periods, hyphens, underscores,
55+
// percent signs, hash symbols, and forward slashes
56+
$sanitized = preg_replace('/[^\w\s(),.%#\/-]/', '', $value);
57+
58+
// Trim whitespace
59+
return trim($sanitized ?? '');
60+
}
61+
4062
/**
4163
* Write CSS to file
4264
*
@@ -47,10 +69,8 @@ public function generate(array $tokens, string $cssSelector): string
4769
*/
4870
public function write(string $content, string $outputPath): bool
4971
{
50-
// Ensure the directory exists by extracting parent directory path
51-
$pathParts = explode('/', $outputPath);
52-
array_pop($pathParts); // Remove filename
53-
$directory = implode('/', $pathParts);
72+
// Ensure the directory exists
73+
$directory = \dirname($outputPath);
5474

5575
if (!$this->fileDriver->isDirectory($directory)) {
5676
$this->fileDriver->createDirectory($directory, 0750);

‎src/Service/ThemeBuilder/HyvaThemes/Builder.php‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -245,9 +245,10 @@ public function watch(string $themePath, SymfonyStyle $io, OutputInterface $outp
245245
$this->shell->execute('npm run watch');
246246
} catch (\Exception $e) {
247247
$io->error('Failed to start watch mode: ' . $e->getMessage());
248+
return false;
249+
} finally {
248250
// phpcs:ignore MEQP1.Security.DiscouragedFunction -- chdir is necessary to restore original directory
249251
chdir($currentDir);
250-
return false;
251252
}
252253

253254
return true;

‎src/Service/ThemeBuilder/TailwindCSS/Builder.php‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -217,9 +217,10 @@ public function watch(string $themePath, SymfonyStyle $io, OutputInterface $outp
217217
$this->shell->execute('npm run watch');
218218
} catch (\Exception $e) {
219219
$io->error('Failed to start watch mode: ' . $e->getMessage());
220+
return false;
221+
} finally {
220222
// phpcs:ignore MEQP1.Security.DiscouragedFunction -- chdir is necessary to restore original directory
221223
chdir($currentDir);
222-
return false;
223224
}
224225

225226
return true;

0 commit comments

Comments
 (0)