Skip to content

Commit f77c7ac

Browse files
Copilotdermatz
Copilot
and
dermatz
authored
Address code review feedback: fix duplicate config, improve regex patterns, enhance security (#65)
* Initial plan * Apply code review feedback: fix duplicate di.xml, improve regex patterns, safer environment handling Co-authored-by: dermatz <[email protected]> * Replace error suppression with explicit exception handling in TTY detection Co-authored-by: dermatz <[email protected]> * Fix RequireJS pattern regex to properly match module references Co-authored-by: dermatz <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: dermatz <[email protected]>
1 parent 2f4a1c1 commit f77c7ac

File tree

5 files changed

+50
-79
lines changed

5 files changed

+50
-79
lines changed

‎docs/commands.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -205,7 +205,7 @@ bin/magento mageforge:hyva:compatibility:check [options]
205205
**Options**:
206206

207207
- `--show-all` / `-a` - Show all modules including compatible ones
208-
- `--third-party-only` / `-t` - Check only third-party modules (exclude Magento\_\* modules)
208+
- `--third-party-only` / `-t` - Check only third-party modules (exclude Magento_* modules)
209209
- `--include-vendor` - Include Magento core modules in scan (default: third-party only)
210210
- `--detailed` / `-d` - Show detailed file-level issues for incompatible modules
211211

‎src/Console/Command/Hyva/CompatibilityCheckCommand.php‎

Lines changed: 27 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -96,8 +96,7 @@ private function runInteractiveMode(InputInterface $input, OutputInterface $outp
9696
label: 'Select scan options',
9797
options: [
9898
'show-all' => 'Show all modules including compatible ones',
99-
'incompatible-only' => 'Show only incompatible modules (default behavior)',
100-
'include-vendor' => 'Include Magento core modules (default: third-party only)',
99+
'include-vendor' => 'Include vendor modules (default: excluded)',
101100
'detailed' => 'Show detailed file-level issues with line numbers',
102101
],
103102
default: [],
@@ -111,7 +110,6 @@ private function runInteractiveMode(InputInterface $input, OutputInterface $outp
111110

112111
// Apply selected options to input
113112
$showAll = in_array('show-all', $selectedOptions);
114-
$incompatibleOnly = in_array('incompatible-only', $selectedOptions);
115113
$includeVendor = in_array('include-vendor', $selectedOptions);
116114
$detailed = in_array('detailed', $selectedOptions);
117115
$thirdPartyOnly = false; // Not needed in interactive mode
@@ -121,13 +119,11 @@ private function runInteractiveMode(InputInterface $input, OutputInterface $outp
121119
$config = [];
122120
if ($showAll) {
123121
$config[] = 'Show all modules';
124-
} elseif ($incompatibleOnly) {
125-
$config[] = 'Show incompatible only';
126122
} else {
127123
$config[] = 'Show modules with issues';
128124
}
129125
if ($includeVendor) {
130-
$config[] = 'Include Magento core';
126+
$config[] = 'Include vendor modules';
131127
} else {
132128
$config[] = 'Third-party modules only';
133129
}
@@ -137,8 +133,8 @@ private function runInteractiveMode(InputInterface $input, OutputInterface $outp
137133
$this->io->comment('Configuration: ' . implode(', ', $config));
138134
$this->io->newLine();
139135

140-
// Run scan with selected options (pass incompatibleOnly flag)
141-
return $this->runScan($showAll, $thirdPartyOnly, $includeVendor, $detailed, $incompatibleOnly, $output);
136+
// Run scan with selected options
137+
return $this->runScan($showAll, $thirdPartyOnly, $includeVendor, $detailed, false, $output);
142138
} catch (\Exception $e) {
143139
$this->resetPromptEnvironment();
144140
$this->io->error('Interactive mode failed: ' . $e->getMessage());
@@ -176,12 +172,11 @@ private function runScan(
176172
): int {
177173

178174
// Determine filter logic for vendor and third-party modules:
179-
// - excludeVendor controls whether to scan modules in the vendor/ directory
180-
// - By default (no flags): scan third-party modules including those in vendor/
181-
// - With --include-vendor: scan everything including Magento_* core modules
175+
// - By default (no flags): scan third-party modules excluding those in vendor/
176+
// - With --include-vendor: include vendor modules in the scan
182177
// - With --third-party-only: explicitly scan only third-party modules
183178
$scanThirdPartyOnly = $thirdPartyOnly || (!$includeVendor && !$thirdPartyOnly);
184-
$excludeVendor = false; // Always include vendor for third-party scanning
179+
$excludeVendor = !$includeVendor;
185180

186181
// Run the compatibility check
187182
$results = $this->compatibilityChecker->check(
@@ -369,9 +364,25 @@ private function isInteractiveTerminal(OutputInterface $output): bool
369364
}
370365
}
371366

372-
// Additional check: try to detect if running in a proper TTY
373-
$sttyOutput = shell_exec('stty -g 2>/dev/null');
374-
return !empty($sttyOutput);
367+
// Additional check: detect if running in a proper TTY using safer methods
368+
if (\function_exists('stream_isatty') && \defined('STDIN')) {
369+
try {
370+
return \stream_isatty(\STDIN);
371+
} catch (\Throwable $e) {
372+
// Fall through to next check
373+
}
374+
}
375+
376+
if (\function_exists('posix_isatty') && \defined('STDIN')) {
377+
try {
378+
return \posix_isatty(\STDIN);
379+
} catch (\Throwable $e) {
380+
// Fall through to default
381+
}
382+
}
383+
384+
// Conservative default if no TTY-detection functions are available
385+
return false;
375386
}
376387

377388
/**
@@ -450,7 +461,7 @@ private function getServerVar(string $name): ?string
450461
private function setEnvVar(string $name, string $value): void
451462
{
452463
$this->secureEnvStorage[$name] = $value;
453-
putenv("$name=$value");
464+
$_SERVER[$name] = $value;
454465
}
455466

456467
/**

‎src/Service/Hyva/IncompatibilityDetector.php‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ class IncompatibilityDetector
4343
'severity' => self::SEVERITY_WARNING,
4444
],
4545
[
46-
'pattern' => '/(?:define|require)\s*\(\s*\[[^\]]*["\']mage\/[^"\']*["\']/',
46+
'pattern' => '/(?:define|require)\s*\(\s*\[[^\]]*["\']mage\/[^"\']*["\']\s*[^\]]*\]/',
4747
'description' => 'Magento RequireJS module reference',
4848
'severity' => self::SEVERITY_CRITICAL,
4949
],
@@ -65,7 +65,7 @@ class IncompatibilityDetector
6565
'severity' => self::SEVERITY_CRITICAL,
6666
],
6767
[
68-
'pattern' => '/<referenceBlock.*remove="true">/',
68+
'pattern' => '/<referenceBlock\b[^>]*\bremove\s*=\s*"true"[^>]*>/s',
6969
'description' => 'Block removal (review for Hyvä compatibility)',
7070
'severity' => self::SEVERITY_WARNING,
7171
],
@@ -82,7 +82,7 @@ class IncompatibilityDetector
8282
'severity' => self::SEVERITY_CRITICAL,
8383
],
8484
[
85-
'pattern' => '/\$\(.*\)\..*\(/',
85+
'pattern' => '/\$\([^)]*\)\s*\.(on|click|ready|change|keyup|keydown|submit|ajax|each|css|hide|show|addClass|removeClass|toggleClass|append|prepend|html|text|val|attr|prop|data|trigger|find|parent|children)\s*\(/',
8686
'description' => 'jQuery DOM manipulation',
8787
'severity' => self::SEVERITY_WARNING,
8888
],

‎src/Service/Hyva/ModuleScanner.php‎

Lines changed: 16 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -107,31 +107,7 @@ private function findRelevantFiles(string $directory): array
107107
}
108108

109109
/**
110-
* Check if module has Hyvä compatibility package based on composer data
111-
*
112-
* @param array $composerData Parsed composer.json data
113-
*/
114-
private function isHyvaCompatibilityPackage(array $composerData): bool
115-
{
116-
// Check if this IS a Hyvä compatibility package
117-
$packageName = $composerData['name'] ?? '';
118-
if (str_starts_with($packageName, 'hyva-themes/') && str_contains($packageName, '-compat')) {
119-
return true;
120-
}
121-
122-
// Check dependencies for Hyvä packages
123-
$requires = $composerData['require'] ?? [];
124-
foreach ($requires as $package => $version) {
125-
if (str_starts_with($package, 'hyva-themes/')) {
126-
return true;
127-
}
128-
}
129-
130-
return false;
131-
}
132-
133-
/**
134-
* Check if module has Hyvä compatibility package (public wrapper)
110+
* Check if module has Hyvä compatibility package
135111
*/
136112
public function hasHyvaCompatibilityPackage(string $modulePath): bool
137113
{
@@ -149,7 +125,21 @@ public function hasHyvaCompatibilityPackage(string $modulePath): bool
149125
return false;
150126
}
151127

152-
return $this->isHyvaCompatibilityPackage($composerData);
128+
// Check if this IS a Hyvä compatibility package
129+
$packageName = $composerData['name'] ?? '';
130+
if (str_starts_with($packageName, 'hyva-themes/') && str_contains($packageName, '-compat')) {
131+
return true;
132+
}
133+
134+
// Check dependencies for Hyvä packages
135+
$requires = $composerData['require'] ?? [];
136+
foreach ($requires as $package => $version) {
137+
if (str_starts_with($package, 'hyva-themes/')) {
138+
return true;
139+
}
140+
}
141+
142+
return false;
153143
} catch (\Exception $e) {
154144
// Log error when debugging to help identify JSON or file read issues
155145
if (getenv('MAGEFORGE_DEBUG')) {

‎src/etc/di.xml‎

Lines changed: 3 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -3,39 +3,6 @@
33
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
44
xsi:noNamespaceSchemaLocation="urn:magento:framework:ObjectManager/etc/config.xsd"
55
>
6-
<type name="Magento\Framework\Console\CommandList">
7-
<arguments>
8-
<argument
9-
name="commands"
10-
xsi:type="array"
11-
>
12-
<item name="mageforge_system_version"
13-
xsi:type="object"
14-
>
15-
OpenForgeProject\MageForge\Console\Command\System\VersionCommand</item>
16-
<item name="mageforge_system_check"
17-
xsi:type="object"
18-
>
19-
OpenForgeProject\MageForge\Console\Command\System\CheckCommand</item>
20-
<item name="mageforge_theme_list"
21-
xsi:type="object"
22-
>
23-
OpenForgeProject\MageForge\Console\Command\Theme\ListCommand</item>
24-
<item name="mageforge_theme_build"
25-
xsi:type="object"
26-
>
27-
OpenForgeProject\MageForge\Console\Command\Theme\BuildCommand</item>
28-
<item name="mageforge_theme_watch"
29-
xsi:type="object"
30-
>
31-
OpenForgeProject\MageForge\Console\Command\Theme\WatchCommand</item>
32-
<item name="mageforge_hyva_compatibility_check"
33-
xsi:type="object"
34-
>
35-
OpenForgeProject\MageForge\Console\Command\Hyva\CompatibilityCheckCommand</item>
36-
</argument>
37-
</arguments>
38-
</type>
396
<type name="Magento\Framework\Console\CommandList">
407
<arguments>
418
<argument
@@ -60,6 +27,9 @@
6027
<item name="mageforge_static_clean"
6128
xsi:type="object"
6229
>OpenForgeProject\MageForge\Console\Command\Static\CleanCommand</item>
30+
<item name="mageforge_hyva_compatibility_check"
31+
xsi:type="object"
32+
>OpenForgeProject\MageForge\Console\Command\Hyva\CompatibilityCheckCommand</item>
6333
</argument>
6434
</arguments>
6535
</type>

0 commit comments

Comments
 (0)