-
Notifications
You must be signed in to change notification settings - Fork 2
Feature hyva compat checker #66
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
Conversation
- New command: mageforge:hyva:compatibility:check (aliases: m:h:c:c, hyva:check) - Scans Magento modules for Hyvä theme incompatibilities - Detects RequireJS, Knockout.js, jQuery, UI Components usage - Options: --show-all, --third-party-only, --include-vendor, --detailed - Service layer: CompatibilityChecker, ModuleScanner, IncompatibilityDetector - Exit code 1 for critical issues, 0 for success - Updated CI/CD workflow and documentation
- Changed default behavior from 0 modules to third-party modules (18) - Without flags: Scans third-party only (excludes Magento_*) - With --include-vendor: Scans all 394 modules including Magento core - Updated documentation to reflect new default behavior
- Interactive mode activated when no options provided - Multi-select menu for scan options: * Show all modules * Include Magento core modules * Detailed file-level issues - Displays selected configuration before scan - Falls back to direct mode if interactive fails - Consistent with theme:build command UX
- New option: Show only incompatible modules (pre-selected by default) - Improves UX by filtering out compatible modules - Users can still choose 'Show all' for complete overview - Configuration display updated to reflect selection
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
* Initial plan * Add actual execution tests for Hyvä compatibility checker command Co-authored-by: dermatz <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: dermatz <[email protected]> Co-authored-by: Mathias Elle <[email protected]>
#61) * Initial plan * Clarify exit code documentation for Hyvä compatibility checker Co-authored-by: dermatz <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: dermatz <[email protected]> Co-authored-by: Mathias Elle <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: Mathias Elle <[email protected]>
Co-authored-by: Copilot <[email protected]>
…terns, 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]>
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 introduces a Hyvä theme compatibility checker feature that scans Magento modules for potential incompatibilities with Hyvä themes. The checker detects problematic patterns like RequireJS, Knockout.js, jQuery, and UI Components usage.
Changes:
- Added new
mageforge:hyva:compatibility:checkcommand with interactive mode support and multiple CLI options - Implemented three new service classes for scanning, detection, and compatibility checking
- Updated documentation with comprehensive command details and pattern descriptions
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| src/etc/di.xml | Registers new Hyvä compatibility command and updates DI configuration (contains critical duplicate type definition) |
| src/Service/Hyva/ModuleScanner.php | New service for recursively scanning module directories and detecting Hyvä compatibility packages |
| src/Service/Hyva/IncompatibilityDetector.php | New service implementing pattern matching for detecting RequireJS, Knockout, jQuery, and UI Component usage |
| src/Service/Hyva/CompatibilityChecker.php | New orchestrator service that coordinates scanning, filtering, and result formatting |
| src/Console/Command/Hyva/CompatibilityCheckCommand.php | New CLI command with interactive mode, multiple options, and detailed reporting capabilities |
| docs/commands.md | Comprehensive documentation for the new command including usage examples, options, and detected patterns |
| CHANGELOG.md | Documents the new feature with bullet points describing capabilities |
| .github/workflows/magento-compatibility.yml | Adds CI/CD tests for the new command across multiple Magento versions |
| <type name="Magento\Framework\Console\CommandList"> | ||
| <arguments> | ||
| <argument | ||
| name="commands" | ||
| xsi:type="array" | ||
| > | ||
| <item name="mageforge_system_version" | ||
| xsi:type="object" | ||
| > | ||
| OpenForgeProject\MageForge\Console\Command\System\VersionCommand</item> | ||
| <item name="mageforge_system_check" | ||
| xsi:type="object" | ||
| > | ||
| OpenForgeProject\MageForge\Console\Command\System\CheckCommand</item> | ||
| <item name="mageforge_theme_list" | ||
| xsi:type="object" | ||
| > | ||
| OpenForgeProject\MageForge\Console\Command\Theme\ListCommand</item> | ||
| <item name="mageforge_theme_build" | ||
| xsi:type="object" | ||
| > | ||
| OpenForgeProject\MageForge\Console\Command\Theme\BuildCommand</item> | ||
| <item name="mageforge_theme_watch" | ||
| xsi:type="object" | ||
| > | ||
| OpenForgeProject\MageForge\Console\Command\Theme\WatchCommand</item> | ||
| <item name="mageforge_hyva_compatibility_check" | ||
| xsi:type="object" | ||
| > | ||
| OpenForgeProject\MageForge\Console\Command\Hyva\CompatibilityCheckCommand</item> | ||
| </argument> | ||
| </arguments> | ||
| </type> |
Copilot
AI
Jan 12, 2026
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.
Duplicate type definition detected. There are two identical type definitions for "Magento\Framework\Console\CommandList" (lines 6-38 and lines 39-65). This will cause a configuration conflict in Magento's dependency injection system. The command registrations from lines 6-38 should be merged into the existing type definition at lines 39-65, or the duplicate definition should be removed entirely.
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.
@copilot open a new pull request to apply changes based on this feedback
| <type name="Magento\Framework\Console\CommandList"> | ||
| <arguments> | ||
| <argument | ||
| name="commands" | ||
| xsi:type="array" | ||
| > | ||
| <item name="mageforge_system_version" | ||
| xsi:type="object" | ||
| > | ||
| OpenForgeProject\MageForge\Console\Command\System\VersionCommand</item> | ||
| <item name="mageforge_system_check" | ||
| xsi:type="object" | ||
| > | ||
| OpenForgeProject\MageForge\Console\Command\System\CheckCommand</item> | ||
| <item name="mageforge_theme_list" | ||
| xsi:type="object" | ||
| > | ||
| OpenForgeProject\MageForge\Console\Command\Theme\ListCommand</item> | ||
| <item name="mageforge_theme_build" | ||
| xsi:type="object" | ||
| > | ||
| OpenForgeProject\MageForge\Console\Command\Theme\BuildCommand</item> | ||
| <item name="mageforge_theme_watch" | ||
| xsi:type="object" | ||
| > | ||
| OpenForgeProject\MageForge\Console\Command\Theme\WatchCommand</item> | ||
| <item name="mageforge_hyva_compatibility_check" | ||
| xsi:type="object" | ||
| > | ||
| OpenForgeProject\MageForge\Console\Command\Hyva\CompatibilityCheckCommand</item> | ||
| </argument> | ||
| </arguments> | ||
| </type> |
Copilot
AI
Jan 12, 2026
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.
Inconsistent indentation style detected. Lines 6-38 use 6 spaces for indentation while lines 39-88 use 4 spaces. According to Magento Coding Standards and the project guidelines, indentation should be consistent throughout the file using 4 spaces. The first type definition (lines 6-38) should be updated to use 4-space indentation to match the rest of the file.
src/etc/di.xml
Outdated
| <item name="mageforge_hyva_compatibility_check" | ||
| xsi:type="object" | ||
| > | ||
| OpenForgeProject\MageForge\Console\Command\Hyva\CompatibilityCheckCommand</item> |
Copilot
AI
Jan 12, 2026
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 new hyva compatibility check command is registered in the duplicate type definition (line 32-35) but missing from the existing type definition (lines 39-65) which includes the static clean command. When the duplicate is removed, this command registration should be added to the remaining type definition to ensure it's properly registered.
| * Get severity color for console output | ||
| */ | ||
| public function getSeverityColor(string $severity): string | ||
| { | ||
| return match ($severity) { | ||
| self::SEVERITY_CRITICAL => 'red', | ||
| self::SEVERITY_WARNING => 'yellow', | ||
| default => 'white', |
Copilot
AI
Jan 12, 2026
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 getSeverityColor returns color names as strings ('red', 'yellow', 'white'), but these are Symfony Console color names. The method should be documented to clarify that these are Symfony Console color tags. Additionally, the 'white' default color may not be visible on light terminal backgrounds. Consider using a more neutral default like 'gray' or documenting terminal compatibility considerations.
| * Get severity color for console output | |
| */ | |
| public function getSeverityColor(string $severity): string | |
| { | |
| return match ($severity) { | |
| self::SEVERITY_CRITICAL => 'red', | |
| self::SEVERITY_WARNING => 'yellow', | |
| default => 'white', | |
| * Get severity color for console output. | |
| * | |
| * The returned string is a Symfony Console color/style name that can be | |
| * used in console formatting tags (e.g. <fg=red> or style definitions). | |
| * | |
| * - critical => "red" | |
| * - warning => "yellow" | |
| * - default => "gray" (chosen to remain visible on light terminals) | |
| */ | |
| public function getSeverityColor(string $severity): string | |
| { | |
| return match ($severity) { | |
| self::SEVERITY_CRITICAL => 'red', | |
| self::SEVERITY_WARNING => 'yellow', | |
| default => 'gray', |
| private function isHyvaCompatibilityPackage(array $composerData): bool | ||
| { | ||
| // Check if this IS a Hyvä compatibility package | ||
| $packageName = $composerData['name'] ?? ''; | ||
| if (str_starts_with($packageName, 'hyva-themes/') && str_contains($packageName, '-compat')) { | ||
| return true; | ||
| } | ||
|
|
||
| // Check dependencies for Hyvä packages | ||
| $requires = $composerData['require'] ?? []; | ||
| foreach ($requires as $package => $version) { | ||
| if (str_starts_with($package, 'hyva-themes/')) { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| /** | ||
| * Check if module has Hyvä compatibility package (public wrapper) | ||
| */ | ||
| public function hasHyvaCompatibilityPackage(string $modulePath): bool | ||
| { | ||
| $composerPath = $modulePath . '/composer.json'; | ||
|
|
||
| if (!$this->fileDriver->isExists($composerPath)) { | ||
| return false; | ||
| } | ||
|
|
||
| try { | ||
| $content = $this->fileDriver->fileGetContents($composerPath); | ||
| $composerData = json_decode($content, true); | ||
|
|
||
| if (!is_array($composerData)) { | ||
| return false; | ||
| } | ||
|
|
||
| return $this->isHyvaCompatibilityPackage($composerData); | ||
| } catch (\Exception $e) { | ||
| // Log error when debugging to help identify JSON or file read issues | ||
| if (getenv('MAGEFORGE_DEBUG')) { | ||
| error_log(sprintf( | ||
| '[MageForge][ModuleScanner] Failed to read composer.json at "%s": %s', | ||
| $composerPath, | ||
| $e->getMessage() | ||
| )); | ||
| } | ||
| return false; | ||
| } | ||
| } |
Copilot
AI
Jan 12, 2026
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 isHyvaCompatibilityPackage is defined as private but has a public wrapper hasHyvaCompatibilityPackage that simply calls it. This adds unnecessary indirection. Consider either making isHyvaCompatibilityPackage public directly and removing the wrapper, or consolidating the logic. Additionally, the public wrapper has inconsistent error handling - it catches exceptions but the private method doesn't document that it may throw exceptions.
| } catch (\Exception $e) { | ||
| // Skip directories that can't be read, but log details when debugging | ||
| if (getenv('MAGEFORGE_DEBUG')) { | ||
| error_log(sprintf( | ||
| '[MageForge][ModuleScanner] Failed to read directory "%s": %s', | ||
| $directory, | ||
| $e->getMessage() | ||
| )); | ||
| } |
Copilot
AI
Jan 12, 2026
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 error logging uses getenv('MAGEFORGE_DEBUG') directly which is less testable and doesn't follow Magento best practices. Consider using Magento's configuration system or injecting a logger with appropriate log levels instead of custom environment variable checks. This would provide better integration with Magento's logging infrastructure.
| } | ||
|
|
||
| // Display results | ||
| $this->displayResults($results, $displayShowAll || $detailed); |
Copilot
AI
Jan 12, 2026
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.
Inconsistent display logic at lines 196-202. The variable 'displayShowAll' is set based on incompatibleOnly flag, but then at line 205 it's combined with $detailed flag using OR operator. This means if --detailed is used, all modules will be shown regardless of the incompatibleOnly setting, which may not be the intended behavior. The logic should clearly separate filtering (what to scan) from display (what to show in results).
| $this->displayResults($results, $displayShowAll || $detailed); | |
| $this->displayResults($results, $displayShowAll); |
|
|
||
| - `--show-all` / `-a` - Show all modules including compatible ones | ||
| - `--third-party-only` / `-t` - Check only third-party modules (exclude Magento\_\* modules) | ||
| - `--include-vendor` - Include Magento core modules in scan (default: third-party only) |
Copilot
AI
Jan 12, 2026
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.
Documentation inconsistency: The description states "Include Magento core modules (default: third-party only)" at line 209, but looking at the actual command option definition and implementation, the --include-vendor flag controls whether to include vendor directory modules, not specifically Magento core modules. The Magento_* core modules are filtered by the --third-party-only flag. This distinction should be clarified in the documentation to accurately reflect the command's behavior.
| - `--include-vendor` - Include Magento core modules in scan (default: third-party only) | |
| - `--include-vendor` - Include modules from the `vendor/` directory in the scan (by default only non-vendor modules are scanned; Magento\_\* core modules are still controlled by `--third-party-only`) |
| **Options**: | ||
|
|
||
| - `--show-all` / `-a` - Show all modules including compatible ones | ||
| - `--third-party-only` / `-t` - Check only third-party modules (exclude Magento\_\* modules) |
Copilot
AI
Jan 12, 2026
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.
Spelling inconsistency: "Magento_*" at line 208 uses an escaped underscore with backslash, which is correct for some Markdown renderers, but the same pattern at line 230 uses "Magento_*" without escaping. For consistency, both instances should use the same escaping pattern. Most modern Markdown renderers don't require escaping underscores when they're part of literal text, so consider removing the backslash escape for consistency.
| - `--third-party-only` / `-t` - Check only third-party modules (exclude Magento\_\* modules) | |
| - `--third-party-only` / `-t` - Check only third-party modules (exclude Magento_* modules) |
Co-authored-by: Copilot <[email protected]>
No description provided.