[WIP] Update Manager Refactor #1259
Conversation
| "require": { | ||
| "php": ">=8.1", | ||
| "winter/storm": "dev-develop as 1.2", | ||
| "winter/storm": "dev-wip/support-uploading-packages-update-manager-changes as 1.2", |
There was a problem hiding this comment.
This will need to be updated before merge.
…ager-changes # Conflicts: # modules/backend/ServiceProvider.php
|
Caution Review failedFailed to post review comments WalkthroughThis pull request introduces a comprehensive extension management framework refactoring. It moves plugin/theme/module management classes to a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MarketplaceUI as Marketplace UI<br/>(Vue Component)
participant UpdatesController as Updates<br/>Controller
participant ExtensionSource
participant ExtensionManager as Extension<br/>Manager
participant MarketPlaceApi
participant FileSystem
User->>MarketplaceUI: Browse/Search Products
MarketplaceUI->>UpdatesController: onGetMarketplaceListings()
UpdatesController->>MarketPlaceApi: requestProductDetails()
MarketPlaceApi-->>UpdatesController: Product Catalog
UpdatesController-->>MarketplaceUI: Listed Products
User->>MarketplaceUI: Install Plugin/Theme
MarketplaceUI->>UpdatesController: onInstallPackage()
UpdatesController->>ExtensionSource: create(SOURCE_MARKET)
ExtensionSource->>MarketPlaceApi: downloadPlugin/Theme()
MarketPlaceApi->>FileSystem: Save Archive
ExtensionSource->>FileSystem: Extract Archive
ExtensionSource->>ExtensionManager: install()
ExtensionManager->>FileSystem: Validate & Register
ExtensionManager-->>UpdatesController: Installation Complete
UpdatesController-->>MarketplaceUI: Status Update
sequenceDiagram
participant ConsoleCmd as Console<br/>Command
participant UpdateManager
participant ModuleManager
participant PluginManager
participant ThemeManager
ConsoleCmd->>UpdateManager: update(migrationsOnly: true)
UpdateManager->>UpdateManager: check available updates
alt Has Updates
UpdateManager->>ModuleManager: update()
ModuleManager->>ModuleManager: Run migrations, seed
UpdateManager->>PluginManager: update()
PluginManager->>PluginManager: Update, migrate, refresh
UpdateManager->>ThemeManager: update()
ThemeManager->>ThemeManager: Refresh themes
UpdateManager-->>ConsoleCmd: Updates Complete
else No Updates
UpdateManager-->>ConsoleCmd: No Updates Available
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Areas requiring extra attention:
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 58
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
modules/system/console/WinterUtil.php (1)
518-524: Add error handling and validate response structure for MarketPlaceApi call.The
MarketPlaceApi::request()method throwsApplicationExceptionon API errors (non-200 responses, invalid JSON, etc.), but the code at line 518 has no try-catch to handle these exceptions. Additionally, the response structure is not validated—the code assumes$result['name']and$result['owner']exist without checking.Wrap the API call in a try-catch block and validate the response structure:
try { $result = MarketPlaceApi::instance()->request(MarketPlaceApi::REQUEST_PROJECT_DETAIL, $projectId); if (empty($result['name']) || empty($result['owner'])) { $this->error("Invalid project response: missing required fields"); return; } Parameter::set([ 'system::project.id' => $projectId, 'system::project.name' => $result['name'], 'system::project.owner' => $result['owner'], ]); } catch (ApplicationException $e) { $this->error("Failed to fetch project details: " . $e->getMessage()); return; }modules/system/console/plugin/PluginRollback.php (1)
29-33: Fix help text: "disable" → "rollback".Minor UX clarity in the command signature.
Apply this diff:
- protected $signature = 'plugin:rollback - {plugin : The plugin to disable. <info>(eg: Winter.Blog)</info>} + protected $signature = 'plugin:rollback + {plugin : The plugin to rollback. <info>(eg: Winter.Blog)</info>} {version? : If this parameter is not specified the plugin will be completely rolled back; otherwise it will stop on the specified version. <info>(eg: 1.3.9)</info>} {--f|force : Force the operation to run and ignore production warnings and confirmation questions.}';modules/cms/classes/ThemeManager.php (1)
161-168: Null-check loaded theme before use in uninstall()Theme::load() can return null; accessing isActiveTheme() would fatally error.
- if (is_string($theme)) { - $theme = Theme::load($theme); - } + if (is_string($theme)) { + $loaded = Theme::load($theme); + if (!$loaded) { + throw new ApplicationException(sprintf('Theme "%s" not found.', $theme)); + } + $theme = $loaded; + }modules/system/controllers/Updates.php (3)
316-357: Broken call to removed UpdateManager::requestUpdateList()This method no longer exists; the new API provides availableUpdates() and update(). This will fatal at runtime.
Suggested minimal interim change (WIP-safe): avoid server step building and delegate directly to update(), keeping UI flow:
- $manager = UpdateManager::instance(); - $result = $manager->requestUpdateList($force); + $manager = UpdateManager::instance(); + // WIP: composer-only flow — build steps to run migrations only + $result = ['core' => null, 'plugins' => [], 'themes' => []]; ... - $updateSteps = $this->buildUpdateSteps($core, $plugins, $themes, false); + $updateSteps = []; ... $updateSteps[] = [ 'code' => 'completeUpdate', 'label' => Lang::get('system::lang.updates.update_completing'), ];Longer-term: replace the legacy step engine with the new event-stream composer workflow hooked into the ExtensionManagers.
159-210: Legacy step executor targets removed methodsdownloadCore/extractCore/downloadPlugin/extractPlugin/etc. were removed. Only completeUpdate/completeInstall should remain here in the short term.
switch ($stepCode) { - case 'downloadCore': - ... - case 'extractPlugin': - ... + // Legacy steps removed in refactor; handled by ExtensionManagers now. case 'completeUpdate': $manager->update(); Flash::success(Lang::get('system::lang.updates.update_success')); return Redirect::refresh(); case 'completeInstall': $manager->update(); Flash::success(Lang::get('system::lang.install.install_success')); return Redirect::refresh(); }Also consider guarding unknown codes with a warning to avoid silent no-ops.
362-449: Rework update step builder to align with ExtensionManagersonApplyUpdates() + buildUpdateSteps() still build legacy server-driven steps. With composer-first updates, generate steps only for finalization (migrations) or replace with SSE-driven install flow.
I can draft a small adapter that translates Module/Plugin/ThemeManager::availableUpdates() into a flat steps list and executes via SSE; want me to open a follow-up PR?
Also applies to: 451-534
♻️ Duplicate comments (6)
composer.json (1)
33-33: Update winter/storm dependency before merge.The dependency is currently pointing to a WIP branch, which is appropriate for development but must be updated to a stable reference before merging to production.
modules/system/controllers/updates/traits/ManagesMarketplaceProject.php (1)
9-9: ApplicationException import is already present.The import for
Winter\Storm\Exception\ApplicationExceptionis already included at line 9, addressing the previous review comment.modules/system/controllers/updates/assets/src/components/Product.vue (1)
47-51: License indicator (repeat).Consider adding license info beside stars/downloads when available from the marketplace.
modules/system/classes/extensions/ExtensionManager.php (1)
56-86: Consider Winter’s Singleton trait (repeat).Using the framework’s Singleton trait would standardize behavior and reduce boilerplate. If __construct constraints block it, document the rationale or adapt as discussed.
modules/system/controllers/updates/assets/src/components/MarketPlace.vue (1)
91-106: i18n placeholders.Strings like “Sort”, “Per Page”, “Categories”, etc., should use translation keys (not urgent if planned).
modules/system/classes/extensions/ExtensionManagerInterface.php (1)
121-134: Name and guard “nuclear” operations.
tearDown()is destructive. ConsideruninstallAll()plus an explicit confirmation mechanism (e.g., an environment flag or “I acknowledge” callback) to reduce accidental use.
🧹 Nitpick comments (44)
modules/system/controllers/updates/form.theme_upload.yaml (1)
1-6: Add a label and validation properties to improve form clarity.This YAML form definition is missing common Winter CMS form field properties that would enhance the user experience. Specifically, the
uploaded_packagefield lacks:
- A
labelfor human-readable display in the UI- A
requiredproperty to enforce validation- Optionally, a
commentproperty for helper textWithout these properties, the form input may appear unlabeled, making it unclear to users what file they should upload.
Consider applying this diff to add standard form field properties:
fields: uploaded_package: type: fileupload span: full mode: file fileTypes: zip + label: winter.system::lang.theme_upload.uploaded_package + required: true + comment: winter.system::lang.theme_upload.uploaded_package_commentNote: Adjust the label and comment keys to match your localization file structure.
modules/system/controllers/updates/assets/.eslintignore (1)
1-2: Consider using more robust patterns for compiled artifacts and clarify Vue file linting strategy.The
dist/*.jspattern only excludes JS files directly indist/, not in nested subdirectories. If your build output includes nested directories, usedist/(to exclude the entire directory) ordist/**/*.js. Additionally, the*.vuepattern excludes all Vue files from ESLint. Confirm whether this is intentional because you're using a separate Vue linter (e.g.,eslint-plugin-vue), or if Vue files should be linted with ESLint using appropriate plugins.If excluding the entire
dist/directory:-dist/*.js +dist/If Vue files should be linted with ESLint plugins, remove the second line or configure
eslint-plugin-vuein your.eslintrcinstead of using the ignore file.modules/system/controllers/updates/form.plugin_upload.yaml (1)
1-6: Add alabelattribute for better UX and accessibility.This form field lacks a label, which is a best practice for form inputs. Adding a label helps users understand what the field is for and improves accessibility.
Apply this diff to add a label:
fields: uploaded_package: type: fileupload + label: Upload Plugin Package span: full mode: file fileTypes: zipAdditionally, consider adding validation constraints (e.g.,
required: true, maximum file size) and a help comment to guide users on acceptable file formats. Since the PR indicates localization strings are deferred, you may want to wrap the label text in a locale key if that pattern is used in this codebase:label: system.update.plugin_upload_labelmodules/system/console/BaseScaffoldCommand.php (1)
106-113: Consider differentiating colors for generated vs. updated files.The rendering uses
bg-yellow-600for both "generated" and "updated" modes. Differentiating the colors could improve visual clarity (e.g., green for new files, blue/yellow for updates).Example approach:
- render(<<<HTML - <div class="mx-2"> - <div class="px-1 bg-yellow-600">File $mode</div> - <em class="ml-1"> - $relativeFile - </em> - </div> - HTML); + $color = $mode === 'generated' ? 'bg-green-600' : 'bg-blue-600'; + render(<<<HTML + <div class="mx-2"> + <div class="px-1 $color">File $mode</div> + <em class="ml-1"> + $relativeFile + </em> + </div> + HTML);modules/system/classes/core/package-list.json (1)
1-1858: Test data should be temporary.Per PR objectives, this is demo data meant to emulate the Winter Marketplace API for experimental testing. Please ensure:
- This file is removed before final merge, or
- Clearly documented as fixture data if intentionally kept
- Future timestamps (2025-05-14, 2025-07-27) and empty URL fields are addressed for production use
Consider moving this to a dedicated test fixtures directory (e.g.,
tests/fixtures/) if it needs to be retained for testing purposes.modules/system/controllers/updates/assets/src/utils/numbers.js (1)
1-36: Consider explicit NaN handling.Returning
0for NaN values (line 6) may mask invalid inputs. Consider whether returning an empty string,null, or throwing an error would be more appropriate for your use case.Additionally, the decimal padding logic on lines 32-34 could be slightly more readable:
- s[1] += new Array(decimals - s[1].length + 1).join('0'); + s[1] += Array(decimals - s[1].length + 1).fill('0').join('');modules/system/classes/extensions/Preserver.php (1)
29-52: Simplify sprintf format and verify archive file extension.The sprintf format on lines 39-45 is complex with positional argument
%4$s. Consider simplifying for better readability.Additionally, verify that the
packArchivemethod (line 49) adds the.zipextension to the archive filename, as it's not explicitly included in the path passed toStorage::path().Simplified alternative:
- $extensionArchiveDir = sprintf( - '%s%4$s%s%4$s%s', - static::ROOT_PATH, - $type, - $extension->getIdentifier(), - DIRECTORY_SEPARATOR - ); + $extensionArchiveDir = implode(DIRECTORY_SEPARATOR, [ + static::ROOT_PATH, + $type, + $extension->getIdentifier() + ]);modules/system/classes/extensions/traits/HasVersionFile.php (1)
38-46: Optional: Decouple script filtering from$this->getPath()by deriving fromversion.yamllocation.Using
$this->getPath().'/updates/'assumes a specific structure. Deriving fromdirname($versionFile)makes the trait more reusable.Example:
- $details = array_values(array_filter($details, function ($string) { - return !Str::endsWith($string, '.php') || !File::exists($this->getPath() . '/updates/' . $string); + $details = array_values(array_filter($details, function ($string) use ($versionFile) { + if (!Str::endsWith($string, '.php')) return true; + $updatesDir = dirname($versionFile); + return !File::exists($updatesDir . DIRECTORY_SEPARATOR . $string); }));modules/system/console/plugin/PluginRollback.php (1)
40-42: Update docblock to reference PluginManager, not UpdateManager.Reflects the new implementation.
- * @throws \Exception if the UpdateManager is unable to rollback the requested plugin to the requested version + * @throws \Exception if the PluginManager is unable to rollback the requested plugin to the requested versionmodules/system/controllers/updates/assets/src/utils/message.js (1)
33-43: Code duplication: Same regex used twice.Lines 33 and 40 use the identical regex pattern
/\.*?[\d][\w]*DONE$/. This is unnecessarily duplicated.Reuse the matched result:
search = line.match(/\.*?[\d][\w]*DONE$/); let ending = null; if (search) { line = `<div class="pl-3">${line.substring(0, line.length - search[0].length)}</div>`; ending = ` <span class="text-orange-500 ml-auto"> - ${search[0].match(/\.*?[\d][\w]*DONE$/)[0].replace(/\.*/, '').replace('DONE', '')} + ${search[0].replace(/\.*/, '').replace('DONE', '')} </span> `; }modules/system/controllers/updates/assets/src/updates.js (1)
23-33: Fix typo in comment.Line 25 has a typo: "vuw" should be "vue".
- // Add request to vuw + // Add request to vue app.request = (handler, options) => Snowboard.request(handler, options);modules/system/controllers/updates/index.php (1)
229-233: Inline style override suggests layout integration issue.The inline style overriding
#layout-bodypadding suggests the new UI doesn't integrate cleanly with the existing layout system. This should ideally be handled through proper CSS architecture rather than inline overrides.Consider:
- Adding a proper CSS class to the layout configuration
- Creating a dedicated stylesheet for the updates view
- Using a layout configuration option to control padding
modules/system/controllers/updates/assets/src/components/Product.vue (2)
95-100: Avoid window in template; compute a safe details URL.Bind a computed detailsHref to avoid direct window access in template, and handle missing /install gracefully.
- <a v-if="product.installed" - :href="window.location.href.replace('/install', `/details/${product.code.toLowerCase().replace('.', '-')}`)" + <a v-if="product.installed" + :href="detailsHref" class="btn btn-outline-info">Add:
@@ export default { props: ['product', 'type'], data: () => ({ @@ computed: { + detailsHref() { + try { + const url = new URL(window.location.href); + url.pathname = url.pathname.replace(/\/install(?:\/)?$/, `/details/${this.product.code.toLowerCase().replace('.', '-')}`); + return url.toString(); + } catch { + return '#'; + } + }, window: {
11-23: Tailwind utility validity.Classes like size-38 and group-hover:w-50 may not exist unless configured; prefer arbitrary values: size-[9.5rem], w-[12.5rem].
modules/system/classes/extensions/ExtensionManager.php (1)
5-13: Remove unused imports.Component and Termwind aren’t used here.
-use Illuminate\Console\View\Components\Component; @@ -use Termwind\Termwind;modules/system/console/plugin/PluginInstall.php (4)
112-113: Remove unused variable.$pluginsToMigrate is assigned but never used.
- $pluginsToMigrate[] = $code;
83-96: Clean replace install path to avoid stale files.When replacing an installed plugin, delete the existing directory before copying to prevent leftover files.
- // @TODO: Check if plugin is already installed if ($pluginManager->findByIdentifier($code)) { if (!$this->confirm(sprintf( 'Plugin %s is already installed. Would you like to replace the currently installed version with the one found in %s?', $code, $pluginName ))) { $this->output->writeln(sprintf('<info>Skipping plugin: %s</info>', $code)); unset($plugins[$code]); continue; } } $this->output->writeln(sprintf('<info>Installing plugin: %s</info>', $code)); $pluginDir = plugins_path(strtolower(str_replace('.', DIRECTORY_SEPARATOR, $code))); try { + if (File::isDirectory($pluginDir)) { + File::deleteDirectory($pluginDir); + } // Copy the files File::copyDirectory(pathinfo($pluginClassFile, PATHINFO_DIRNAME), $pluginDir);Also applies to: 100-126
151-154: Fix sprintf usage in message.Argument provided but not used; include %s or remove the arg.
- $this->output->writeln(sprintf('<info>Migrating plugin...</info>', $code)); + $this->output->writeln(sprintf('<info>Migrating plugin: %s</info>', $code));
66-71: Zip extraction safety.Ensure extractArchive protects against zip-slip (path traversal) and malicious symlinks within archives.
modules/system/controllers/updates/assets/src/components/MarketPlace.vue (5)
286-296: Don’t mutate source when sorting.Clone before sort to keep this.products stable.
- return products.sort({ + return [...products].sort({ new: (a, b) => (new Date(b.updated_at)).getTime() - (new Date(a.updated_at)).getTime(), downloads: (a, b) => b.downloads - a.downloads, stars: (a, b) => b.stars - a.stars, }[this.sort]);
109-111: Add keys to v-for for stable rendering.Provide unique :key for products.
- <Product v-for="product in pageProducts" :product="product" :type="this.mode"></Product> + <Product v-for="product in pageProducts" :key="product.code" :product="product" :type="mode"></Product>
19-31: Add keys to other v-for loops.Keys improve diffing and performance.
- <li v-for="type in ['all', 'free', 'paid']"> + <li v-for="type in ['all', 'free', 'paid']" :key="type"> ... - <li v-for="feature in [{label: 'Translate Support', key: 'translate', icon: 'icon-language'}]" class="h-full"> + <li v-for="feature in [{label: 'Translate Support', key: 'translate', icon: 'icon-language'}]" :key="feature.key" class="h-full"> ... - <li v-for="category in categories" class="mb-1"> + <li v-for="category in categories" :key="category.id" class="mb-1">Also applies to: 34-50, 57-66
160-166: Case-insensitive search and null guards.Make includes checks safer.
- results = results.filter((product) => { - return product.name.includes(this.search) - || product.description.includes(this.search) - || product.composer_package.includes(this.search); - }); + const q = this.search.toLowerCase(); + results = results.filter((p) => + (p.name || '').toLowerCase().includes(q) || + (p.description || '').toLowerCase().includes(q) || + (p.composer_package || '').toLowerCase().includes(q) + );
228-235: Remove filterPrice toggle hack; initialize cleanly.Set default to 'all' and drop the forced toggling.
- // ... vue really doesn't like `all` being the default ... - this.filterPrice = 'all'; - this.filterPrice = null; - this.$nextTick(() => { - this.filterPrice = 'all'; - }); + this.filterPrice = 'all';modules/system/classes/extensions/ExtensionManagerInterface.php (3)
35-35: Avoid reserved-name methodlist().Using
list()risks confusion with PHP’s language construct. Preferall(),listInstalled(), orgetAll().
72-86: Narrow theenable/disableflag type.
string|bool $flagis ambiguous. Prefer a dedicated enum (PHP 8.1+) or?stringwith documented string-literals (DISABLED_*). Improves static analysis and call-site clarity.
94-110: Return types are too loose.
mixedonupdate/refresh/rollbackhinders consumers. Consider concrete returns (bool|array|WinterExtension|null) per method contract and update docs accordingly.modules/system/controllers/updates/traits/ManagesPlugins.php (1)
250-267: Ensure cached streaming output uses a TTL.
doWrite()appends to a cache entry without expiry. Add a TTL to each write to keep the key alive for a bounded time window (e.g., 15 minutes).Apply this diff:
- Cache::put($this->key, Cache::get($this->key, '') . trim($message) . ($newline ? "\n" : '')); + Cache::put( + $this->key, + Cache::get($this->key, '') . trim($message) . ($newline ? "\n" : ''), + now()->addMinutes(15) + );modules/system/classes/extensions/PluginVersionManager.php (1)
498-506: Optional: add existence check before packDown for symmetry.You check for file existence in
applyDatabaseScriptbut not inremoveDatabaseScript. Consider mirroring the guard to avoid noisy failures when files are missing.- $this->updater->packDown($updateFile); + if (File::isFile($updateFile)) { + $this->updater->packDown($updateFile); + } else { + $this->pluginManager->renderComponent(Error::class, sprintf('Migration file "%s" not found.', $script)); + }Also applies to: 517-526
modules/system/aliases.php (1)
94-98: Alias targets verified and present; deprecation notes are optional.All scaffold and extension manager alias targets have been confirmed to exist and are correctly namespaced:
System\Console\Create\CreateCommand,CreateModel,CreatePlugin,CreateSettingsSystem\Classes\Extensions\PluginManager,PluginBaseThe backward compatibility mappings are sound. Adding deprecation notes in CHANGELOG/UPGRADE is optional but recommended to guide plugin authors toward the canonical System namespace.
modules/system/classes/extensions/ModuleManager.php (1)
36-43: Guard output usage to avoid null dereferenceinit()/setOutput() call $this->migrator->setOutput($this->output) before $this->output may be set. Add a null check.
- $this->migrator->setOutput($this->output); + if (isset($this->output)) { + $this->migrator->setOutput($this->output); + }And similarly where writing to $this->output when $firstUp.
Also applies to: 91-97
modules/cms/classes/ThemeManager.php (1)
49-63: Doc return type should allow nullfindByDirName() can return null; update docblock to reflect nullable return.
- * @return string + * @return string|nullmodules/system/classes/extensions/PluginBase.php (1)
472-495: Return early on first existing markdown fileFor array input, you currently overwrite $contents and return the last found file. Prefer first match.
- foreach ($filenames as $file) { + foreach ($filenames as $file) { if (!File::exists($path . '/' . $file)) { continue; } $contents = File::get($path . '/' . $file); // Parse markdown, clean HTML, remove first H1 tag $contents = Markdown::parse($contents); $contents = Html::clean($contents); $contents = preg_replace('@<h1[^>]*?>.*?<\/h1>@si', '', $contents, 1); + break; }modules/system/controllers/Updates.php (3)
227-245: hasImportantUpdates logic needs revisitingCounting modules to set "important" no longer matches the old "!!!" semantics. Either drop the flag or compute importance from metadata when available.
106-113: Remove dead asset include commentLeftover commented addJs; remove or restore Vite alternative if needed.
73-74: Surface warnings earlierGood call pulling warnings from PluginManager. Consider also surfacing cms.disableCoreUpdates here for UX consistency with ModuleManager.
modules/system/classes/UpdateManager.php (3)
87-95: Normalize nulls from managersEnsure availableUpdates() always returns arrays to avoid count()/array_reduce warnings.
- return [ - 'modules' => ModuleManager::instance()->availableUpdates(), - 'plugins' => PluginManager::instance()->availableUpdates(), - 'themes' => ThemeManager::instance()->availableUpdates(), - ]; + return [ + 'modules' => ModuleManager::instance()->availableUpdates() ?? [], + 'plugins' => PluginManager::instance()->availableUpdates() ?? [], + 'themes' => ThemeManager::instance()->availableUpdates() ?? [], + ];
28-39: Consider honoring disableCoreUpdatesYou store disableCoreUpdates but never use it; either remove the property or use it to skip core-related checks/updates, and surface a warning (consistent with ModuleManager).
Also applies to: 96-107
56-85: Retry logic refinement in check()Current short-circuit only when previous count > 0; you might want to respect the retry window regardless to throttle remote checks, and cache zeroes too.
- && $updateCount > 0Remove this guard or gate by a separate "lastChecked" timestamp.
modules/system/classes/core/MarketPlaceApi.php (2)
183-195: Redundant JSON validity checks after JSON_THROW_ON_ERROR.With JSON_THROW_ON_ERROR, json_decode won’t return false. Reduce to a single is_array guard.
try { $resultData = json_decode($result->body, true, flags: JSON_THROW_ON_ERROR); } catch (Exception $ex) { throw new ApplicationException(Lang::get('system::lang.server.response_invalid')); } - - if ($resultData === false || (is_string($resultData) && !strlen($resultData))) { - throw new ApplicationException(Lang::get('system::lang.server.response_invalid')); - } - - if (!is_array($resultData)) { + if (!is_array($resultData)) { throw new ApplicationException(Lang::get('system::lang.server.response_invalid')); }
507-512: Low‑signal error message from getPackagesFromBazaar().Replace “Bad things have happened” with a translated, actionable message.
- if ($result->code !== 200) { - throw new ApplicationException('Bad things have happened'); - } + if ($result->code !== 200) { + throw new ApplicationException(Lang::get('system::lang.server.response_invalid')); + }modules/system/classes/extensions/PluginManager.php (3)
172-186: “install” path calls updatePlugin(); confirm intended install behavior.This treats install as an update of migrations only. If files must be fetched (composer/market), ensure that happens here before migrations.
353-369: Unused $code variable in foreach (PHPMD).Remove the unused key variable to silence the warning and clarify intent.
- foreach ($this->list() as $code => $plugin) { + foreach ($this->list() as $plugin) {
1493-1506: resolveIdentifier(): scope is fine; optional strengthening.Looks good. Optionally tighten accepted types in PHPDoc to reflect allowed WinterExtension implementations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (9)
modules/system/assets/dist/assets/updates-8hFxTJT2.cssis excluded by!**/dist/**modules/system/assets/dist/assets/updates-BHH8rdGj.cssis excluded by!**/dist/**modules/system/assets/dist/assets/updates-CRrv2aGu.jsis excluded by!**/dist/**modules/system/assets/dist/manifest.jsonis excluded by!**/dist/**modules/system/tests/fixtures/plugin-archives/entire-plugins-directory.zipis excluded by!**/*.zipmodules/system/tests/fixtures/plugin-archives/multiple-authors.zipis excluded by!**/*.zipmodules/system/tests/fixtures/plugin-archives/multiple-plugins-no-author.zipis excluded by!**/*.zipmodules/system/tests/fixtures/plugin-archives/single-plugin-contents.zipis excluded by!**/*.zipmodules/system/tests/fixtures/plugin-archives/single-plugin-nested.zipis excluded by!**/*.zip
📒 Files selected for processing (107)
composer.json(2 hunks)modules/backend/ServiceProvider.php(1 hunks)modules/backend/classes/AuthManager.php(1 hunks)modules/backend/classes/BackendController.php(1 hunks)modules/backend/classes/NavigationManager.php(1 hunks)modules/backend/classes/WidgetManager.php(1 hunks)modules/backend/widgets/Lists.php(1 hunks)modules/cms/ServiceProvider.php(5 hunks)modules/cms/classes/ComponentManager.php(1 hunks)modules/cms/classes/Theme.php(6 hunks)modules/cms/classes/ThemeManager.php(3 hunks)modules/cms/console/ThemeInstall.php(2 hunks)modules/cms/widgets/ComponentList.php(1 hunks)modules/system/ServiceProvider.php(8 hunks)modules/system/aliases.php(1 hunks)modules/system/classes/FileManifest.php(2 hunks)modules/system/classes/MailManager.php(1 hunks)modules/system/classes/MarkupManager.php(2 hunks)modules/system/classes/SettingsManager.php(1 hunks)modules/system/classes/SourceManifest.php(2 hunks)modules/system/classes/UpdateManager.php(5 hunks)modules/system/classes/asset/PackageManager.php(1 hunks)modules/system/classes/core/MarketPlaceApi.php(1 hunks)modules/system/classes/core/package-list.json(1 hunks)modules/system/classes/extensions/ExtensionManager.php(1 hunks)modules/system/classes/extensions/ExtensionManagerInterface.php(1 hunks)modules/system/classes/extensions/ModuleManager.php(1 hunks)modules/system/classes/extensions/PluginBase.php(8 hunks)modules/system/classes/extensions/PluginManager.php(9 hunks)modules/system/classes/extensions/PluginVersionManager.php(17 hunks)modules/system/classes/extensions/Preserver.php(1 hunks)modules/system/classes/extensions/source/ComposerSource.php(1 hunks)modules/system/classes/extensions/source/ExtensionSource.php(1 hunks)modules/system/classes/extensions/source/LocalSource.php(1 hunks)modules/system/classes/extensions/source/MarketSource.php(1 hunks)modules/system/classes/extensions/traits/HasVersionFile.php(1 hunks)modules/system/console/BaseScaffoldCommand.php(2 hunks)modules/system/console/PluginInstall.php(0 hunks)modules/system/console/WinterDown.php(3 hunks)modules/system/console/WinterInstall.php(0 hunks)modules/system/console/WinterTest.php(1 hunks)modules/system/console/WinterUp.php(2 hunks)modules/system/console/WinterUpdate.php(1 hunks)modules/system/console/WinterUtil.php(2 hunks)modules/system/console/asset/AssetCreate.php(2 hunks)modules/system/console/asset/AssetInstall.php(1 hunks)modules/system/console/asset/fixtures/config/tailwind/tailwind.module.config.js.fixture(1 hunks)modules/system/console/asset/npm/NpmVersion.php(1 hunks)modules/system/console/create/CreateCommand.php(1 hunks)modules/system/console/create/CreateFactory.php(1 hunks)modules/system/console/create/CreateJob.php(1 hunks)modules/system/console/create/CreateMigration.php(2 hunks)modules/system/console/create/CreateModel.php(1 hunks)modules/system/console/create/CreatePlugin.php(1 hunks)modules/system/console/create/CreateSettings.php(1 hunks)modules/system/console/create/CreateTest.php(1 hunks)modules/system/console/create/scaffold/plugin/plugin.stub(1 hunks)modules/system/console/plugin/PluginDisable.php(2 hunks)modules/system/console/plugin/PluginEnable.php(2 hunks)modules/system/console/plugin/PluginInstall.php(1 hunks)modules/system/console/plugin/PluginList.php(3 hunks)modules/system/console/plugin/PluginRefresh.php(2 hunks)modules/system/console/plugin/PluginRemove.php(3 hunks)modules/system/console/plugin/PluginRollback.php(4 hunks)modules/system/console/traits/HasPluginArgument.php(1 hunks)modules/system/controllers/Updates.php(20 hunks)modules/system/controllers/updates/_install_plugins.php(1 hunks)modules/system/controllers/updates/_install_themes.php(1 hunks)modules/system/controllers/updates/_is_enabled.php(0 hunks)modules/system/controllers/updates/_is_unfrozen.php(0 hunks)modules/system/controllers/updates/_list_manage_toolbar.php(0 hunks)modules/system/controllers/updates/_list_toolbar.php(0 hunks)modules/system/controllers/updates/_name.php(1 hunks)modules/system/controllers/updates/_open_link.php(1 hunks)modules/system/controllers/updates/_popup_upload_plugin.php(1 hunks)modules/system/controllers/updates/_update_list.php(1 hunks)modules/system/controllers/updates/_version.php(1 hunks)modules/system/controllers/updates/assets/.eslintignore(1 hunks)modules/system/controllers/updates/assets/src/components/MarketPlace.vue(1 hunks)modules/system/controllers/updates/assets/src/components/Pagination.vue(1 hunks)modules/system/controllers/updates/assets/src/components/Product.vue(1 hunks)modules/system/controllers/updates/assets/src/tailwind.config.js(1 hunks)modules/system/controllers/updates/assets/src/updates.css(1 hunks)modules/system/controllers/updates/assets/src/updates.js(1 hunks)modules/system/controllers/updates/assets/src/utils/message.js(1 hunks)modules/system/controllers/updates/assets/src/utils/numbers.js(1 hunks)modules/system/controllers/updates/assets/src/utils/winter-request.js(1 hunks)modules/system/controllers/updates/config_list.yaml(1 hunks)modules/system/controllers/updates/config_manage_list.yaml(0 hunks)modules/system/controllers/updates/form.plugin_upload.yaml(1 hunks)modules/system/controllers/updates/form.theme_upload.yaml(1 hunks)modules/system/controllers/updates/index.php(1 hunks)modules/system/controllers/updates/install.php(1 hunks)modules/system/controllers/updates/manage.php(0 hunks)modules/system/controllers/updates/traits/ManagesMarketplaceProject.php(1 hunks)modules/system/controllers/updates/traits/ManagesPlugins.php(1 hunks)modules/system/controllers/updates/traits/ManagesThemes.php(1 hunks)modules/system/lang/en/lang.php(3 hunks)modules/system/models/MailSetting.php(2 hunks)modules/system/models/Parameter.php(1 hunks)modules/system/models/PluginVersion.php(2 hunks)modules/system/models/pluginversion/columns.yaml(2 hunks)modules/system/models/pluginversion/columns_manage.yaml(0 hunks)modules/system/package.json(2 hunks)modules/system/postcss.config.mjs(1 hunks)modules/system/reportwidgets/Status.php(1 hunks)modules/system/tests/README.md(1 hunks)
⛔ Files not processed due to max files limit (33)
- modules/system/tests/bootstrap/PluginManagerTestCase.php
- modules/system/tests/bootstrap/PluginTestCase.php
- modules/system/tests/classes/CoreLangTest.php
- modules/system/tests/classes/PluginManagerTest.php
- modules/system/tests/classes/UpdateManagerTest.php
- modules/system/tests/classes/VersionManagerTest.php
- modules/system/tests/console/asset/mix/MixCreateTest.php
- modules/system/tests/console/asset/vite/BaseViteTestCase.php
- modules/system/tests/console/asset/vite/ViteCompileTest.php
- modules/system/tests/console/asset/vite/ViteCreateTest.php
- modules/system/tests/fixtures/plugins/database/tester/Plugin.php
- modules/system/tests/fixtures/plugins/dependencytest/acme/Plugin.php
- modules/system/tests/fixtures/plugins/dependencytest/dependency/Plugin.php
- modules/system/tests/fixtures/plugins/dependencytest/found/Plugin.php
- modules/system/tests/fixtures/plugins/dependencytest/notfound/Plugin.php
- modules/system/tests/fixtures/plugins/dependencytest/wrongcase/Plugin.php
- modules/system/tests/fixtures/plugins/mix/testa/Plugin.php
- modules/system/tests/fixtures/plugins/mix/testb/Plugin.php
- modules/system/tests/fixtures/plugins/mix/testc/Plugin.php
- modules/system/tests/fixtures/plugins/testvendor/goto/Plugin.php
- modules/system/tests/fixtures/plugins/testvendor/test/Plugin.php
- modules/system/tests/fixtures/plugins/winter/invalidreplacement/Plugin.php
- modules/system/tests/fixtures/plugins/winter/noupdates/Plugin.php
- modules/system/tests/fixtures/plugins/winter/original/Plugin.php
- modules/system/tests/fixtures/plugins/winter/replacement/Plugin.php
- modules/system/tests/fixtures/plugins/winter/replacenotinstalled/Plugin.php
- modules/system/tests/fixtures/plugins/winter/sample/Plugin.php
- modules/system/tests/fixtures/plugins/winter/tester/Plugin.php
- modules/system/traits/AssetMaker.php
- modules/system/traits/InteractsWithZip.php
- modules/system/vite.config.mjs
- modules/system/winter.mix.js
- plugins/winter/demo/Plugin.php
💤 Files with no reviewable changes (9)
- modules/system/controllers/updates/_is_enabled.php
- modules/system/console/WinterInstall.php
- modules/system/controllers/updates/_list_toolbar.php
- modules/system/console/PluginInstall.php
- modules/system/controllers/updates/_list_manage_toolbar.php
- modules/system/models/pluginversion/columns_manage.yaml
- modules/system/controllers/updates/_is_unfrozen.php
- modules/system/controllers/updates/config_manage_list.yaml
- modules/system/controllers/updates/manage.php
🧰 Additional context used
🪛 Biome (2.1.2)
modules/system/controllers/updates/assets/src/updates.css
[error] 47-47: expected } but instead the file ends
the file ends here
(parse)
🪛 GitHub Actions: Tests
modules/system/classes/extensions/PluginBase.php
[error] 27-27: While running 'php artisan package:discover', PHP Fatal error: Trait 'Winter\Storm\Support\Traits\HasComposerPackage' not found in PluginBase.php (line 27).
🪛 PHPMD (2.15.0)
modules/system/console/plugin/PluginInstall.php
112-112: Avoid unused local variables such as '$pluginsToMigrate'. (undefined)
(UnusedLocalVariable)
modules/system/classes/core/MarketPlaceApi.php
303-303: Avoid unused parameters such as '$postData'. (undefined)
(UnusedFormalParameter)
modules/system/controllers/updates/traits/ManagesPlugins.php
45-45: Avoid unused local variables such as '$name'. (undefined)
(UnusedLocalVariable)
194-194: Avoid unused local variables such as '$manager'. (undefined)
(UnusedLocalVariable)
modules/cms/classes/ThemeManager.php
123-123: Avoid unused parameters such as '$extension'. (undefined)
(UnusedFormalParameter)
123-123: Avoid unused parameters such as '$flag'. (undefined)
(UnusedFormalParameter)
128-128: Avoid unused parameters such as '$extension'. (undefined)
(UnusedFormalParameter)
128-128: Avoid unused parameters such as '$flag'. (undefined)
(UnusedFormalParameter)
133-133: Avoid unused parameters such as '$extension'. (undefined)
(UnusedFormalParameter)
133-133: Avoid unused parameters such as '$migrationsOnly'. (undefined)
(UnusedFormalParameter)
139-139: Avoid unused parameters such as '$extension'. (undefined)
(UnusedFormalParameter)
144-144: Avoid unused parameters such as '$extension'. (undefined)
(UnusedFormalParameter)
144-144: Avoid unused parameters such as '$targetVersion'. (undefined)
(UnusedFormalParameter)
155-155: Avoid unused parameters such as '$noRollback'. (undefined)
(UnusedFormalParameter)
modules/system/classes/extensions/ModuleManager.php
53-53: Avoid unused parameters such as '$extension'. (undefined)
(UnusedFormalParameter)
72-72: Avoid unused parameters such as '$extension'. (undefined)
(UnusedFormalParameter)
72-72: Avoid unused parameters such as '$flag'. (undefined)
(UnusedFormalParameter)
78-78: Avoid unused parameters such as '$extension'. (undefined)
(UnusedFormalParameter)
78-78: Avoid unused parameters such as '$flag'. (undefined)
(UnusedFormalParameter)
168-168: Avoid unused parameters such as '$targetVersion'. (undefined)
(UnusedFormalParameter)
modules/system/classes/extensions/PluginManager.php
355-355: Avoid unused local variables such as '$code'. (undefined)
(UnusedLocalVariable)
| public function getVersion(): string | ||
| { | ||
| if (isset($this->version)) { | ||
| return $this->version; | ||
| } | ||
|
|
||
| $versions = $this->getVersionsFromYaml( | ||
| versionFile: $this->getPath() . DIRECTORY_SEPARATOR . 'version.yaml', | ||
| includeScripts: false | ||
| ); | ||
| if (empty($versions)) { | ||
| return $this->version = '0'; | ||
| } | ||
|
|
||
| return $this->version = trim(key(array_slice($versions, -1, 1))); | ||
| } |
There was a problem hiding this comment.
getVersion() returns "0"/wrong key due to array_slice reindexing; use array_key_last instead.
array_slice reindexes keys, so key(array_slice(...)) yields 0. Return the actual last version key.
Apply this diff:
public function getVersion(): string
{
if (isset($this->version)) {
return $this->version;
}
$versions = $this->getVersionsFromYaml(
versionFile: $this->getPath() . DIRECTORY_SEPARATOR . 'version.yaml',
includeScripts: false
);
if (empty($versions)) {
return $this->version = '0';
}
- return $this->version = trim(key(array_slice($versions, -1, 1)));
+ $last = array_key_last($versions);
+ return $this->version = is_string($last) ? trim($last) : '0';
}🤖 Prompt for AI Agents
In modules/cms/classes/Theme.php around lines 728 to 743, the current code uses
key(array_slice($versions, -1, 1)) which reindexes the array and returns 0
instead of the actual last version key; replace that logic with array_key_last
to get the real last key, assign it to $this->version and return its trimmed
string value (fall back to '0' if array_key_last returns null/false). Make the
change so you compute $lastKey = array_key_last($versions); if ($lastKey ===
null || $lastKey === false) return $this->version = '0'; otherwise return
$this->version = trim((string)$lastKey).
| /** | ||
| * Flushes local cache | ||
| * | ||
| * @return $this | ||
| */ | ||
| public function forgetCache(): static | ||
| { | ||
| // unset($this->databaseHistory, $this->databaseVersions, $this->fileVersions); | ||
|
|
||
| return $this; | ||
| } |
There was a problem hiding this comment.
forgetCache() currently does nothing.
It should clear $databaseHistory, $databaseVersions, and $fileVersions; otherwise callers will get stale data.
Apply this diff:
public function forgetCache(): static
{
-// unset($this->databaseHistory, $this->databaseVersions, $this->fileVersions);
+ unset($this->databaseHistory, $this->databaseVersions, $this->fileVersions);
return $this;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Flushes local cache | |
| * | |
| * @return $this | |
| */ | |
| public function forgetCache(): static | |
| { | |
| // unset($this->databaseHistory, $this->databaseVersions, $this->fileVersions); | |
| return $this; | |
| } | |
| /** | |
| * Flushes local cache | |
| * | |
| * @return $this | |
| */ | |
| public function forgetCache(): static | |
| { | |
| unset($this->databaseHistory, $this->databaseVersions, $this->fileVersions); | |
| return $this; | |
| } |
🤖 Prompt for AI Agents
In modules/system/classes/extensions/PluginVersionManager.php around lines 666
to 676, forgetCache() currently returns $this without clearing any cached state;
restore clearing of the cache by resetting $this->databaseHistory,
$this->databaseVersions, and $this->fileVersions (e.g., unset them or set them
to null/empty arrays as appropriate for the class) before returning $this so
callers no longer receive stale data.
| <?php | ||
|
|
||
| namespace System\Classes\Extensions; | ||
|
|
||
| use Cms\Classes\Theme; | ||
| use Illuminate\Support\Facades\Storage; | ||
| use System\Traits\InteractsWithZip; | ||
| use Winter\Storm\Exception\ApplicationException; | ||
| use Winter\Storm\Foundation\Extension\WinterExtension; | ||
| use Winter\Storm\Support\ModuleServiceProvider; | ||
| use Winter\Storm\Support\Traits\Singleton; | ||
|
|
||
| class Preserver | ||
| { | ||
| use Singleton; | ||
| use InteractsWithZip; | ||
|
|
||
| public const ROOT_PATH = 'archive'; | ||
|
|
||
| protected array $classMap = [ | ||
| ModuleServiceProvider::class => 'modules', | ||
| PluginBase::class => 'plugins', | ||
| Theme::class => 'themes', | ||
| ]; |
There was a problem hiding this comment.
Missing import for PluginBase class.
Line 22 references PluginBase::class in the $classMap array, but there's no corresponding import statement at the top of the file.
Add the missing import:
use Cms\Classes\Theme;
use Illuminate\Support\Facades\Storage;
+use System\Classes\Extensions\PluginBase;
use System\Traits\InteractsWithZip;🤖 Prompt for AI Agents
In modules/system/classes/extensions/Preserver.php around lines 1 to 24, the
$classMap references PluginBase::class but PluginBase is not imported; add the
missing import statement "use System\Classes\PluginBase;" alongside the other
use statements at the top of the file (grouped/sorted with the existing imports)
so the class reference resolves correctly.
| use Winter\Packager\Exceptions\CommandException; | ||
| use Winter\Storm\Exception\ApplicationException; |
There was a problem hiding this comment.
Wrong CommandException namespace — composer errors won’t be caught.
Composer is from Winter\Storm\Packager\Composer but you catch Winter\Packager\Exceptions\CommandException. This will miss actual exceptions.
Apply this diff to fix both catch sites:
- use Winter\Packager\Exceptions\CommandException;
+ use Winter\Storm\Packager\Exceptions\CommandException;Also applies to: 120-129, 232-237
🤖 Prompt for AI Agents
In modules/system/classes/extensions/source/ExtensionSource.php around lines
16-17, 120-129, and 232-237 the code imports and catches
Winter\Packager\Exceptions\CommandException which is the wrong namespace for
Composer-related errors; update the import to
Winter\Storm\Packager\Exceptions\CommandException (or fully-qualify that correct
namespace where used) and update the corresponding catch clauses so they catch
the corrected CommandException type (ensuring any existing catch order remains
correct and Composer errors will be caught).
| public static function fromZip(string $path): array | ||
| { | ||
| if (!File::exists($path)) { | ||
| throw new ApplicationException("$path doesn't exist"); | ||
| } | ||
|
|
||
| $dir = temp_path(time()); | ||
|
|
||
| if (!File::exists($dir)) { | ||
| File::makeDirectory($dir); | ||
| } | ||
|
|
||
| (new class { | ||
| use InteractsWithZip; | ||
| })->extractArchive($path, $dir); |
There was a problem hiding this comment.
Potential directory name collision and path exposure.
Line 34 uses time() to create a temporary directory name, which can collide if fromZip is called multiple times within the same second.
Line 31 exposes the server path in the error message, which could be a minor information disclosure issue.
- if (!File::exists($path)) {
- throw new ApplicationException("$path doesn't exist");
+ if (!File::exists($path)) {
+ throw new ApplicationException("Provided file path doesn't exist");
}
- $dir = temp_path(time());
+ $dir = temp_path(uniqid('extension_', true));🤖 Prompt for AI Agents
In modules/system/classes/extensions/source/LocalSource.php around lines 28 to
42, the current implementation uses time() to generate a temporary directory
(risking collisions if called multiple times per second) and throws an
ApplicationException that contains the full server path (potential path
disclosure). Replace the time()-based name with a securely unique temporary
directory name (e.g., using uniqid or a system temp utility) and create it
atomically (check for existence and retry or use a temp directory helper) and
change the exception message to a generic user-safe message (e.g., "Archive file
not found") that does not expose the server filesystem path; ensure directory
creation uses proper flags (recursive/permissions) as needed.
| public function onInstallUploadedPlugin(): string | ||
| { | ||
| try { | ||
| // Get the deferred binding record for the uploaded file | ||
| $widget = $this->getPackageUploadWidget(); | ||
| $class = str_contains($class = Str::before(get_class($widget->model), chr(0)), '\\\\') | ||
| ? str_replace('\\\\', '\\', $class) | ||
| : $class; | ||
|
|
||
| $deferred = DeferredBinding::query() | ||
| ->where('master_type', 'LIKE', $class . '%') | ||
| ->where('master_field', 'uploaded_package') | ||
| ->where('session_key', $widget->getSessionKey()) | ||
| ->first(); | ||
|
|
||
| // Attempt to get the file from the deferred binding | ||
| if (!$deferred || !$deferred->slave) { | ||
| throw new ApplicationException(Lang::get('system::lang.server.response_invalid')); | ||
| } | ||
|
|
||
| $file = $deferred->slave; | ||
| $localPath = $file->disk_name; | ||
| if (!FileHelper::copyBetweenDisks($file->getDisk(), 'temp', $file->getDiskPath(), $localPath)) { | ||
| throw new ApplicationException(Lang::get('system::lang.server.shit_gone_fucky')); | ||
| } | ||
|
|
||
| /** | ||
| * @TODO: | ||
| * - Process the uploaded file to identify the plugins to install | ||
| * - (optional) require confirmation to install each detected plugin | ||
| * - Install the identified plugins | ||
| * - Ensure that deferred binding records and uploaded files are removed post processing or on failure | ||
| */ | ||
|
|
||
| $manager = UpdateManager::instance(); | ||
|
|
||
| // @TODO: Implement | ||
| // $result = $manager->installUploadedPlugin(Storage::disk('temp')->path($localPath)); | ||
|
|
||
| if (!isset($result['code']) || !isset($result['hash'])) { | ||
| throw new ApplicationException(Lang::get('system::lang.server.response_invalid')); | ||
| } | ||
|
|
||
| $name = $result['code']; | ||
| $hash = $result['hash']; | ||
| $plugins = [$name => $hash]; | ||
| $plugins = $this->appendRequiredPlugins($plugins, $result); | ||
|
|
||
| /* | ||
| * Update steps | ||
| */ | ||
| $updateSteps = $this->buildUpdateSteps(null, $plugins, [], true); | ||
|
|
||
| /* | ||
| * Finish up | ||
| */ | ||
| $updateSteps[] = [ | ||
| 'code' => 'completeInstall', | ||
| 'label' => Lang::get('system::lang.install.install_completing'), | ||
| ]; | ||
|
|
||
| $this->vars['updateSteps'] = $updateSteps; | ||
|
|
||
| return $this->makePartial('execute'); | ||
| } | ||
| catch (Exception $ex) { | ||
| // @TODO: Remove this, temporary debugging | ||
| throw $ex; | ||
| $this->handleError($ex); | ||
| return $this->makePartial('plugin_uploader'); | ||
| } | ||
| } |
There was a problem hiding this comment.
Uploaded-plugin install handler is incomplete and always fails; remove debug throw and fix placeholders.
$resultis never set; the check will always fail.throw $ex;in catch makes the later error handling unreachable.- Message key
system::lang.server.shit_gone_fuckyis inappropriate and likely missing. $manageris unused.
Apply this interim hardening diff to avoid crashing until implementation lands:
@@
- if (!FileHelper::copyBetweenDisks($file->getDisk(), 'temp', $file->getDiskPath(), $localPath)) {
- throw new ApplicationException(Lang::get('system::lang.server.shit_gone_fucky'));
- }
+ if (!FileHelper::copyBetweenDisks($file->getDisk(), 'temp', $file->getDiskPath(), $localPath)) {
+ throw new ApplicationException(Lang::get('system::lang.server.file_copy_failed'));
+ }
@@
- $manager = UpdateManager::instance();
+ // $manager = UpdateManager::instance(); // TODO: use when implementing installUploadedPlugin()
@@
- if (!isset($result['code']) || !isset($result['hash'])) {
- throw new ApplicationException(Lang::get('system::lang.server.response_invalid'));
- }
+ // TODO: Implement installUploadedPlugin() and populate $result; for now bail out gracefully.
+ throw new ApplicationException(Lang::get('system::lang.updates.not_implemented'));
@@
- catch (Exception $ex) {
- // @TODO: Remove this, temporary debugging
- throw $ex;
- $this->handleError($ex);
- return $this->makePartial('plugin_uploader');
- }
+ catch (Exception $ex) {
+ $this->handleError($ex);
+ return $this->makePartial('plugin_uploader');
+ }Also remove the unused $name variable in details() as flagged by PHPMD.
🧰 Tools
🪛 PHPMD (2.15.0)
194-194: Avoid unused local variables such as '$manager'. (undefined)
(UnusedLocalVariable)
| public function onInstallPackage(): array | ||
| { | ||
| if (!$code = trim(post('package'))) { | ||
| throw new ApplicationException(Lang::get('system::lang.install.missing_package_name')); | ||
| } | ||
|
|
||
| if (!($type = trim(post('type'))) || !in_array($type, ['plugin', 'theme', 'module'])) { | ||
| throw new ApplicationException(Lang::get('system::lang.install.invalid_package_type')); | ||
| } | ||
|
|
||
| $key = base64_encode($this->cachePrefix . Session::getId() . md5(time() . $code)); | ||
|
|
||
| App::terminating(function () use ($code, $type, $key) { | ||
| $output = new class extends BufferedOutput { | ||
| protected string $key; | ||
|
|
||
| protected function doWrite(string $message, bool $newline): void | ||
| { | ||
| Cache::put($this->key, Cache::get($this->key, '') . trim($message) . ($newline ? "\n" : '')); | ||
| } | ||
|
|
||
| public function setKey(string $key): void | ||
| { | ||
| $this->key = $key; | ||
| } | ||
| }; | ||
|
|
||
| $output->setKey($key); | ||
|
|
||
| switch ($type) { | ||
| case 'plugin': | ||
| PluginManager::instance()->setOutput(new OutputStyle(new ArrayInput([]), $output)); | ||
| break; | ||
| case 'theme': | ||
| ThemeManager::instance()->setOutput(new OutputStyle(new ArrayInput([]), $output)); | ||
| break; | ||
| } | ||
|
|
||
| try { | ||
| $response = (new ComposerSource(match ($type) { | ||
| 'plugin' => ExtensionSource::TYPE_PLUGIN, | ||
| 'theme' => ExtensionSource::TYPE_THEME, | ||
| 'module' => ExtensionSource::TYPE_MODULE, | ||
| }, composerPackage: $code))->install(); | ||
| } catch (\Throwable $e) { | ||
| $response = null; | ||
| Cache::put($key, Cache::get($key, '') . 'ERROR: ' . implode(PHP_EOL, [ | ||
| $e->getMessage(), | ||
| $e->getFile() . '@' . $e->getLine(), | ||
| $e->getTraceAsString(), | ||
| ])); | ||
| } finally { | ||
| Cache::put($key, Cache::get($key, '') . 'FINISHED:' . ($response ? 'SUCCESS' : 'FAILED')); | ||
| } | ||
| }); | ||
|
|
||
| return [ | ||
| 'install_key' => $key | ||
| ]; | ||
| } |
There was a problem hiding this comment.
Avoid leaking stack traces to the UI; add TTL for cached logs and support module output.
- Writing
$e->getFile(),$e->getTraceAsString()to a cache that’s returned to the client exposes sensitive details. - No TTL set on
Cache::put, causing unbounded growth if not cleaned. - Missing output routing for
moduleinstalls.
Apply this diff:
@@
- switch ($type) {
+ switch ($type) {
case 'plugin':
PluginManager::instance()->setOutput(new OutputStyle(new ArrayInput([]), $output));
break;
case 'theme':
ThemeManager::instance()->setOutput(new OutputStyle(new ArrayInput([]), $output));
break;
+ case 'module':
+ \System\Classes\Extensions\ModuleManager::instance()->setOutput(new OutputStyle(new ArrayInput([]), $output));
+ break;
}
@@
- } catch (\Throwable $e) {
- $response = null;
- Cache::put($key, Cache::get($key, '') . 'ERROR: ' . implode(PHP_EOL, [
- $e->getMessage(),
- $e->getFile() . '@' . $e->getLine(),
- $e->getTraceAsString(),
- ]));
- } finally {
- Cache::put($key, Cache::get($key, '') . 'FINISHED:' . ($response ? 'SUCCESS' : 'FAILED'));
+ } catch (\Throwable $e) {
+ $response = null;
+ // Log full details server-side, return minimal error to client
+ \Log::error('[install] ' . $e->getMessage(), ['exception' => $e]);
+ Cache::put($key, Cache::get($key, '') . 'ERROR: ' . $e->getMessage() . PHP_EOL, now()->addMinutes(15));
+ } finally {
+ Cache::put($key, Cache::get($key, '') . 'FINISHED:' . ($response ? 'SUCCESS' : 'FAILED'), now()->addMinutes(15));
}Also set a TTL in doWrite() calls to avoid indefinite growth (see next comment).
🤖 Prompt for AI Agents
In modules/system/controllers/updates/traits/ManagesPlugins.php around lines 238
to 297, replace direct dumping of exception file/trace into the cache with a
safe, generic error string for the client and send the full exception to the
server log; ensure every Cache::put call (including inside the anonymous
BufferedOutput::doWrite and in the catch/finally blocks) supplies a TTL (e.g.
3600 seconds) to avoid indefinite growth; add a 'module' branch in the switch to
route output for module installs (similar to plugin/theme) so module install
output is captured; and in the catch block write only a non-sensitive message
like "ERROR: {message}" to the cache while logging $e via Log::error($e) or
similar.
| { | ||
| if (!$key = trim(post('install_key'))) { | ||
| throw new ApplicationException(Lang::get('system::lang.install.missing_plugin_name')); | ||
| } | ||
|
|
||
| if (!str_starts_with(base64_decode($key), $this->cachePrefix . Session::getId())) { | ||
| throw new ApplicationException(Lang::get('system::lang.server.response_invalid')); | ||
| } | ||
|
|
||
| $data = Cache::get($key, ''); | ||
|
|
||
| return [ | ||
| 'done' => !$data || str_contains($data, 'FINISHED:SUCCESS') || str_contains($data, 'FINISHED:FAILED'), | ||
| 'success' => str_contains($data, 'FINISHED:SUCCESS'), | ||
| 'data' => $data | ||
| ]; | ||
| } |
There was a problem hiding this comment.
Status endpoint marks empty output as “done”.
Treating empty cache as done causes immediate completion on the first poll.
Apply this diff:
- 'done' => !$data || str_contains($data, 'FINISHED:SUCCESS') || str_contains($data, 'FINISHED:FAILED'),
+ 'done' => str_contains($data, 'FINISHED:SUCCESS') || str_contains($data, 'FINISHED:FAILED'),
'success' => str_contains($data, 'FINISHED:SUCCESS'),Also adjust the error message for a missing key:
- if (!$key = trim(post('install_key'))) {
- throw new ApplicationException(Lang::get('system::lang.install.missing_plugin_name'));
- }
+ if (!$key = trim(post('install_key'))) {
+ throw new ApplicationException(Lang::get('system::lang.install.missing_install_key'));
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| { | |
| if (!$key = trim(post('install_key'))) { | |
| throw new ApplicationException(Lang::get('system::lang.install.missing_plugin_name')); | |
| } | |
| if (!str_starts_with(base64_decode($key), $this->cachePrefix . Session::getId())) { | |
| throw new ApplicationException(Lang::get('system::lang.server.response_invalid')); | |
| } | |
| $data = Cache::get($key, ''); | |
| return [ | |
| 'done' => !$data || str_contains($data, 'FINISHED:SUCCESS') || str_contains($data, 'FINISHED:FAILED'), | |
| 'success' => str_contains($data, 'FINISHED:SUCCESS'), | |
| 'data' => $data | |
| ]; | |
| } | |
| { | |
| if (!$key = trim(post('install_key'))) { | |
| throw new ApplicationException(Lang::get('system::lang.install.missing_install_key')); | |
| } | |
| if (!str_starts_with(base64_decode($key), $this->cachePrefix . Session::getId())) { | |
| throw new ApplicationException(Lang::get('system::lang.server.response_invalid')); | |
| } | |
| $data = Cache::get($key, ''); | |
| return [ | |
| 'done' => str_contains($data, 'FINISHED:SUCCESS') || str_contains($data, 'FINISHED:FAILED'), | |
| 'success' => str_contains($data, 'FINISHED:SUCCESS'), | |
| 'data' => $data | |
| ]; | |
| } |
🤖 Prompt for AI Agents
In modules/system/controllers/updates/traits/ManagesPlugins.php around lines
300–316, the endpoint currently treats an empty cache value as "done" and uses
an incorrect/misleading error message key for a missing install key. Change the
missing-key check to throw
ApplicationException(Lang::get('system::lang.install.missing_install_key')) (or
the correct language key for a missing install key), and update the return so
"done" is true only when the cached data contains FINISHED:SUCCESS or
FINISHED:FAILED (do not treat empty/blank cache as done); keep "success" true
only when FINISHED:SUCCESS and return the raw data as-is.
| use Illuminate\Http\RedirectResponse; | ||
| use Illuminate\Support\Facades\Lang; | ||
| use Illuminate\Support\Facades\Redirect; | ||
| use October\Rain\Exception\ApplicationException; |
There was a problem hiding this comment.
Use Winter namespace for ApplicationException.
Line 10 imports October\Rain\Exception\ApplicationException, but Winter CMS should use Winter\Storm\Exception\ApplicationException for consistency with the framework's namespace.
-use October\Rain\Exception\ApplicationException;
+use Winter\Storm\Exception\ApplicationException;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| use October\Rain\Exception\ApplicationException; | |
| use Winter\Storm\Exception\ApplicationException; |
🤖 Prompt for AI Agents
In modules/system/controllers/updates/traits/ManagesThemes.php around line 10,
the code imports October\Rain\Exception\ApplicationException but Winter CMS uses
the Winter namespace; replace that import with
Winter\Storm\Exception\ApplicationException so the correct ApplicationException
class from Winter\Storm is referenced and update any references if namespaced
aliasing is used.
| is_unfrozen: | ||
| label: system::lang.plugins.unfrozen | ||
| type: partial | ||
| path: is_unfrozen | ||
| sortable: false | ||
|
|
||
| is_enabled: | ||
| label: system::lang.plugins.enabled | ||
| type: partial | ||
| path: is_enabled | ||
| sortable: false |
There was a problem hiding this comment.
🧩 Analysis chain
Verify partial files and language keys for new status columns.
The new is_unfrozen and is_enabled columns reference partial views and language keys that may not exist yet.
🏁 Script executed:
#!/bin/bash
# Verify partials and language keys for is_unfrozen and is_enabled columns
# Check for partial view files
echo "=== Checking for partial files ==="
fd -t f "_is_unfrozen.htm" modules/system/models/pluginversion/
fd -t f "_is_enabled.htm" modules/system/models/pluginversion/
# Check for language key definitions
echo "=== Checking for language keys ==="
rg -n "['\"](unfrozen|enabled)['\"]" modules/system/lang/ --type=php
# Check if these keys are defined in the plugins language file
rg -n "plugins\.unfrozen|plugins\.enabled" modules/system/lang/ --type=phpLength of output: 3132
---
🏁 Script executed:
```shell
#!/bin/bash
# Get broader context on partial files and directory structure
echo "=== Files in pluginversion directory ==="
fd -t f . modules/system/models/pluginversion/ | head -20
echo "=== All .htm files in pluginversion directory ==="
fd -t f "\.htm" modules/system/models/pluginversion/
echo "=== Search for is_unfrozen or is_enabled anywhere in codebase ==="
fd -t f -e htm | xargs grep -l "is_unfrozen\|is_enabled" 2>/dev/null || echo "No matches found"
echo "=== Check where other partials like is_active, status, etc. exist ==="
fd -t f "_is_" modules/system/models/pluginversion/
Length of output: 292
---
🏁 Script executed:
```shell
#!/bin/bash
# Find where model partials are typically stored in the system module
echo "=== Search for _*.htm partial files in system module ==="
fd -t f "_.*\.htm" modules/system/ | head -15
echo "=== Check directory structure of system module ==="
fd -t d . modules/system/ -max-depth 3 | head -20
echo "=== Look for any partials or views directory in system ==="
fd -t d "partials|views|templates" modules/system/
Length of output: 706
---
🏁 Script executed:
```shell
#!/bin/bash
# Check contents of system/partials directory
echo "=== Files in modules/system/partials/ ==="
fd -t f . modules/system/partials/
echo "=== Look for any is_* partial files ==="
fd -t f "is_" modules/system/partials/
echo "=== Count how many partials exist in system/partials ==="
fd -t f . modules/system/partials/ | wc -l
Length of output: 327
Create missing partial view files for is_unfrozen and is_enabled columns.
The language keys are properly defined across all translations, but the referenced partial view files are missing. Create _is_unfrozen.htm and _is_enabled.htm in modules/system/partials/ to render these status columns.
🤖 Prompt for AI Agents
In modules/system/models/pluginversion/columns.yaml around lines 26 to 36 the
columns reference missing partials is_unfrozen and is_enabled; create the
partial view files modules/system/partials/_is_unfrozen.htm and
modules/system/partials/_is_enabled.htm that render the plugin status (use the
provided language keys system::lang.plugins.unfrozen and
system::lang.plugins.enabled), output appropriate status markup (e.g.,
icon/label classes for enabled/disabled and frozen/unfrozen states) and ensure
they accept the column value/context used by the List/Columns renderer so the
status displays correctly in the backend list.
Me and Luke are working on a little refactor of how the UpdateManager works...
It also includes some components from my old composer PR found here: #967
The core idea of this is that there should be consistent interfaces to interact with different things built on top of Winter (i.e.
modules,plugins&themes). Each of these elements will now have their class (i.e.Theme,PluginBase,ModuleServiceProvider) will now extendWinterExtensionwhich will allow them to all interact with the update manager and other system functions directly.In addition
PluginManager,ThemeManager&ModuleManager(new) will all implement theExtensionManagerinterface, which will allow for a consistant way to interact with all Winter extensions.I.e. You will be able to call
PluginManager::instance()->update($plugin)orThemeManager::instance()->update($theme).This also allows us to do some cool stuff like (only support for plugin installing is currently functional)
By the end of the PR, the UpdateManager may no longer exist as is, instead each
ExtensionManagerwill be responsible for its existing functionality.Requires storm branch: https://github.com/wintercms/storm/tree/wip/support-uploading-packages-update-manager-changes
Summary by CodeRabbit
New Features
Improvements