Skip to content

Address code review feedback: fix duplicate config, improve regex patterns, enhance security #65

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.

Sign up for GitHub

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

Jump to bottom
Merged
dermatz merged 4 commits into from
Jan 12, 2026
Merged

Address code review feedback: fix duplicate config, improve regex patterns, enhance security #65

dermatz merged 4 commits into from
Jan 12, 2026
+50 −79

Conversation

Copy link
Contributor

Copilot AI commented Jan 12, 2026 •
edited
Loading

Addresses 10 code review comments from PR #59, focusing on configuration cleanup, pattern matching accuracy, and security improvements.

Configuration & Code Structure

  • di.xml: Remove duplicate CommandList configuration block (lines 6-38)
  • ModuleScanner: Merge private isHyvaCompatibilityPackage() into public wrapper to eliminate unnecessary indirection

Regex Pattern Improvements

  • RequireJS detection: Fix pattern to properly match mage/ module references in define()/require() calls
  • jQuery detection: Replace overly broad \$\(.*\)\..*\(/ with specific method matching (click, on, ready, etc.) to reduce false positives
  • XML block removal: Add word boundaries and multiline support: /<referenceBlock\b[^>]*\bremove\s*=\s*"true"[^>]*>/s

Security & Environment Handling

  • TTY detection: Replace shell_exec('stty -g') with stream_isatty(STDIN) / posix_isatty(STDIN) fallback chain
  • Environment variables: Use $_SERVER[$name] instead of putenv() to avoid modifying global process environment
  • Error handling: Replace @ suppression with explicit try-catch blocks

Interactive Mode Alignment

  • Remove non-existent "incompatible-only" option from interactive menu
  • Update descriptions to match actual flag behavior ("Include vendor modules" vs "Include Magento core")
  • Clarify vendor filtering logic: $excludeVendor = !$includeVendor

Documentation

  • Fix escaped underscore in Markdown: Magento_* (was Magento\_\*)

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Copilot AI mentioned this pull request Jan 12, 2026
Merged
Copilot AI and others added 3 commits January 12, 2026 08:47
@dermatz
Apply code review feedback: fix duplicate di.xml, improve regex patte…
9a14389 
…rns, safer environment handling

Co-authored-by: dermatz <6103201+dermatz@users.noreply.github.com>
@dermatz
Replace error suppression with explicit exception handling in TTY det…
27d022c 
…ection

Co-authored-by: dermatz <6103201+dermatz@users.noreply.github.com>
@dermatz
Fix RequireJS pattern regex to properly match module references
19b7f07 
Co-authored-by: dermatz <6103201+dermatz@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Jan 12, 2026

Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • https://api.github.com/repos/magento/magento-coding-standard/zipball/f24dd98f46a98cd7081e4648b19299c27db35264
    • Triggering command: /usr/bin/php8.3 /usr/bin/php8.3 -n -c /tmp/Ijb4Bw /usr/bin/composer create-project magento/magento-coding-standard --stability=dev /tmp/magento-coding-standard (http block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot AI changed the title [WIP] Add Hyvä compatibility checker command Address code review feedback: fix duplicate config, improve regex patterns, enhance security Jan 12, 2026
Copilot AI requested a review from dermatz January 12, 2026 08:50
@dermatz dermatz marked this pull request as ready for review January 12, 2026 08:54
@dermatz dermatz merged commit f77c7ac into feature-hyva-compat-checker Jan 12, 2026
2 checks passed
@dermatz dermatz deleted the copilot/sub-pr-59-another-one branch January 12, 2026 08:54
dermatz added a commit that referenced this pull request Jan 12, 2026
@dermatz
add Hyvä compatibility checker command (#59)
2a05707 
* Add Hyvä compatibility checker command

- 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

* Fix: Default scan now includes third-party modules

- 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

* Add interactive menu with Laravel Prompts

- 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

* Docs: Add interactive mode documentation

* Add 'incompatible only' option to interactive menu

- 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

* Remove pre-selection from 'incompatible only' option

* Update src/Service/Hyva/ModuleScanner.php

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Update CHANGELOG for Hyvä compatibility checker

* ✨ feat: update CHANGELOG and improve di.xml formatting

* Update src/Service/Hyva/IncompatibilityDetector.php

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Add actual execution tests for Hyvä compatibility checker in CI (#60)

* Initial plan

* Add actual execution tests for Hyvä compatibility checker command

Co-authored-by: dermatz <6103201+dermatz@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: dermatz <6103201+dermatz@users.noreply.github.com>
Co-authored-by: Mathias Elle <m.elle@dermatz.de>

* Clarify exit code behavior in Hyvä compatibility checker documentation (#61)

* Initial plan

* Clarify exit code documentation for Hyvä compatibility checker

Co-authored-by: dermatz <6103201+dermatz@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: dermatz <6103201+dermatz@users.noreply.github.com>
Co-authored-by: Mathias Elle <m.elle@dermatz.de>

* Update src/Console/Command/Hyva/CompatibilityCheckCommand.php

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Initial plan (#64)

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: Mathias Elle <m.elle@dermatz.de>

* Update src/Service/Hyva/IncompatibilityDetector.php

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* 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 <6103201+dermatz@users.noreply.github.com>

* Replace error suppression with explicit exception handling in TTY detection

Co-authored-by: dermatz <6103201+dermatz@users.noreply.github.com>

* Fix RequireJS pattern regex to properly match module references

Co-authored-by: dermatz <6103201+dermatz@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: dermatz <6103201+dermatz@users.noreply.github.com>

---------

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com>
dermatz added a commit that referenced this pull request Jan 12, 2026
@dermatz
Revert "Address code review feedback: fix duplicate config, improve r…
4bf1568 
…egex patterns, enhance security (#65)"

This reverts commit f77c7ac.
dermatz added a commit that referenced this pull request Jan 12, 2026
@dermatz
Feature hyva compat checker (#66)
8d92503 
* Add Hyvä compatibility checker command

- 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

* Fix: Default scan now includes third-party modules

- 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

* Add interactive menu with Laravel Prompts

- 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

* Docs: Add interactive mode documentation

* Add 'incompatible only' option to interactive menu

- 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

* Remove pre-selection from 'incompatible only' option

* Update src/Service/Hyva/ModuleScanner.php

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Update CHANGELOG for Hyvä compatibility checker

* ✨ feat: update CHANGELOG and improve di.xml formatting

* Update src/Service/Hyva/IncompatibilityDetector.php

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Add actual execution tests for Hyvä compatibility checker in CI (#60)

* Initial plan

* Add actual execution tests for Hyvä compatibility checker command

Co-authored-by: dermatz <6103201+dermatz@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: dermatz <6103201+dermatz@users.noreply.github.com>
Co-authored-by: Mathias Elle <m.elle@dermatz.de>

* Clarify exit code behavior in Hyvä compatibility checker documentation (#61)

* Initial plan

* Clarify exit code documentation for Hyvä compatibility checker

Co-authored-by: dermatz <6103201+dermatz@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: dermatz <6103201+dermatz@users.noreply.github.com>
Co-authored-by: Mathias Elle <m.elle@dermatz.de>

* Update src/Console/Command/Hyva/CompatibilityCheckCommand.php

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Initial plan (#64)

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: Mathias Elle <m.elle@dermatz.de>

* Update src/Service/Hyva/IncompatibilityDetector.php

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* 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 <6103201+dermatz@users.noreply.github.com>

* Replace error suppression with explicit exception handling in TTY detection

Co-authored-by: dermatz <6103201+dermatz@users.noreply.github.com>

* Fix RequireJS pattern regex to properly match module references

Co-authored-by: dermatz <6103201+dermatz@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: dermatz <6103201+dermatz@users.noreply.github.com>

* Revert "Address code review feedback: fix duplicate config, improve regex patterns, enhance security (#65)"

This reverts commit f77c7ac.

* Update docs/commands.md

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* ✨ feat: register Hyvä compatibility check command in di.xml

* ♻️ refactor: improve compatibility check command options and logic

---------

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dermatz dermatz Awaiting requested review from dermatz

Assignees

@dermatz dermatz

Copilot code review Copilot

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@dermatz