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. |
| $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).
| // 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.
| 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(); |
dermatz
force-pushed
the
110-feature-request-copy-vendor-files-to-theme
branch
from
ed8af66 to
ca088d1
Compare
| 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; |
| 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.
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'); |
| // 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} | |
| */ |
| $this->vendorFileMapper = new VendorFileMapper( | ||
| $this->componentRegistrar, | ||
| $this->directoryList | ||
| ); |
There was a problem hiding this comment.
The VendorFileMapper constructor requires three parameters (ComponentRegistrarInterface, DirectoryList, and ObjectManagerInterface), but the test setUp method only provides two mocks. This will cause all tests to fail with a fatal error. Add a mock for ObjectManagerInterface as the third parameter.
| use Magento\Framework\Filesystem\DirectoryList; | ||
| use RuntimeException; | ||
|
|
||
| class VendorFileMapper |
There was a problem hiding this comment.
Missing class-level PHPDoc comment. According to the coding guidelines (ID: 1000000) and established conventions (see src/Service/ThemeSuggester.php:9-13), all service classes require class-level PHPDoc comments describing their purpose.
| 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.
Laravel Prompts usage missing try-catch-finally block with restoreTty() call. According to established patterns (see src/Console/Command/Theme/CleanCommand.php:186-212), all Laravel Prompts function calls must be wrapped in try-catch-finally blocks that call restoreTty() and resetPromptEnvironment() to prevent broken terminals in CI/production environments. The search() call here lacks this error handling pattern.
| 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...' | |
| ); | |
| try { | |
| 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...' | |
| ); | |
| } catch (\Throwable $exception) { | |
| throw $exception; | |
| } finally { | |
| $this->restoreTty(); | |
| $this->resetPromptEnvironment(); | |
| } |
| ) { | ||
| parent::__construct(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Missing PHPDoc comment with {@inheritdoc} tag for the configure() method. According to established conventions (see src/Console/Command/Theme/BuildCommand.php:41-43, src/Console/Command/Theme/CleanCommand.php:39-41), all command configure() methods should have a PHPDoc comment with the {@inheritdoc} tag.
| /** | |
| * {@inheritdoc} | |
| */ |
| 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 | ||
| ) | ||
| ); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Incomplete validation in nested module pattern. The code at line 94 only validates the area if parts[1] === 'view', but if this condition is false (e.g., for paths like "Magento_Catalog/templates/file.phtml" without "view"), it still returns the path at line 109 without any area validation. This could allow mapping files from incompatible areas or non-view files. Either throw an exception if 'view' is not present, or add comprehensive validation for all nested module patterns.
| 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 | |
| ) | |
| ); | |
| } | |
| } | |
| if (count($parts) < 3 || $parts[1] !== 'view') { | |
| // Only Vendor_Module/view/{area}/... patterns are supported for nested modules | |
| throw new RuntimeException( | |
| sprintf( | |
| "Unsupported nested module path (missing 'view' segment): %s", | |
| $sourcePath | |
| ) | |
| ); | |
| } | |
| // Format: Vendor_Module/view/{area}/... | |
| $areaPath = $parts[2]; | |
| $areaParts = explode('/', $areaPath, 2); | |
| $area = $areaParts[0]; | |
| if (!$this->isAreaCompatible($area, $themeArea)) { | |
| throw new RuntimeException( | |
| sprintf( | |
| "Cannot map file from area '%s' to %s theme. File: %s", | |
| $area, | |
| $themeArea, | |
| $sourcePath | |
| ) | |
| ); | |
| } |
| use function Laravel\Prompts\confirm; | ||
| use function Laravel\Prompts\search; | ||
|
|
||
| class CopyFromVendorCommand extends AbstractCommand |
There was a problem hiding this comment.
Missing class-level PHPDoc comment. According to the coding guidelines (ID: 1000000) and established conventions in the codebase, all command classes require class-level PHPDoc comments describing their purpose. Add a PHPDoc comment similar to other commands in the same directory.
| private function fixPromptEnvironment(): void | ||
| { | ||
| $this->setPromptEnvironment(); | ||
| } |
There was a problem hiding this comment.
The fixPromptEnvironment() method only calls setPromptEnvironment() without proper cleanup handling. According to the environment variable handling pattern (see AbstractCommand.php:232-266 and CleanCommand.php:186-212), Laravel Prompts require a try-catch-finally pattern with resetPromptEnvironment() in both catch and finally blocks. This method doesn't add value and should be removed - call setPromptEnvironment() directly before the try block containing the search() call.
| private function fixPromptEnvironment(): void | |
| { | |
| $this->setPromptEnvironment(); | |
| } |
| public function getThemeByCode(string $code): ?\Magento\Framework\View\Design\ThemeInterface | ||
| { | ||
| $themes = $this->getAllThemes(); | ||
| foreach ($themes as $theme) { | ||
| if ($theme->getCode() === $code) { | ||
| return $theme; | ||
| } | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Linear search through all themes on every call is inefficient. The getAllThemes() method is called on every invocation, and then a linear search is performed. For installations with many themes, this could be slow. Consider implementing caching of the theme lookup or using Magento's built-in theme loading by code if available. However, this is acceptable for now given typical installations don't have many themes.
| $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.
getThemeCode() calls fixPromptEnvironment() (which only calls setPromptEnvironment()), but never restores the terminal TTY nor resets the prompt environment after the search() prompt. This can leave COLUMNS/LINES/TERM modified for the remainder of the command/process and can break subsequent console output. Wrap the prompt in try/finally and always call Prompt::terminal()->restoreTty() + resetPromptEnvironment().
| $result = confirm( | ||
| label: 'Overwrite existing file?', | ||
| default: false | ||
| ); | ||
| \Laravel\Prompts\Prompt::terminal()->restoreTty(); | ||
| $this->resetPromptEnvironment(); | ||
| return $result; | ||
| } | ||
|
|
||
| $result = confirm( | ||
| label: 'Proceed with copy?', | ||
| default: true | ||
| ); | ||
| \Laravel\Prompts\Prompt::terminal()->restoreTty(); | ||
| $this->resetPromptEnvironment(); | ||
| return $result; | ||
| } catch (\Exception $e) { | ||
| $this->resetPromptEnvironment(); | ||
| $this->io->error('Interactive mode failed: ' . $e->getMessage()); | ||
| return false; |
There was a problem hiding this comment.
confirmCopy() resets the prompt environment in the normal flow, but it does not restore the TTY in the exception path, and the reset/restore is duplicated across branches rather than guaranteed in a finally. Use a try { ... } finally { Prompt::terminal()->restoreTty(); $this->resetPromptEnvironment(); } so the terminal is restored even when confirm() throws.
| $result = confirm( | |
| label: 'Overwrite existing file?', | |
| default: false | |
| ); | |
| \Laravel\Prompts\Prompt::terminal()->restoreTty(); | |
| $this->resetPromptEnvironment(); | |
| return $result; | |
| } | |
| $result = confirm( | |
| label: 'Proceed with copy?', | |
| default: true | |
| ); | |
| \Laravel\Prompts\Prompt::terminal()->restoreTty(); | |
| $this->resetPromptEnvironment(); | |
| return $result; | |
| } catch (\Exception $e) { | |
| $this->resetPromptEnvironment(); | |
| $this->io->error('Interactive mode failed: ' . $e->getMessage()); | |
| return false; | |
| return confirm( | |
| label: 'Overwrite existing file?', | |
| default: false | |
| ); | |
| } | |
| return confirm( | |
| label: 'Proceed with copy?', | |
| default: true | |
| ); | |
| } catch (\Exception $e) { | |
| $this->io->error('Interactive mode failed: ' . $e->getMessage()); | |
| return false; | |
| } finally { | |
| \Laravel\Prompts\Prompt::terminal()->restoreTty(); | |
| $this->resetPromptEnvironment(); |
| 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'); | ||
| } | ||
|
|
||
| protected function executeCommand(InputInterface $input, OutputInterface $output): int | ||
| { |
There was a problem hiding this comment.
Other command classes include /** {@inheritdoc} */ PHPDoc on configure() and executeCommand() (see e.g. BuildCommand). For consistency with the established command documentation pattern, add those docblocks here as well.
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>
dermatz
force-pushed
the
110-feature-request-copy-vendor-files-to-theme
branch
from
ea9aa38 to
203d83f
Compare
| $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 |
There was a problem hiding this comment.
In the nested-module mapping branch, $parts = explode('/', $relativePath, 3) makes $parts[2] contain {area}/... (not just the area). That means area validation will be wrong for paths like Vendor_Module/view/frontend/... and can incorrectly throw. Parse the area segment explicitly (e.g., split into 4+ segments or use a regex) so $area is just frontend|adminhtml|base.
| use Magento\Framework\Filesystem\DirectoryList; | ||
| use RuntimeException; | ||
| use Magento\Framework\App\Area; | ||
| use Magento\Framework\Filesystem\Driver\File; |
There was a problem hiding this comment.
New service class is missing the class-level PHPDoc block that other services in this codebase include (e.g. ThemeCleaner). Adding a short description helps keep generated docs and IDE navigation consistent.
| use Magento\Framework\Filesystem\Driver\File; | |
| /** | |
| * Service for mapping vendor file paths to theme override locations. | |
| */ |
| public function __construct( | ||
| private readonly ThemeList $themeList, | ||
| private readonly VendorFileMapper $vendorFileMapper, | ||
| private readonly DirectoryList $directoryList, |
There was a problem hiding this comment.
This command hard-codes the full command name instead of using AbstractCommand::getCommandName(), which is how the other theme commands are named (e.g. BuildCommand/CleanCommand). Using the helper keeps naming consistent and avoids divergence if the prefix ever changes.
| private readonly DirectoryList $directoryList, | |
| $this->setName($this->getCommandName('theme:copy-from-vendor')) |
| // Example: templates/Mollie_Payment/foo.phtml -> templates/foo.phtml | ||
| // This prevents Theme/Mollie_Payment/templates/Mollie_Payment/foo.phtml | ||
| // Note: Check both strict and case-insensitive to be safe | ||
| if (str_contains($targetPath, '/' . $originalModule . '/')) { |
There was a problem hiding this comment.
Minor formatting: the indentation on this line is off compared to surrounding code, which can trip PHPCS in strict configurations. Align it with the other statements in the block.
| if (str_contains($targetPath, '/' . $originalModule . '/')) { | |
| $targetPath = str_ireplace('/' . $originalModule . '/', '/', $targetPath); |
| $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.
getThemeCode() sets the Laravel Prompts environment before calling search(), but never restores TTY / resets the prompt environment afterward. This can leave the terminal in a broken state and leak env var changes into the rest of the command. Use the same restore/reset pattern used elsewhere after the prompt completes (and on failure).
| $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...' | |
| ); | |
| $restorePromptEnvironment = $this->fixPromptEnvironment(); | |
| try { | |
| $themeCode = (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...' | |
| ); | |
| } finally { | |
| if (is_callable($restorePromptEnvironment)) { | |
| $restorePromptEnvironment(); | |
| } | |
| } | |
| return $themeCode; |
| use RuntimeException; | ||
| use Magento\Framework\App\Area; | ||
| use Magento\Framework\Filesystem\Driver\File; | ||
|
|
There was a problem hiding this comment.
VendorFileMapper is a new service but it doesn’t have a class-level PHPDoc describing its purpose. Other services in this codebase generally include a short class docblock (e.g. NodePackageManager, SymlinkCleaner), so adding one here would keep documentation consistent.
| /** | |
| * Service that maps Magento vendor file paths to the corresponding | |
| * theme override locations, resolving the correct area and directory | |
| * structure for theme customization. | |
| */ |
| $themes = $this->themeList->getAllThemes(); | ||
| $options = []; | ||
| foreach ($themes as $theme) { | ||
| $options[$theme->getCode()] = $theme->getCode(); | ||
| } |
There was a problem hiding this comment.
When the theme argument is omitted, the command always invokes an interactive Laravel Prompts search() selection. Unlike other commands (e.g. theme build/clean), this doesn’t check isInteractiveTerminal() first, so non-interactive runs can fail unexpectedly. Consider adding a non-interactive fallback (print available themes + error) when prompting isn’t possible.
| $parts = explode('/', $relativePath, 3); | ||
| if (count($parts) >= 3 && $parts[1] === 'view') { | ||
| // Format: Vendor_Module/view/{area}/... | ||
| $area = $parts[2]; | ||
| if (!$this->isAreaCompatible($area, $themeArea)) { |
There was a problem hiding this comment.
In the nested-module fallback, $area is derived from explode('/', $relativePath, 3) so it will include the remainder of the path (e.g. frontend/some/file) rather than just the area segment. That makes isAreaCompatible() comparisons fail for valid paths like Vendor_Module/view/frontend/.... Extract only the single {area} segment (e.g. split without the 3 limit or use a regex capture).
| $dom->loadXML($content); | ||
| libxml_use_internal_errors($libxmlState); | ||
|
|
||
| $xpath = new \DOMXPath($dom); |
There was a problem hiding this comment.
DOMXPath queries using @xsi:type won’t match unless the xsi namespace is registered on the XPath instance. As written, this query will return empty/false and the compat-module XML fallback will never detect registrations.
| $xpath = new \DOMXPath($dom); | |
| $xpath = new \DOMXPath($dom); | |
| $xpath->registerNamespace('xsi', 'http://www.w3.org/2001/XMLSchema-instance'); |
| } catch (\Exception $e) { | ||
| $this->resetPromptEnvironment(); | ||
| $this->io->error('Interactive mode failed: ' . $e->getMessage()); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
If the confirm() prompt throws, the catch block resets the environment but does not restore the terminal TTY. Move Prompt::terminal()->restoreTty() + resetPromptEnvironment() into a finally (or ensure both are called in the catch) so the terminal is always restored.
| $this->vendorFileMapper = new VendorFileMapper( | ||
| $this->componentRegistrar, | ||
| $this->directoryList | ||
| ); | ||
| } |
There was a problem hiding this comment.
VendorFileMapper now requires 5 constructor dependencies, but the unit test instantiates it with only 2, which will fatal before any assertions run. Update the test to provide mocks/stubs for the new dependencies (ObjectManagerInterface, App\State, and File driver) or refactor the service constructor to keep unit tests viable.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
|
||
| $this->io->note("No files were modified (dry-run mode)."); | ||
| } | ||
|
|
||
| /** | ||
| * Fix prompt environment | ||
| * | ||
| * @return void | ||
| */ |
There was a problem hiding this comment.
fixPromptEnvironment() is a redundant wrapper around setPromptEnvironment() and is only called once. Removing it and calling setPromptEnvironment() directly would reduce indirection and keep prompt handling consistent with other commands.
| libxml_use_internal_errors($libxmlState); | ||
|
|
||
| $xpath = new \DOMXPath($dom); | ||
| $query = "//argument[@name='compatModules'][@xsi:type='array']/item[@xsi:type='array']"; |
There was a problem hiding this comment.
DOMXPath is querying attributes with the xsi: prefix (@xsi:type) but no namespace is registered on the DOMXPath instance. In PHP this typically yields an “Undefined namespace prefix” XPath error and $xpath->query() returns false, so the compat-module XML fallback will never find matches. Register the XSI namespace (usually http://www.w3.org/2001/XMLSchema-instance) via $xpath->registerNamespace('xsi', ...), or avoid the prefix by querying with local-name()/namespace-uri() checks.
| $query = "//argument[@name='compatModules'][@xsi:type='array']/item[@xsi:type='array']"; | |
| $xpath->registerNamespace( | |
| 'xsi', | |
| 'http://www.w3.org/2001/XMLSchema-instance' | |
| ); | |
| $query = "//argument[@name='compatModules']" | |
| . "[@xsi:type='array']/item[@xsi:type='array']"; |
| if (str_starts_with($sourceFile, '/')) { | ||
| $absoluteSourcePath = $sourceFile; | ||
| } else { | ||
| $absoluteSourcePath = $rootPath . '/' . $sourceFile; | ||
| } | ||
|
|
||
| if (!$this->fileDriver->isExists($absoluteSourcePath)) { | ||
| throw new \RuntimeException("Source file not found: $absoluteSourcePath"); | ||
| } | ||
|
|
||
| return $absoluteSourcePath; |
There was a problem hiding this comment.
getAbsoluteSourcePath() accepts any absolute path (or any relative path under the Magento root) as long as it exists, but this command is described as copying from vendor/. This allows copying arbitrary files (including non-vendor and potentially sensitive files) if the mapper happens to resolve them. After normalization, enforce that the source path is within the Magento vendor/ directory (and reject .. traversal) before proceeding.
| if (str_starts_with($sourceFile, '/')) { | |
| $absoluteSourcePath = $sourceFile; | |
| } else { | |
| $absoluteSourcePath = $rootPath . '/' . $sourceFile; | |
| } | |
| if (!$this->fileDriver->isExists($absoluteSourcePath)) { | |
| throw new \RuntimeException("Source file not found: $absoluteSourcePath"); | |
| } | |
| return $absoluteSourcePath; | |
| if (str_starts_with($sourceFile, '/')) { | |
| $absoluteSourcePath = $sourceFile; | |
| } else { | |
| $absoluteSourcePath = $rootPath . DIRECTORY_SEPARATOR . $sourceFile; | |
| } | |
| if (!$this->fileDriver->isExists($absoluteSourcePath)) { | |
| throw new \RuntimeException( | |
| "Source file not found: {$absoluteSourcePath}" | |
| ); | |
| } | |
| $normalizedSourcePath = $this->fileDriver->getRealPath($absoluteSourcePath); | |
| if ($normalizedSourcePath === false) { | |
| throw new \RuntimeException( | |
| "Unable to resolve real path for source file: {$absoluteSourcePath}" | |
| ); | |
| } | |
| $vendorPath = $rootPath . DIRECTORY_SEPARATOR . 'vendor'; | |
| $normalizedVendorPath = $this->fileDriver->getRealPath($vendorPath); | |
| if ($normalizedVendorPath === false) { | |
| throw new \RuntimeException( | |
| "Unable to resolve Magento vendor directory at: {$vendorPath}" | |
| ); | |
| } | |
| $normalizedVendorPath = rtrim($normalizedVendorPath, DIRECTORY_SEPARATOR) | |
| . DIRECTORY_SEPARATOR; | |
| if ( | |
| !str_starts_with($normalizedSourcePath, $normalizedVendorPath) | |
| ) { | |
| throw new \RuntimeException( | |
| "Source file must be inside the Magento vendor/ directory." | |
| ); | |
| } | |
| return $normalizedSourcePath; |
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.