Check theme source files in Virtual Theme Check - #26
Merged
Conversation
Making a virtual theme physical while its source files are unavailable is what breaks the storefront with "Required parameter 'theme_dir' was not passed", so the check now reports the state of the files and the fixer refuses to touch the themes it cannot read. - Virtual Theme Check gets Path and Status columns - Status names the files Magento can't read. Unreadable file is never reported as a removed one - a failed stat cannot tell them apart - Fix All opens up the permissions of unreadable source files and skips the themes that are still unreadable, listing them in the response - Readability check extracted into Swissup\Core\Model\Theme\SourceFiles, shared by the block and the controller - Config table styles shared between Modules and Virtual Theme Check Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds source-file validation and safer repair behavior for virtual Magento themes.
Changes:
- Reports each virtual theme’s path and unreadable source files.
- Repairs file permissions and skips unsafe conversions.
- Displays accurate fix results and shares table styling.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
Model/Theme/SourceFiles.php |
Checks and repairs required theme files. |
Controller/Adminhtml/Theme/FixVirtualThemes.php |
Safely converts eligible themes and reports outcomes. |
Block/Adminhtml/Config/Form/Field/VirtualCheck.php |
Collects theme path and source-file status. |
Block/Adminhtml/Config/Form/Field/FixVirtualThemes.php |
Updates the fixer container class. |
view/adminhtml/templates/config/field/virtual_check.phtml |
Renders the expanded status table. |
view/adminhtml/templates/config/field/modules.phtml |
Adopts shared table and monospace styles. |
view/adminhtml/web/js/virtualfix.js |
Handles fixed/skipped results dynamically. |
view/adminhtml/web/css/system-config/modules.less |
Extracts reusable configuration-table styling. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #19
Virtual Theme Check only told you a theme was virtual. It could not tell a theme whose DB flag is simply wrong from one whose source files are unavailable — and Fix All happily made both physical. Converting the second kind is exactly what produced the
Required parameter 'theme_dir' was not passedexception behind the missing product images in #19.Check
Two new columns — Path and Status:
frontend/Swissup/argentobreeze-blankfrontend/Swissup/breeze-evolutionregistration.phpandtheme.xmlare probed through the theme's registered directory.Why it never says "removed"
The issue asks to mark a theme "removed" when its source files are gone. The module deliberately does not claim that, because it cannot prove it:
ComponentRegistrar::getPath()returning null means not registered, not not present. A theme underapp/designwhose directory lacks read permission is skipped by theglob()inapp/etc/NonComposerComponentRegistration.phpwith no error at all — files sitting right there, invisible to Magento.file_exists()returning false proves nothing either. If a parent directory lacks execute permission the stat fails identically to the file being absent.So the status reports what was actually observed — the files could not be read — and leaves the cause to the admin. Same information, no invented certainty.
Fixer
Fix Allnow, per theme:0644and re-checks it — recovers the common permissions case without manual shell workThe response carries
fixed/skippedcounts, so the JS shows the real outcome instead of a hardcoded "Success!" — the table is hidden only when something actually changed, and the button stays available while themes remain skipped.Structure
The readability check lives in
Swissup\Core\Model\Theme\SourceFilesso the block and the controller cannot drift apart:getUnreadable()returns the filenames for the status text,makeReadable()returns whether the theme is safe to convert..swissup-config-tableand.font-monoare split out of.swissup-modules-tableand shared with this table.Testing
Exercised against a real install for each case — theme registered and readable, registered with
theme.xmlatchmod 000(recovered by the fixer), theme absent from disk, and a theme with notheme_path. Fix All verified end to end in the admin.