feat: add command to copy files from vendor to theme (#133)#133
feat: add command to copy files from vendor to theme (#133)#133
Conversation
dermatz
changed the title
There was a problem hiding this comment.
Pull request overview
Adds a new MageForge CLI command to copy a vendor file into the correct override location inside a Magento theme, with supporting path-mapping logic and CI workflow updates to recognize the new command/alias.
Changes:
- Introduces
mageforge:theme:copy-from-vendor(aliastheme:copy) to copy vendor files into themes (with dry-run + confirmations). - Adds
VendorFileMapperservice andThemeList::getThemeByCode()to support target resolution. - Updates GitHub Actions workflows to validate the new command/alias and bumps Elasticsearch image version in compatibility CI.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
src/etc/di.xml |
Registers the new CLI command in DI so Magento can discover it. |
src/Service/VendorFileMapper.php |
Implements vendor/module path-to-theme override path mapping. |
src/Model/ThemeList.php |
Adds lookup helper to fetch a theme by its code. |
src/Console/Command/Theme/CopyFromVendorCommand.php |
Implements the new CLI command UX and copy execution flow. |
.github/workflows/magento-compatibility.yml |
Updates Elasticsearch service version and CI command/alias checks. |
.github/workflows/functional-tests.yml |
Adds CI checks for the new command/alias presence. |
| putenv('COLUMNS=100'); | ||
| putenv('LINES=40'); | ||
| putenv('TERM=xterm-256color'); |
There was a problem hiding this comment.
fixPromptEnvironment() sets COLUMNS/LINES/TERM but never restores previous values, and it only runs under DDEV_PROJECT. This can leave the user’s terminal env mutated and behaves differently than the existing prompt env handling elsewhere; please store originals and reset after prompting (or centralize the logic similarly to other commands).
| putenv('COLUMNS=100'); | |
| putenv('LINES=40'); | |
| putenv('TERM=xterm-256color'); | |
| $originalColumns = getenv('COLUMNS'); | |
| $originalLines = getenv('LINES'); | |
| $originalTerm = getenv('TERM'); | |
| register_shutdown_function( | |
| static function ( | |
| $originalColumns, | |
| $originalLines, | |
| $originalTerm | |
| ): void { | |
| if ($originalColumns === false) { | |
| putenv('COLUMNS'); | |
| } else { | |
| putenv('COLUMNS=' . $originalColumns); | |
| } | |
| if ($originalLines === false) { | |
| putenv('LINES'); | |
| } else { | |
| putenv('LINES=' . $originalLines); | |
| } | |
| if ($originalTerm === false) { | |
| putenv('TERM'); | |
| } else { | |
| putenv('TERM=' . $originalTerm); | |
| } | |
| }, | |
| $originalColumns, | |
| $originalLines, | |
| $originalTerm | |
| ); | |
| putenv('COLUMNS=100'); | |
| putenv('LINES=40'); | |
| putenv('TERM=xterm-256color'); |
| $themes = $this->themeList->getAllThemes(); | ||
| $options = []; | ||
| foreach ($themes as $theme) { | ||
| $options[$theme->getCode()] = $theme->getCode(); | ||
| } | ||
|
|
||
| if (empty($options)) { | ||
| throw new \RuntimeException('No themes found to copy to.'); | ||
| } | ||
|
|
||
| $this->fixPromptEnvironment(); | ||
|
|
||
| return (string) search( | ||
| label: 'Select target theme', | ||
| options: fn (string $value) => array_filter( | ||
| $options, | ||
| fn ($option) => str_contains(strtolower($option), strtolower($value)) | ||
| ), | ||
| placeholder: 'Search for a theme...' | ||
| ); | ||
| } |
There was a problem hiding this comment.
This command launches an interactive Laravel Prompts search when no theme argument is provided, but it doesn’t guard against non-interactive environments and doesn’t restore TTY/reset prompt env like other commands do. Consider adding a non-interactive fallback (e.g., list themes + usage) and wrapping the prompt with environment setup + Prompt::terminal()->restoreTty() + environment reset (see patterns in BuildCommand/CleanCommand/AbstractCommand).
| if (!mkdir($directory, 0777, true) && !is_dir($directory)) { | ||
| throw new \RuntimeException(sprintf('Directory "%s" was not created', $directory)); | ||
| } | ||
| } | ||
| copy($absoluteSourcePath, $absoluteDestPath); |
There was a problem hiding this comment.
performCopy() uses raw mkdir(..., 0777) and copy() without checking the return value. Please use the Magento file driver (as elsewhere in the repo) or at least validate copy() succeeded and throw a clear error on failure; also consider a less permissive directory mode (e.g., 0755) for created directories.
| if (!mkdir($directory, 0777, true) && !is_dir($directory)) { | |
| throw new \RuntimeException(sprintf('Directory "%s" was not created', $directory)); | |
| } | |
| } | |
| copy($absoluteSourcePath, $absoluteDestPath); | |
| if (!mkdir($directory, 0755, true) && !is_dir($directory)) { | |
| throw new \RuntimeException( | |
| sprintf('Directory "%s" was not created', $directory) | |
| ); | |
| } | |
| } | |
| $copyResult = @copy($absoluteSourcePath, $absoluteDestPath); | |
| if ($copyResult === false) { | |
| throw new \RuntimeException( | |
| sprintf( | |
| 'Failed to copy file from "%s" to "%s".', | |
| $absoluteSourcePath, | |
| $absoluteDestPath | |
| ) | |
| ); | |
| } |
| // Regex search for a segment matching Vendor_Module (e.g. Magento_Catalog). | ||
| // Captures (Group 1): "Vendor_Module" | ||
| if (preg_match('/([A-Z][a-zA-Z0-9]*_[A-Z][a-zA-Z0-9]*)/', $sourcePath, $matches, PREG_OFFSET_CAPTURE)) { |
There was a problem hiding this comment.
The regex pattern /([A-Z][a-zA-Z0-9]*_[A-Z][a-zA-Z0-9]*)/ will match the first occurrence of a Vendor_Module pattern in the path, but if there are multiple such patterns in a path (e.g., a nested vendor structure or when a module name appears in multiple segments), it only captures the first one. Consider if this is the intended behavior or if the pattern should be more specific about where in the path the module name should be found (e.g., require it to be preceded by a path separator or specific directory structure). The comment on line 48 says "Captures (Group 1): 'Vendor_Module'" but doesn't specify that it captures the FIRST occurrence, which could be misleading.
| // Regex search for a segment matching Vendor_Module (e.g. Magento_Catalog). | |
| // Captures (Group 1): "Vendor_Module" | |
| if (preg_match('/([A-Z][a-zA-Z0-9]*_[A-Z][a-zA-Z0-9]*)/', $sourcePath, $matches, PREG_OFFSET_CAPTURE)) { | |
| // Regex search for the first path segment matching Vendor_Module | |
| // (e.g. "/vendor/module/Magento_Catalog/..."). | |
| // Captures (Group 1): the first matching "Vendor_Module" segment in the path. | |
| if (preg_match('#(?:^|[\\/])([A-Z][a-zA-Z0-9]*_[A-Z][a-zA-Z0-9]*)#', $sourcePath, $matches, PREG_OFFSET_CAPTURE)) { |
| throw new RuntimeException("Could not determine target module or theme structure for file: " . $sourcePath); | ||
| } |
There was a problem hiding this comment.
The error message includes the raw $sourcePath which could potentially be a very long absolute path or contain sensitive information. Consider truncating long paths or showing only relevant parts (e.g., relative from vendor/) for better error message clarity. Additionally, the error message could be more helpful by suggesting what patterns are supported (e.g., "Expected path to contain a module with view/frontend or view/base structure, or a Vendor_Module pattern").
| throw new RuntimeException("Could not determine target module or theme structure for file: " . $sourcePath); | |
| } | |
| $displayPath = $this->formatSourcePathForError($sourcePath); | |
| throw new RuntimeException( | |
| 'Could not determine target module or theme structure for file: ' | |
| . $displayPath | |
| . '. Expected path to contain either a registered module root ' | |
| . 'with a view/frontend or view/base directory, or a segment ' | |
| . 'matching the Vendor_Module pattern (for example ' | |
| . 'Magento_Catalog).' | |
| ); | |
| } | |
| /** | |
| * Format a source path for use in error messages by stripping the | |
| * Magento root prefix and truncating very long paths. | |
| */ | |
| private function formatSourcePathForError(string $sourcePath): string | |
| { | |
| $rootPath = rtrim($this->directoryList->getRoot(), '/'); | |
| if (str_starts_with($sourcePath, $rootPath . '/')) { | |
| $sourcePath = substr($sourcePath, strlen($rootPath) + 1); | |
| } | |
| $maxLength = 80; | |
| if (strlen($sourcePath) <= $maxLength) { | |
| return $sourcePath; | |
| } | |
| return '...' . substr($sourcePath, -($maxLength - 3)); | |
| } |
| protected function configure(): void | ||
| { | ||
| $this->setName('mageforge:theme:copy-from-vendor') | ||
| ->setDescription('Copy a file from vendor/ to a specific theme with correct path resolution') | ||
| ->setAliases(['theme:copy']) | ||
| ->addArgument('file', InputArgument::REQUIRED, 'Path to the source file (vendor/...)') | ||
| ->addArgument('theme', InputArgument::OPTIONAL, 'Target theme code (e.g. Magento/luma)') | ||
| ->addOption('dry-run', null, InputOption::VALUE_NONE, 'Preview the copy operation without performing it'); | ||
| } |
There was a problem hiding this comment.
The configure method is missing PHPDoc documentation. All other commands in the codebase have PHPDoc comments for their configure methods, following Magento's coding standards. For example, BuildCommand.php:47-49 has "{@inheritdoc}" and CleanCommand.php:46-48 has "{@inheritdoc}". This ensures consistency in documentation across the codebase.
| protected function executeCommand(InputInterface $input, OutputInterface $output): int | ||
| { | ||
| try { | ||
| $sourceFileArg = $input->getArgument('file'); | ||
| $isDryRun = $input->getOption('dry-run'); | ||
| $absoluteSourcePath = $this->getAbsoluteSourcePath($sourceFileArg); | ||
|
|
||
| // Update sourceFileArg if it was normalized to relative path | ||
| $rootPath = $this->directoryList->getRoot(); | ||
| $sourceFile = str_starts_with($absoluteSourcePath, $rootPath . '/') | ||
| ? substr($absoluteSourcePath, strlen($rootPath) + 1) | ||
| : $sourceFileArg; | ||
|
|
||
| $themeCode = $this->getThemeCode($input); | ||
| $themePath = $this->getThemePath($themeCode); | ||
|
|
||
| $destinationPath = $this->vendorFileMapper->mapToThemePath($sourceFile, $themePath); | ||
| $absoluteDestPath = $this->getAbsoluteDestPath($destinationPath, $rootPath); | ||
|
|
||
| if ($isDryRun) { | ||
| $this->showDryRunPreview($sourceFile, $absoluteDestPath, $rootPath); | ||
| return Cli::RETURN_SUCCESS; | ||
| } | ||
|
|
||
| if (!$this->confirmCopy($sourceFile, $absoluteDestPath, $rootPath)) { | ||
| return Cli::RETURN_SUCCESS; | ||
| } | ||
|
|
||
| $this->performCopy($absoluteSourcePath, $absoluteDestPath); | ||
| $this->io->success("File copied successfully."); | ||
|
|
||
| return Cli::RETURN_SUCCESS; | ||
| } catch (\Exception $e) { | ||
| $this->io->error($e->getMessage()); | ||
| return Cli::RETURN_FAILURE; | ||
| } | ||
| } |
There was a problem hiding this comment.
The executeCommand method is missing PHPDoc documentation. All other commands in the codebase have PHPDoc comments for executeCommand, following Magento's coding standards. For example, BuildCommand.php:62-64 has "{@inheritdoc}" and other commands follow the same pattern. This ensures consistency across the codebase.
src/Service/VendorFileMapper.php
Outdated
| public function mapToThemePath(string $sourcePath, string $themePath): string | ||
| { | ||
| // 1. Normalize: Ensure $sourcePath is relative from Magento Root if it's absolute | ||
| $rootPath = rtrim($this->directoryList->getRoot(), '/'); | ||
| if (str_starts_with($sourcePath, $rootPath . '/')) { | ||
| $sourcePath = substr($sourcePath, strlen($rootPath) + 1); | ||
| } | ||
|
|
||
| // 2. Detect "Standard Module" Pattern (Priority 1) - Best for Local Modules & Composer Packages | ||
| $modules = $this->componentRegistrar->getPaths(ComponentRegistrar::MODULE); | ||
| foreach ($modules as $moduleName => $path) { | ||
| // Normalize module path relative to root | ||
| if (str_starts_with($path, $rootPath . '/')) { | ||
| $path = substr($path, strlen($rootPath) + 1); | ||
| } | ||
|
|
||
| // Check if source starts with this module path | ||
| if (str_starts_with($sourcePath, $path . '/')) { | ||
| $pathInsideModule = substr($sourcePath, strlen($path) + 1); | ||
|
|
||
| // Remove view/frontend/ or view/base/ from the path | ||
| $cleanPath = (string) preg_replace('#^view/(frontend|base)/#', '', $pathInsideModule); | ||
|
|
||
| return rtrim($themePath, '/') . '/' . $moduleName . '/' . ltrim($cleanPath, '/'); | ||
| } | ||
| } | ||
|
|
||
| // 3. Detect "Nested Module" Pattern (Priority 2) - Works for Hyva Compat & Vendor Themes | ||
| // Regex search for a segment matching Vendor_Module (e.g. Magento_Catalog). | ||
| // Captures (Group 1): "Vendor_Module" | ||
| if (preg_match('/([A-Z][a-zA-Z0-9]*_[A-Z][a-zA-Z0-9]*)/', $sourcePath, $matches, PREG_OFFSET_CAPTURE)) { | ||
| $offset = $matches[1][1]; | ||
|
|
||
| // Extract from Vendor_Module onwards (e.g. "Mollie_Payment/templates/file.phtml") | ||
| $relativePath = substr($sourcePath, $offset); | ||
|
|
||
| return rtrim($themePath, '/') . '/' . ltrim($relativePath, '/'); | ||
| } | ||
|
|
||
| // 4. Fallback | ||
| throw new RuntimeException("Could not determine target module or theme structure for file: " . $sourcePath); | ||
| } |
There was a problem hiding this comment.
The mapToThemePath method is missing PHPDoc documentation. All other service methods in the codebase have comprehensive PHPDoc comments documenting parameters, return types, exceptions thrown, and algorithm details. For example, ThemeSuggester::findSimilarThemes has detailed documentation (lines 31-41). Given this method's complexity with multiple pattern detection strategies, documentation is especially important for maintainability.
| if (getenv('DDEV_PROJECT')) { | ||
| putenv('COLUMNS=100'); | ||
| putenv('LINES=40'); | ||
| putenv('TERM=xterm-256color'); | ||
| } |
There was a problem hiding this comment.
The fixPromptEnvironment method in this command uses a different pattern than the rest of the codebase. Other commands in this codebase use setPromptEnvironment() and resetPromptEnvironment() methods (defined in AbstractCommand or duplicated in the command itself) that properly store and restore environment variables with try-catch-finally blocks. This custom fixPromptEnvironment method only checks for DDEV_PROJECT and directly uses putenv() without proper cleanup or error handling, which deviates from the established pattern. Consider using the AbstractCommand.setPromptEnvironment() method instead (if accessible), or implementing the full setPromptEnvironment/resetPromptEnvironment pattern with proper try-catch-finally blocks as seen in BuildCommand, CleanCommand, and CompatibilityCheckCommand (examples: src/Console/Command/AbstractCommand.php:161-173, src/Console/Command/Theme/BuildCommand.php:82-106, src/Console/Command/Theme/CleanCommand.php:193-219).
| if (getenv('DDEV_PROJECT')) { | |
| putenv('COLUMNS=100'); | |
| putenv('LINES=40'); | |
| putenv('TERM=xterm-256color'); | |
| } | |
| $this->setPromptEnvironment(); |
src/Service/VendorFileMapper.php
Outdated
| class VendorFileMapper | ||
| { | ||
| public function __construct( | ||
| private readonly ComponentRegistrarInterface $componentRegistrar, | ||
| private readonly DirectoryList $directoryList | ||
| ) {} |
There was a problem hiding this comment.
The VendorFileMapper class is missing a class-level PHPDoc comment. All other service classes in the codebase have PHPDoc comments describing their purpose, such as ThemeSuggester.php:9-13 ("Service for suggesting similar theme names...") and ThemeCleaner.php. This is important for maintainability and follows Magento's coding standards.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
dermatz
force-pushed
the
110-feature-request-copy-vendor-files-to-theme
branch
from
ed8af66 to
ca088d1
Compare
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| if (str_starts_with($sourceFile, '/')) { | ||
| $absoluteSourcePath = $sourceFile; | ||
| } else { | ||
| $absoluteSourcePath = $rootPath . '/' . $sourceFile; | ||
| } | ||
|
|
||
| if (!file_exists($absoluteSourcePath)) { | ||
| throw new \RuntimeException("Source file not found: $absoluteSourcePath"); | ||
| } | ||
|
|
||
| return $absoluteSourcePath; |
There was a problem hiding this comment.
The source path handling allows absolute paths (e.g. /etc/passwd) and relative traversal (../) and only checks file_exists(). Since this command is explicitly "copy-from-vendor", it should validate that the resolved realpath is inside <magento_root>/vendor (and reject anything outside) to prevent copying arbitrary files into the theme.
| if (str_starts_with($sourceFile, '/')) { | |
| $absoluteSourcePath = $sourceFile; | |
| } else { | |
| $absoluteSourcePath = $rootPath . '/' . $sourceFile; | |
| } | |
| if (!file_exists($absoluteSourcePath)) { | |
| throw new \RuntimeException("Source file not found: $absoluteSourcePath"); | |
| } | |
| return $absoluteSourcePath; | |
| $vendorRoot = $rootPath . '/vendor'; | |
| if (str_starts_with($sourceFile, '/')) { | |
| $candidatePath = $sourceFile; | |
| } else { | |
| $candidatePath = $rootPath . '/' . ltrim($sourceFile, '/'); | |
| } | |
| $realSourcePath = realpath($candidatePath); | |
| if ($realSourcePath === false || !is_file($realSourcePath)) { | |
| throw new \RuntimeException( | |
| "Source file not found or is not a file: $candidatePath" | |
| ); | |
| } | |
| $vendorRootReal = realpath($vendorRoot) ?: $vendorRoot; | |
| $vendorRootReal = rtrim($vendorRootReal, DIRECTORY_SEPARATOR); | |
| $isInsideVendor = str_starts_with( | |
| $realSourcePath, | |
| $vendorRootReal . DIRECTORY_SEPARATOR | |
| ); | |
| if (!$isInsideVendor) { | |
| throw new \RuntimeException( | |
| 'Source file must be inside the vendor directory.' | |
| ); | |
| } | |
| return $realSourcePath; |
| $this->fixPromptEnvironment(); | ||
|
|
||
| return (string) search( | ||
| label: 'Select target theme', | ||
| options: fn (string $value) => array_filter( | ||
| $options, | ||
| fn ($option) => str_contains(strtolower($option), strtolower($value)) | ||
| ), | ||
| placeholder: 'Search for a theme...' | ||
| ); |
There was a problem hiding this comment.
Interactive theme selection uses Laravel Prompts search() without checking isInteractiveTerminal() and without the standard setPromptEnvironment()/resetPromptEnvironment() + TTY restore pattern used elsewhere. This can hang or leave the terminal in a bad state. Consider following the AbstractCommand prompt pattern with try/finally and provide a non-interactive fallback (e.g., error + list themes) when no theme argument is provided.
| $this->fixPromptEnvironment(); | |
| return (string) search( | |
| label: 'Select target theme', | |
| options: fn (string $value) => array_filter( | |
| $options, | |
| fn ($option) => str_contains(strtolower($option), strtolower($value)) | |
| ), | |
| placeholder: 'Search for a theme...' | |
| ); | |
| if (!$this->isInteractiveTerminal()) { | |
| $this->io->error( | |
| 'No theme argument provided and interactive selection is ' | |
| . 'not available in this environment.' | |
| ); | |
| $this->io->writeln('Available themes:'); | |
| foreach ($options as $option) { | |
| $this->io->writeln(' - ' . $option); | |
| } | |
| throw new \RuntimeException( | |
| 'Interactive theme selection is not available. Please ' | |
| . 'rerun the command with the "theme" argument.' | |
| ); | |
| } | |
| $this->setPromptEnvironment(); | |
| try { | |
| $selectedTheme = search( | |
| label: 'Select target theme', | |
| options: fn (string $value) => array_filter( | |
| $options, | |
| fn ($option) => str_contains( | |
| strtolower($option), | |
| strtolower($value) | |
| ) | |
| ), | |
| placeholder: 'Search for a theme...' | |
| ); | |
| } finally { | |
| $this->resetPromptEnvironment(); | |
| } | |
| return (string) $selectedTheme; |
| if (getenv('DDEV_PROJECT')) { | ||
| putenv('COLUMNS=100'); | ||
| putenv('LINES=40'); | ||
| putenv('TERM=xterm-256color'); | ||
| } |
There was a problem hiding this comment.
fixPromptEnvironment() duplicates AbstractCommand::setPromptEnvironment() logic, only applies under DDEV, and never restores original env values. Prefer reusing setPromptEnvironment()/resetPromptEnvironment() from AbstractCommand so prompt behavior is consistent and environment is always reset.
| if (getenv('DDEV_PROJECT')) { | |
| putenv('COLUMNS=100'); | |
| putenv('LINES=40'); | |
| putenv('TERM=xterm-256color'); | |
| } | |
| // Use shared prompt environment handling from AbstractCommand so that | |
| // behavior is consistent across commands and environment values are | |
| // always restored after execution. | |
| $this->setPromptEnvironment(); | |
| register_shutdown_function([$this, 'resetPromptEnvironment']); |
| $directory = dirname($absoluteDestPath); | ||
| if (!is_dir($directory)) { | ||
| if (!mkdir($directory, 0777, true) && !is_dir($directory)) { | ||
| throw new \RuntimeException(sprintf('Directory "%s" was not created', $directory)); | ||
| } | ||
| } | ||
| copy($absoluteSourcePath, $absoluteDestPath); |
There was a problem hiding this comment.
performCopy() ignores the return value of copy(). If the copy fails (permissions, invalid destination, etc.) the command will still print "File copied successfully.". Please check the result of copy() and throw/return a failure when it fails (and consider validating the destination is not a directory).
| $directory = dirname($absoluteDestPath); | |
| if (!is_dir($directory)) { | |
| if (!mkdir($directory, 0777, true) && !is_dir($directory)) { | |
| throw new \RuntimeException(sprintf('Directory "%s" was not created', $directory)); | |
| } | |
| } | |
| copy($absoluteSourcePath, $absoluteDestPath); | |
| if (is_dir($absoluteDestPath)) { | |
| throw new \RuntimeException( | |
| sprintf( | |
| 'Destination path "%s" is a directory and cannot be used as a file target', | |
| $absoluteDestPath | |
| ) | |
| ); | |
| } | |
| $directory = dirname($absoluteDestPath); | |
| if (!is_dir($directory)) { | |
| if (!mkdir($directory, 0777, true) && !is_dir($directory)) { | |
| throw new \RuntimeException( | |
| sprintf('Directory "%s" was not created', $directory) | |
| ); | |
| } | |
| } | |
| if (!copy($absoluteSourcePath, $absoluteDestPath)) { | |
| throw new \RuntimeException( | |
| sprintf( | |
| 'Failed to copy file from "%s" to "%s".', | |
| $absoluteSourcePath, | |
| $absoluteDestPath | |
| ) | |
| ); | |
| } |
| elasticsearch: | ||
| image: elasticsearch:7.17.25 | ||
| env: | ||
| discovery.type: single-node | ||
| plugins.security.disabled: true | ||
| OPENSEARCH_JAVA_OPTS: -Xms512m -Xmx512m | ||
| options: --health-cmd="curl http://localhost:9200/_cluster/health" --health-interval=10s --health-timeout=5s --health-retries=10 | ||
| xpack.security.enabled: false | ||
| ES_JAVA_OPTS: "-Xms512m -Xmx512m" | ||
| ports: | ||
| - 9200:9200 | ||
| options: --health-cmd="curl -s http://localhost:9200/_cluster/health" --health-interval=10s --health-timeout=5s --health-retries=10 |
There was a problem hiding this comment.
The job/matrix still configures Magento with --search-engine=opensearch and opensearch host/port, but the service container was switched to elasticsearch:7.17.25. This mismatch is very likely to break the workflow. Either keep the OpenSearch service for opensearch runs, or change the matrix/search-engine-name and setup:install flags to match Elasticsearch (and adjust host/port option names accordingly).
|
|
||
| use Magento\Framework\Console\Cli; | ||
| use Magento\Framework\Filesystem\DirectoryList; | ||
| use Magento\Framework\Component\ComponentRegistrar; |
There was a problem hiding this comment.
Unused imports: InvalidArgumentException and Laravel\Prompts\SearchPrompt are imported but never used in this command. Please remove them to keep the file clean and avoid PHPCS/phpstan warnings.
| use Magento\Framework\Component\ComponentRegistrar; |
| $this->setName('mageforge:theme:copy-from-vendor') | ||
| ->setDescription('Copy a file from vendor/ to a specific theme with correct path resolution') | ||
| ->setAliases(['theme:copy']) | ||
| ->addArgument('file', InputArgument::REQUIRED, 'Path to the source file (vendor/...)') | ||
| ->addArgument('theme', InputArgument::OPTIONAL, 'Target theme code (e.g. Magento/luma)') | ||
| ->addOption('dry-run', null, InputOption::VALUE_NONE, 'Preview the copy operation without performing it'); | ||
| } | ||
|
|
There was a problem hiding this comment.
configure() deviates from the established command conventions: other commands use $this->getCommandName(...) for setName() and include a PHPDoc block with {@inheritdoc}. Aligning this keeps naming consistent and avoids duplicated hard-coded command strings.
src/Service/VendorFileMapper.php
Outdated
| // Check if source starts with this module path | ||
| if (str_starts_with($sourcePath, $path . '/')) { | ||
| $pathInsideModule = substr($sourcePath, strlen($path) + 1); | ||
|
|
||
| // Remove view/frontend/ or view/base/ from the path | ||
| $cleanPath = (string) preg_replace( | ||
| '#^view/(frontend|base)/#', | ||
| '', | ||
| $pathInsideModule |
There was a problem hiding this comment.
mapToThemePath() always maps any file under a module path into //..., even if it is not under view/(frontend|base). This can produce incorrect theme override paths (e.g., for etc/ files) and it also doesn't handle adminhtml themes (view/adminhtml) at all. Consider validating the area segment based on the target theme area and only mapping supported view/* paths, otherwise throw a clear exception.
github-actions
bot
added
the
documentation
|
|
||
| $this->io->note("No files were modified (dry-run mode)."); | ||
| } |
There was a problem hiding this comment.
Incorrect indentation - the line uses 5 spaces followed by tab characters instead of the standard 4 spaces used throughout the codebase. This violates the PSR coding standards followed by the project.
| $this->io->note("No files were modified (dry-run mode)."); | |
| } | |
| putenv('COLUMNS=100'); | |
| putenv('LINES=40'); | |
| putenv('TERM=xterm-256color'); |
| echo "Test dry-run without required arguments (expect validation failure but command should execute):" | ||
| bin/magento mageforge:theme:copy-from-vendor --dry-run || echo "Expected failure - missing or invalid arguments for copy-from-vendor" | ||
|
|
||
| echo "Test alias dry-run without required arguments (expect validation failure but alias should execute):" | ||
| bin/magento theme:copy --dry-run || echo "Expected failure - missing or invalid arguments for theme:copy alias" |
There was a problem hiding this comment.
The test commands at lines 168-172 will fail because the 'file' argument is required (InputArgument::REQUIRED), but the test runs the command with only the --dry-run option and no file argument. Magento's Symfony Console will throw an error before executeCommand() is reached. The test should either provide a valid file path or expect the specific validation error message.
| echo "Test dry-run without required arguments (expect validation failure but command should execute):" | |
| bin/magento mageforge:theme:copy-from-vendor --dry-run || echo "Expected failure - missing or invalid arguments for copy-from-vendor" | |
| echo "Test alias dry-run without required arguments (expect validation failure but alias should execute):" | |
| bin/magento theme:copy --dry-run || echo "Expected failure - missing or invalid arguments for theme:copy alias" | |
| echo "Test dry-run with placeholder file argument (command should execute without performing copy):" | |
| bin/magento mageforge:theme:copy-from-vendor dummy-file --dry-run || echo "Expected failure - missing or invalid configuration for copy-from-vendor" | |
| echo "Test alias dry-run with placeholder file argument (alias should execute without performing copy):" | |
| bin/magento theme:copy dummy-file --dry-run || echo "Expected failure - missing or invalid configuration for theme:copy alias" |
| // Captures (Group 1): "Vendor_Module" | ||
| if (preg_match('/([A-Z][a-zA-Z0-9]*_[A-Z][a-zA-Z0-9]*)/', $sourcePath, $matches, PREG_OFFSET_CAPTURE)) { | ||
| $offset = $matches[1][1]; | ||
|
|
||
| // Extract from Vendor_Module onwards (e.g. "Mollie_Payment/templates/file.phtml") | ||
| $relativePath = substr($sourcePath, $offset); | ||
|
|
||
| // Validate that this path contains a valid view area | ||
| // Extract the part after Vendor_Module to check | ||
| $parts = explode('/', $relativePath, 3); | ||
| if (count($parts) >= 3 && $parts[1] === 'view') { | ||
| // Format: Vendor_Module/view/{area}/... | ||
| $area = $parts[2]; | ||
| if (!$this->isAreaCompatible($area, $themeArea)) { | ||
| throw new RuntimeException( | ||
| sprintf( | ||
| "Cannot map file from area '%s' to %s theme. File: %s", | ||
| $area, | ||
| $themeArea, | ||
| $sourcePath | ||
| ) | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| return rtrim($themePath, '/') . '/' . ltrim($relativePath, '/'); |
There was a problem hiding this comment.
The nested module pattern (line 65-90) lacks proper validation when a Vendor_Module pattern is found but the path doesn't contain a 'view' directory structure. If the regex matches but the validation at line 74 fails (count($parts) < 3 or $parts[1] !== 'view'), the code still proceeds to return the path at line 89 without verifying it's under a valid view directory. This could lead to incorrect path mapping for non-view files that happen to have a Vendor_Module pattern in their path.
| $relativePath = substr($sourcePath, $offset); | ||
|
|
||
| // Validate that this path contains a valid view area | ||
| // Extract the part after Vendor_Module to check |
There was a problem hiding this comment.
The explode limit of 3 at line 73 is incorrect. With explode('/', $relativePath, 3), for a path like "Magento_Catalog/view/frontend/templates/test.phtml", the result is ["Magento_Catalog", "view", "frontend/templates/test.phtml"]. This means $parts[2] contains "frontend/templates/test.phtml", not just "frontend". The area extraction at line 76 will fail or produce incorrect results. The limit should be 4 to get ["Magento_Catalog", "view", "frontend", "templates/test.phtml"], or the area should be extracted differently (e.g., by splitting $parts[2] on '/').
| // Extract the part after Vendor_Module to check | |
| $parts = explode('/', $relativePath, 4); |
| use Symfony\Component\Console\Output\OutputInterface; | ||
|
|
||
| use function Laravel\Prompts\search; | ||
|
|
There was a problem hiding this comment.
Missing class-level PHPDoc comment. All other command classes in the codebase have a PHPDoc block describing their purpose (e.g., BuildCommand has "Command for building Magento themes"). This documentation is important for understanding the command's purpose.
| /** | |
| * Command for copying files from the vendor directory into Magento themes | |
| * with correct path resolution. | |
| */ |
| use Magento\Framework\Component\ComponentRegistrarInterface; | ||
| use Magento\Framework\Filesystem\DirectoryList; | ||
| use RuntimeException; | ||
|
|
There was a problem hiding this comment.
Missing class-level PHPDoc comment. All other service classes in the codebase have a PHPDoc block describing their purpose (e.g., ThemeSuggester has detailed documentation about its functionality). This documentation is important for understanding the service's purpose and behavior.
| /** | |
| * Service responsible for mapping Magento vendor module view files | |
| * to their corresponding theme override paths. | |
| * | |
| * This mapper resolves the originating module for a given source file, | |
| * validates its location under the appropriate `view/{area}/` directory, | |
| * checks area compatibility (e.g. base, frontend, adminhtml), and | |
| * constructs the correct path within a target theme where the file | |
| * can be overridden. | |
| */ |
| $this->fixPromptEnvironment(); | ||
|
|
||
| return (string) search( | ||
| label: 'Select target theme', | ||
| options: fn (string $value) => array_filter( | ||
| $options, | ||
| fn ($option) => str_contains(strtolower($option), strtolower($value)) |
There was a problem hiding this comment.
The command uses Laravel Prompts search() at line 110 without checking if the terminal is interactive (using isInteractiveTerminal()). All other commands in the codebase check if the terminal is interactive before using Laravel Prompts and provide fallback behavior for non-interactive environments like CI. This will cause the command to fail in CI environments.
| $this->fixPromptEnvironment(); | |
| return (string) search( | |
| label: 'Select target theme', | |
| options: fn (string $value) => array_filter( | |
| $options, | |
| fn ($option) => str_contains(strtolower($option), strtolower($value)) | |
| if (!$this->isInteractiveTerminal()) { | |
| if (count($options) === 1) { | |
| /** @var string $singleThemeCode */ | |
| $singleThemeCode = array_key_first($options); | |
| return $singleThemeCode; | |
| } | |
| throw new \RuntimeException( | |
| 'Non-interactive mode: please provide the target theme code ' . | |
| 'as the "theme" argument.' | |
| ); | |
| } | |
| $this->fixPromptEnvironment(); | |
| return (string) search( | |
| label: 'Select target theme', | |
| options: fn (string $value) => array_filter( | |
| $options, | |
| fn ($option) => str_contains( | |
| strtolower($option), | |
| strtolower($value) | |
| ) |
| ) { | ||
| parent::__construct(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Missing PHPDoc comment with {@inheritdoc} tag for the configure method. Based on the codebase conventions, all configure methods in command classes should have a PHPDoc comment with the {@inheritdoc} tag (see BuildCommand.php:41-43, CleanCommand.php:39-41, VersionCommand.php:31-33).
| /** | |
| * {@inheritdoc} | |
| */ |
1 participant
This pull request introduces a new CLI command for copying files from the
vendor/directory into a Magento theme, along with related updates to the codebase and CI workflows. The main focus is on improving developer productivity by simplifying the process of customizing vendor files within themes. The changes also update test workflows to accommodate the new command and its aliases, and update the Elasticsearch version used in CI.New Command and Supporting Functionality:
mageforge:theme:copy-from-vendorcommand (with aliastheme:copy) to copy files fromvendor/to a specified theme, supporting dry-run previews, interactive theme selection, and overwrite confirmation. (src/Console/Command/Theme/CopyFromVendorCommand.php)VendorFileMapperservice to resolve source vendor file paths to the correct target path within a theme, handling both nested and standard module patterns. (src/Service/VendorFileMapper.php)getThemeByCodemethod toThemeListto retrieve a theme by its code, supporting the new command's logic. (src/Model/ThemeList.php)src/etc/di.xml)Continuous Integration and Workflow Updates:
copy-from-vendorcommand and its alias in test steps. (.github/workflows/functional-tests.yml,.github/workflows/magento-compatibility.yml) [1] [2] [3] [4] [5] [6].github/workflows/magento-compatibility.yml)These changes streamline vendor file customization for theme development and ensure the new functionality is fully covered by automated testing.