feat: Enhance plugin installation command to support 'download-only' option #2464
Conversation
WalkthroughAdds a download-only mode and Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Script as plugin (script)
participant SysVer as /etc/unraid-version (check_version)
participant Fetcher as Downloader (/tmp/plugins)
participant Hooks
participant Runner
participant Logger
User->>Script: plugin [download|install|update] <PLUGIN-FILE> [TARGET-VERSION]
Script->>Script: parse argv → script, method, optional_args, set $download_only
Script->>SysVer: read /etc/unraid-version -> $check_version
Script->>Fetcher: fetch/stage FILE(s) -> /tmp/plugins
alt TARGET-VERSION present
Script->>SysVer: compare TARGET-VERSION vs $check_version
end
alt download-only
Fetcher-->>Script: files staged
Script->>Logger: log "downloaded / staged"
Script->>Hooks: skip pre_hooks/post_hooks
Script->>Runner: skip Run entries
Script->>Logger: log completion (downloaded/staged)
else normal install/update
Fetcher-->>Script: files staged
Script->>Hooks: execute pre_hooks
Script->>Runner: execute Run entries
Script->>Hooks: execute post_hooks
Script->>Logger: log completion (installed/executed)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
🔧 PR Test Plugin AvailableA test plugin has been generated for this PR that includes the modified files. Version: 📥 Installation Instructions:Install via Unraid Web UI:
Alternative: Direct Download
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
emhttp/plugins/dynamix.plugin.manager/scripts/plugin (1)
752-767: Reconsider plugin registration in download-only mode.In download-only mode, the plugin is still registered as installed (symlink created in
/var/log/plugins, copied to/boot/config/plugins, logged as "installed"). However, since Run commands were skipped, the plugin may not be functional. This creates an inconsistent state where:
- The plugin appears installed to the system and Plugin Manager
- Required services, dependencies, or configurations may not be set up
- At next boot, the plugin will attempt to install again from
/boot/config/pluginsThis could lead to user confusion and potential system issues.
Consider one of these approaches:
Option 1 (Recommended): Don't register the plugin as installed in download-only mode:
// register successful install $target = "$boot/$plugin"; + if ($download_only) { + write("plugin: $plugin files downloaded (not installed)\n"); + my_logger("$plugin files downloaded only", $logger); + } else { if (!plugin('noInstall', $plugin_file, $error)) { if ($target != $plugin_file) copy($plugin_file, $target); symlink($target, $symlink); write("plugin: $plugin installed\n"); my_logger("$plugin installed", $logger); } else { write("script: $plugin executed\n"); my_logger("script: $plugin executed", $logger); } + }Option 2: Store downloaded plugins in a separate location (e.g.,
/boot/config/plugins-downloaded) to distinguish them from installed plugins.
🧹 Nitpick comments (1)
emhttp/plugins/dynamix.plugin.manager/scripts/plugin (1)
17-28: Clarify usage text regarding plugin registration behavior.The usage text indicates that
download-onlywill "download and replace plugin files without executing any Run commands," but doesn't mention that the plugin will still be registered as installed (symlink created in/var/log/plugins). This could mislead users into thinking the plugin is not installed when it actually is.Consider updating the usage text to clarify:
- download-only is optional and can be used to download and replace plugin files without executing any - Run commands defined for the install method. + download-only is optional and can be used to download and replace plugin files without executing any + Run commands defined for the install method. The plugin will still be registered as installed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
emhttp/plugins/dynamix.plugin.manager/scripts/plugin(14 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-05T23:34:04.847Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2355
File: emhttp/plugins/dynamix.plugin.manager/include/PluginHelpers.php:21-28
Timestamp: 2025-09-05T23:34:04.847Z
Learning: In the Unraid webGUI plugin system, the static $methods array in PluginHelpers.php contains only methods that should bypass caching and call the plugin script directly. Methods like 'checkall' are handled at the PHP wrapper level via recursive calls to the plugin() function, not as direct subcommands in the plugin script itself.
Applied to files:
emhttp/plugins/dynamix.plugin.manager/scripts/plugin
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
emhttp/plugins/dynamix.plugin.manager/scripts/plugin (5)
495-504: LGTM - Variable initialization looks good.The initialization of global variables is correct. The new
$scriptvariable improves maintainability by storing$argv[0]once rather than referencing it multiple times.
612-614: LGTM - Clear user feedback.The message appropriately informs users that download-only mode is active and install commands will be skipped.
461-465: LGTM - Run command skipping is correctly implemented.The conditional logic properly skips Run command execution in download-only mode while still downloading/creating the files. The logging message clearly indicates why the command was skipped.
520-573: LGTM - Improved error message consistency.Using the
$scriptvariable instead of directly referencing$argv[0]improves code maintainability and ensures consistent error reporting across checkall, updateall, and checkos commands.
620-622: LGTM - Consistent hook skipping in download-only mode.The conditional execution of
pre_hooks()andpost_hooks()is correctly applied throughout all install flow paths (success, error, and early exit scenarios). This prevents hook scripts from running when the plugin is not fully installed.Also applies to: 629-631, 645-647, 657-659, 669-671, 679-681, 688-690, 699-701, 707-709, 714-716, 727-729, 736-738, 746-748, 764-766
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
emhttp/plugins/dynamix.plugin.manager/scripts/plugin (1)
619-620: Fix incorrect forced install logic (duplicate issue).The logic at line 620 incorrectly sets
$forced = truewhenever ANY option other than 'force' or 'forced' is present. This means the 'nchan' option (used for output control) would unintentionally bypass version checks.Apply this diff to fix the logic:
- $non_flag_options = array_diff($options, ['force', 'forced']); - $forced = in_array('force', $options, true) || in_array('forced', $options, true) || !empty($non_flag_options); + $forced = in_array('force', $options, true) || in_array('forced', $options, true);Based on learnings, this issue was previously identified and remains unresolved.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
emhttp/plugins/dynamix.plugin.manager/scripts/plugin(13 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-03-27T22:04:00.594Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2099
File: emhttp/plugins/dynamix.my.servers/include/activation-code-extractor.php:58-74
Timestamp: 2025-03-27T22:04:00.594Z
Learning: The file `emhttp/plugins/dynamix.my.servers/include/activation-code-extractor.php` is synced from a different repository, and modifications should not be suggested in this repository's context. Changes should be proposed in the source repository instead.
Applied to files:
emhttp/plugins/dynamix.plugin.manager/scripts/plugin
📚 Learning: 2025-09-05T23:34:04.847Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2355
File: emhttp/plugins/dynamix.plugin.manager/include/PluginHelpers.php:21-28
Timestamp: 2025-09-05T23:34:04.847Z
Learning: In the Unraid webGUI plugin system, the static $methods array in PluginHelpers.php contains only methods that should bypass caching and call the plugin script directly. Methods like 'checkall' are handled at the PHP wrapper level via recursive calls to the plugin() function, not as direct subcommands in the plugin script itself.
Applied to files:
emhttp/plugins/dynamix.plugin.manager/scripts/plugin
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (7)
emhttp/plugins/dynamix.plugin.manager/scripts/plugin (7)
26-36: LGTM! Clear documentation for the new download command.The usage documentation effectively explains the new download-only workflow and clarifies that FILE elements are processed similarly to install, minus the Run command execution.
297-297: LGTM! Proper global variable declaration.The
$download_onlyglobal variable is correctly added to support download-only mode checks within the plugin() function.
467-471: LGTM! Run command skip logic is correct.The guard properly prevents Run command execution in download-only mode while providing clear logging of skipped actions.
612-618: Verify intentional breaking change for legacy download-only flags.This code rejects the legacy
download-onlyanddownloadflags when used withplugin install, forcing users to use the newplugin downloadcommand instead. This is a breaking change from the previous behavior.Please confirm this breaking change is intentional and that users will be notified in release notes or migration documentation. If backward compatibility is desired, consider accepting the legacy flags with a deprecation warning instead of an error.
634-636: LGTM! Hook guards are properly implemented.All pre_hooks() and post_hooks() calls in the URL download and version validation paths are correctly guarded to skip execution in download-only mode.
Also applies to: 643-645, 659-662, 671-674
683-686: LGTM! Comprehensive hook guard coverage.All pre_hooks() and post_hooks() calls throughout the install/download workflow are consistently guarded with
!$download_onlychecks. This ensures hooks are not executed in download-only mode across all code paths including success, failure, and early exit scenarios.Also applies to: 693-696, 702-705, 713-716, 721-724, 728-730, 741-743, 750-752, 760-762, 780-782
625-629: LGTM! Clear and accurate messaging.The conditional messaging correctly differentiates between download-only and install operations, providing clear feedback to users about which action is being performed and the resulting status.
Also applies to: 771-777
elibosley
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
emhttp/plugins/dynamix.plugin.manager/scripts/plugin (1)
503-512: Refine nchan detection to avoid treating plugin filename as an optionAs previously noted, including PLUGIN-FILE in
$optional_argsmeans a plugin literally namednchan.plgwill incorrectly enable nchan mode. You can avoid this while still supportingcheckall/updateall/checkosby varying the slice offset based on whether the current method expects a plugin argument.$notify = '/usr/local/emhttp/webGui/scripts/notify'; $boot = '/boot/config/plugins'; $plugins = '/var/log/plugins'; $tmp = '/tmp/plugins'; $download_only = false; $script = $argv[0]; $method = $argv[1]; -$optional_args = array_slice($argv, 2); +$has_plugin_arg = !in_array($method, ['checkall', 'updateall', 'checkos'], true); +// Methods with a PLUGIN/PLUGIN-FILE argument have it at $argv[2], +// so option flags (including "nchan") begin at $argv[3]. +// Methods without a plugin argument (checkall/updateall/checkos) +// take options starting at $argv[2]. +$optional_args = array_slice($argv, $has_plugin_arg ? 3 : 2); $builtin = ['unRAIDServer','unRAIDServer-']; $nchan = in_array('nchan', $optional_args, true); // console or nchan outputThis preserves
plugin checkall nchan(options from index 2) while ensuringplugin install myplugin.plg nchanand similar commands don’t treat the plugin filename itself as thenchanflag.
🧹 Nitpick comments (1)
emhttp/plugins/dynamix.plugin.manager/scripts/plugin (1)
608-623: Download-only parsing and forced behavior are coherent with existing conventionsThe new
install/downloadbranch cleanly:
- Normalizes the plugin argument,
- Distinguishes options from the PLUGIN-FILE,
- Rejects legacy
plugin install ... download-onlyusage with a clear message, and- Computes
$forcedin a way that preserves the existing (intentional) behavior where any non-force/forcedoption implies a forced install/download.The extra “download-only mode enabled” and
$actionmessages should make CLI behavior much clearer for callers.If you expect future readers to be surprised by the “any non-flag option implies forced” rule, a short inline comment next to the
$forcedassignment explaining that this is historical/intentional behavior would help avoid accidental “simplifications”.Also applies to: 627-632
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
emhttp/plugins/dynamix.plugin.manager/scripts/plugin(11 hunks)
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2025-11-24T15:12:41.007Z
Learnt from: elibosley
Repo: unraid/webgui PR: 2464
File: emhttp/plugins/dynamix.plugin.manager/scripts/plugin:0-0
Timestamp: 2025-11-24T15:12:41.007Z
Learning: In emhttp/plugins/dynamix.plugin.manager/scripts/plugin, the $forced variable logic intentionally treats ANY option argument (beyond the explicitly handled ones like 'download-only', 'download', 'force', 'forced') as triggering forced install mode. This means that options like 'nchan' will cause $forced to be true. This is by design, even though it's unusual behavior.
Applied to files:
emhttp/plugins/dynamix.plugin.manager/scripts/plugin
📚 Learning: 2025-03-27T22:04:00.594Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2099
File: emhttp/plugins/dynamix.my.servers/include/activation-code-extractor.php:58-74
Timestamp: 2025-03-27T22:04:00.594Z
Learning: The file `emhttp/plugins/dynamix.my.servers/include/activation-code-extractor.php` is synced from a different repository, and modifications should not be suggested in this repository's context. Changes should be proposed in the source repository instead.
Applied to files:
emhttp/plugins/dynamix.plugin.manager/scripts/plugin
📚 Learning: 2025-09-05T23:34:04.847Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2355
File: emhttp/plugins/dynamix.plugin.manager/include/PluginHelpers.php:21-28
Timestamp: 2025-09-05T23:34:04.847Z
Learning: In the Unraid webGUI plugin system, the static $methods array in PluginHelpers.php contains only methods that should bypass caching and call the plugin script directly. Methods like 'checkall' are handled at the PHP wrapper level via recursive calls to the plugin() function, not as direct subcommands in the plugin script itself.
Applied to files:
emhttp/plugins/dynamix.plugin.manager/scripts/plugin
📚 Learning: 2025-08-13T03:23:31.900Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2333
File: emhttp/plugins/dynamix.docker.manager/DockerContainers.page:6-6
Timestamp: 2025-08-13T03:23:31.900Z
Learning: In unraid/webgui codebase, Squidly271 prefers to maintain existing naming conventions even when there are technical inconsistencies (like Nchan="docker_load" vs actual channel "dockerload") if they've been stable for a long time, to avoid causing confusion by changing long-standing patterns.
Applied to files:
emhttp/plugins/dynamix.plugin.manager/scripts/plugin
📚 Learning: 2025-08-10T20:48:52.086Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2326
File: emhttp/plugins/dynamix.vm.manager/nchan/vm_usage:93-93
Timestamp: 2025-08-10T20:48:52.086Z
Learning: In the unraid/webgui codebase, Squidly271 prefers the publish() function to internally handle parameter type overloading (e.g., accepting boolean true in place of buffer length 1) to avoid repetitive parameter specifications and maintain backwards compatibility. The publish() function in emhttp/plugins/dynamix/include/publish.php is designed to sort out these parameters internally rather than requiring explicit values at every call site.
Applied to files:
emhttp/plugins/dynamix.plugin.manager/scripts/plugin
📚 Learning: 2025-09-05T16:33:12.970Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2353
File: emhttp/plugins/dynamix/DashStats.page:2257-2257
Timestamp: 2025-09-05T16:33:12.970Z
Learning: In the Unraid webGUI Nchan system, publisher scripts (like "update_1") publish to normalized endpoint names (like "update1"). The Nchan header lists the script names to start, while JavaScript subscribes to the published endpoints. For example: Nchan="update_1" starts the script which calls publish_noDupe('update1', data), and JavaScript subscribes to '/sub/update1'. This is the intended design, not a mismatch.
Applied to files:
emhttp/plugins/dynamix.plugin.manager/scripts/plugin
📚 Learning: 2025-09-05T16:33:12.970Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2353
File: emhttp/plugins/dynamix/DashStats.page:2257-2257
Timestamp: 2025-09-05T16:33:12.970Z
Learning: In the Unraid webGUI Nchan system, the Nchan header declaration lists publisher scripts to start (e.g., "update_1"), while the JavaScript subscription endpoints use normalized names (e.g., "/sub/update1"). The publisher script "update_1" publishes to "/pub/update1" endpoint, creating a separation between script names and published endpoints. This is by design and not a mismatch.
Applied to files:
emhttp/plugins/dynamix.plugin.manager/scripts/plugin
📚 Learning: 2025-10-03T02:57:29.994Z
Learnt from: ljm42
Repo: unraid/webgui PR: 2414
File: etc/rc.d/rc.nginx:374-376
Timestamp: 2025-10-03T02:57:29.994Z
Learning: Repo unraid/webgui: In etc/rc.d/rc.nginx, maintainers prefer not to add explicit mv-failure checks or EXIT trap clearing around atomic writes in build_servers(), build_locations(), and build_ini(); treat mv failures (e.g., disk full/permissions) as non-recoverable and keep the implementation simple.
Applied to files:
emhttp/plugins/dynamix.plugin.manager/scripts/plugin
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
emhttp/plugins/dynamix.plugin.manager/scripts/plugin (5)
26-37: Download usage text matches implementation semanticsThe new
plugin downloadusage, including the note that both install and download process FILE elements withinstall(or no) method, correctly reflects the behavior inplugin('install', ...)withdownload_onlygating only Run execution.
213-235: Hooks correctly suppressed in download-only modeShort-circuiting
pre_hooks()andpost_hooks()when$download_onlyis true cleanly prevents hook side effects duringplugin downloadwhile preserving behavior for all other methods.
299-299: Run commands are safely skipped in download-only modeImporting
$download_onlyintoplugin()and bailing out before executing FILERuncommands achieves the intended “download without executing install commands” behavior, including inline scripts, while leaving normal install/update/remove flows unchanged.Also applies to: 467-493
527-545: Consistent script path resolution via$scriptSwitching
checkall,updateall, andcheckosto resolve$cmdfrom$scriptcentralizes how the executable path is derived and yields clearer error messages when the script path cannot be resolved, without changing the core behavior of these commands.Also applies to: 550-573, 578-594
742-754: Confirm that download-only should still register the plugin as installedIn the registration block,
plugin downloadfor a normal plugin still:
- Copies the
.plginto/boot/config/plugins, and- Creates the
/var/log/pluginssymlink,but reports the status as
downloadedinstead ofinstalled. FornoInstallscripts, it reportsstagedinstead ofexecuted.This means download-only mode pre-stages the plugin and marks it as installed in the usual way, but defers FILE
Runexecution (thanks to$download_onlyand the Run guard inplugin()). If the intent was strictly “download artifacts without changing install state”, you’d need an additional guard here to skip the copy+symlink in download-only mode.Would you like to keep this “pre-stage but not execute” behavior, or should
plugin downloadavoid registering the plugin at all?
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
emhttp/plugins/dynamix.plugin.manager/scripts/plugin (1)
503-513: Guard$argvaccess to avoid notices; minor nchan edge case remainsRight now
$method = $argv[1];and$optional_args = array_slice($argv, 2);are evaluated before the$argc < 2check. If someone runspluginwith no subcommand, this will emit PHP notices for undefined$argv[1]. You can avoid that with a small defensive tweak:$notify = '/usr/local/emhttp/webGui/scripts/notify'; $boot = '/boot/config/plugins'; $plugins = '/var/log/plugins'; $tmp = '/tmp/plugins'; -$download_only = false; -$script = $argv[0]; -$method = $argv[1]; -$optional_args = array_slice($argv, 2); -$builtin = ['unRAIDServer','unRAIDServer-']; -$nchan = in_array('nchan', $optional_args, true); // console or nchan output +$download_only = false; +$script = $argv[0]; +$method = $argv[1] ?? null; +$optional_args = $argc > 2 ? array_slice($argv, 2) : []; +$builtin = ['unRAIDServer','unRAIDServer-']; +$nchan = in_array('nchan', $optional_args, true); // console or nchan outputNote: as previously discussed,
$optional_argsstill includes the plugin filename for commands that take a PLUGIN‑FILE, so a plugin literally namednchanwould toggle$nchan. This is extremely unlikely in practice but still a tiny edge case; if you ever want to tighten it, you could strip the plugin filename from$optional_argsonly for methods that require one (install/download/check/update/remove).Also applies to: 518-522
🧹 Nitpick comments (1)
emhttp/plugins/dynamix.plugin.manager/scripts/plugin (1)
602-607: Clarify intended semantics ofplugin downloadvs registrationThe current
downloadflow still:
- Runs
plugin('install', $plugin_file, $error)with$download_only = true(so FILEs are processed but Run commands skipped), and- Registers the plugin in
/boot/config/pluginsand/var/log/pluginsjust like a normal install, only changing status/log messages to “downloaded”/“staged”.This means a “downloaded” plugin is already treated as installed by the system (e.g., symlink exists, included in
checkall/updateall), even though none of its Run steps or hooks have ever executed, and a laterplugin installusing the same PLUGIN‑FILE path will hit the “not re‑installing same plugin” short‑circuit.If your goal is strictly “install all FILEs but skip Run and hooks” then this behavior is consistent. If instead the intent is more of an “offline fetch/stage without marking the plugin as installed yet”, you may want to consider:
- Skipping the
$targetcopy and$symlinkcreation when$download_onlyis true, or- Marking the resulting state in a way that allows a follow‑up real install (e.g., allowing re‑install from the same PLUGIN‑FILE when the previous run was download‑only).
Can you confirm which behavior you want for
plugin download? If you’d like, I can sketch a diff for one of the above approaches.Also applies to: 608-615, 619-623, 739-746
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
emhttp/plugins/dynamix.plugin.manager/scripts/plugin(11 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: elibosley
Repo: unraid/webgui PR: 2464
File: emhttp/plugins/dynamix.plugin.manager/scripts/plugin:0-0
Timestamp: 2025-11-24T15:12:41.007Z
Learning: In emhttp/plugins/dynamix.plugin.manager/scripts/plugin, the $forced variable logic intentionally treats ANY option argument (beyond the explicitly handled ones like 'download-only', 'download', 'force', 'forced') as triggering forced install mode. This means that options like 'nchan' will cause $forced to be true. This is by design, even though it's unusual behavior.
📚 Learning: 2025-11-24T15:12:41.007Z
Learnt from: elibosley
Repo: unraid/webgui PR: 2464
File: emhttp/plugins/dynamix.plugin.manager/scripts/plugin:0-0
Timestamp: 2025-11-24T15:12:41.007Z
Learning: In emhttp/plugins/dynamix.plugin.manager/scripts/plugin, the $forced variable logic intentionally treats ANY option argument (beyond the explicitly handled ones like 'download-only', 'download', 'force', 'forced') as triggering forced install mode. This means that options like 'nchan' will cause $forced to be true. This is by design, even though it's unusual behavior.
Applied to files:
emhttp/plugins/dynamix.plugin.manager/scripts/plugin
📚 Learning: 2025-03-27T22:04:00.594Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2099
File: emhttp/plugins/dynamix.my.servers/include/activation-code-extractor.php:58-74
Timestamp: 2025-03-27T22:04:00.594Z
Learning: The file `emhttp/plugins/dynamix.my.servers/include/activation-code-extractor.php` is synced from a different repository, and modifications should not be suggested in this repository's context. Changes should be proposed in the source repository instead.
Applied to files:
emhttp/plugins/dynamix.plugin.manager/scripts/plugin
📚 Learning: 2025-09-05T23:34:04.847Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2355
File: emhttp/plugins/dynamix.plugin.manager/include/PluginHelpers.php:21-28
Timestamp: 2025-09-05T23:34:04.847Z
Learning: In the Unraid webGUI plugin system, the static $methods array in PluginHelpers.php contains only methods that should bypass caching and call the plugin script directly. Methods like 'checkall' are handled at the PHP wrapper level via recursive calls to the plugin() function, not as direct subcommands in the plugin script itself.
Applied to files:
emhttp/plugins/dynamix.plugin.manager/scripts/plugin
📚 Learning: 2025-08-13T03:23:31.900Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2333
File: emhttp/plugins/dynamix.docker.manager/DockerContainers.page:6-6
Timestamp: 2025-08-13T03:23:31.900Z
Learning: In unraid/webgui codebase, Squidly271 prefers to maintain existing naming conventions even when there are technical inconsistencies (like Nchan="docker_load" vs actual channel "dockerload") if they've been stable for a long time, to avoid causing confusion by changing long-standing patterns.
Applied to files:
emhttp/plugins/dynamix.plugin.manager/scripts/plugin
📚 Learning: 2025-08-10T20:48:52.086Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2326
File: emhttp/plugins/dynamix.vm.manager/nchan/vm_usage:93-93
Timestamp: 2025-08-10T20:48:52.086Z
Learning: In the unraid/webgui codebase, Squidly271 prefers the publish() function to internally handle parameter type overloading (e.g., accepting boolean true in place of buffer length 1) to avoid repetitive parameter specifications and maintain backwards compatibility. The publish() function in emhttp/plugins/dynamix/include/publish.php is designed to sort out these parameters internally rather than requiring explicit values at every call site.
Applied to files:
emhttp/plugins/dynamix.plugin.manager/scripts/plugin
📚 Learning: 2025-09-05T16:33:12.970Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2353
File: emhttp/plugins/dynamix/DashStats.page:2257-2257
Timestamp: 2025-09-05T16:33:12.970Z
Learning: In the Unraid webGUI Nchan system, publisher scripts (like "update_1") publish to normalized endpoint names (like "update1"). The Nchan header lists the script names to start, while JavaScript subscribes to the published endpoints. For example: Nchan="update_1" starts the script which calls publish_noDupe('update1', data), and JavaScript subscribes to '/sub/update1'. This is the intended design, not a mismatch.
Applied to files:
emhttp/plugins/dynamix.plugin.manager/scripts/plugin
📚 Learning: 2025-09-05T16:33:12.970Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2353
File: emhttp/plugins/dynamix/DashStats.page:2257-2257
Timestamp: 2025-09-05T16:33:12.970Z
Learning: In the Unraid webGUI Nchan system, the Nchan header declaration lists publisher scripts to start (e.g., "update_1"), while the JavaScript subscription endpoints use normalized names (e.g., "/sub/update1"). The publisher script "update_1" publishes to "/pub/update1" endpoint, creating a separation between script names and published endpoints. This is by design and not a mismatch.
Applied to files:
emhttp/plugins/dynamix.plugin.manager/scripts/plugin
📚 Learning: 2025-10-03T02:57:29.994Z
Learnt from: ljm42
Repo: unraid/webgui PR: 2414
File: etc/rc.d/rc.nginx:374-376
Timestamp: 2025-10-03T02:57:29.994Z
Learning: Repo unraid/webgui: In etc/rc.d/rc.nginx, maintainers prefer not to add explicit mv-failure checks or EXIT trap clearing around atomic writes in build_servers(), build_locations(), and build_ini(); treat mv failures (e.g., disk full/permissions) as non-recoverable and keep the implementation simple.
Applied to files:
emhttp/plugins/dynamix.plugin.manager/scripts/plugin
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
emhttp/plugins/dynamix.plugin.manager/scripts/plugin (4)
26-36: Download subcommand usage text looks consistent with behaviorThe new
plugin downloadusage block clearly documents that FILE elements for theinstallmethod are still processed while Run commands are skipped, which matches the implementation below. No issues here.
213-230: Skipping hooks in download-only mode matches intentConditioning
pre_hooks()/post_hooks()on$download_onlyis a clean way to ensure download-only runs don't trigger external hook scripts, while other methods (check,update,remove, etc.) still execute hooks because$download_onlyremainsfalse. Looks good.
298-300: Run skipping insideplugin()is correct and well-loggedUsing the global
$download_onlyinplugin()to short‑circuit Run blocks, with a clear log message and a reasonable$targetfallback (Name,LOCAL, or'inline script'), achieves “download without execution” semantics without affecting normal installs.Also applies to: 467-473
528-529: Usingrealpath($script)improves consistencySwitching the helper commands (
checkall,updateall,checkos) to userealpath($script)instead of inlining$argv[0]gives a single source of truth for the script path and aligns with the new initialization block. Behavior stays the same for normal invocations.Also applies to: 551-552, 579-580
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
emhttp/plugins/dynamix.plugin.manager/scripts/plugin (1)
510-512: Consider extracting options consistently to avoid edge case.
$optional_argson line 510 includes the plugin filename (argv[2]), which means the nchan check on line 512 could incorrectly enable nchan mode if a plugin were literally named 'nchan'. Line 618 correctly extracts only options usingarray_slice($argv, 3). Consider using the same approach here for consistency.-$optional_args = array_slice($argv, 2); +$optional_args = array_slice($argv, 3);Note: This issue was previously identified but remains unaddressed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
emhttp/plugins/dynamix.plugin.manager/scripts/plugin(12 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: elibosley
Repo: unraid/webgui PR: 2464
File: emhttp/plugins/dynamix.plugin.manager/scripts/plugin:0-0
Timestamp: 2025-11-24T15:12:41.007Z
Learning: In emhttp/plugins/dynamix.plugin.manager/scripts/plugin, the $forced variable logic intentionally treats ANY option argument (beyond the explicitly handled ones like 'download-only', 'download', 'force', 'forced') as triggering forced install mode. This means that options like 'nchan' will cause $forced to be true. This is by design, even though it's unusual behavior.
📚 Learning: 2025-11-24T15:12:41.007Z
Learnt from: elibosley
Repo: unraid/webgui PR: 2464
File: emhttp/plugins/dynamix.plugin.manager/scripts/plugin:0-0
Timestamp: 2025-11-24T15:12:41.007Z
Learning: In emhttp/plugins/dynamix.plugin.manager/scripts/plugin, the $forced variable logic intentionally treats ANY option argument (beyond the explicitly handled ones like 'download-only', 'download', 'force', 'forced') as triggering forced install mode. This means that options like 'nchan' will cause $forced to be true. This is by design, even though it's unusual behavior.
Applied to files:
emhttp/plugins/dynamix.plugin.manager/scripts/plugin
📚 Learning: 2025-03-27T22:04:00.594Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2099
File: emhttp/plugins/dynamix.my.servers/include/activation-code-extractor.php:58-74
Timestamp: 2025-03-27T22:04:00.594Z
Learning: The file `emhttp/plugins/dynamix.my.servers/include/activation-code-extractor.php` is synced from a different repository, and modifications should not be suggested in this repository's context. Changes should be proposed in the source repository instead.
Applied to files:
emhttp/plugins/dynamix.plugin.manager/scripts/plugin
📚 Learning: 2025-09-05T23:34:04.847Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2355
File: emhttp/plugins/dynamix.plugin.manager/include/PluginHelpers.php:21-28
Timestamp: 2025-09-05T23:34:04.847Z
Learning: In the Unraid webGUI plugin system, the static $methods array in PluginHelpers.php contains only methods that should bypass caching and call the plugin script directly. Methods like 'checkall' are handled at the PHP wrapper level via recursive calls to the plugin() function, not as direct subcommands in the plugin script itself.
Applied to files:
emhttp/plugins/dynamix.plugin.manager/scripts/plugin
📚 Learning: 2025-08-13T03:23:31.900Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2333
File: emhttp/plugins/dynamix.docker.manager/DockerContainers.page:6-6
Timestamp: 2025-08-13T03:23:31.900Z
Learning: In unraid/webgui codebase, Squidly271 prefers to maintain existing naming conventions even when there are technical inconsistencies (like Nchan="docker_load" vs actual channel "dockerload") if they've been stable for a long time, to avoid causing confusion by changing long-standing patterns.
Applied to files:
emhttp/plugins/dynamix.plugin.manager/scripts/plugin
📚 Learning: 2025-08-10T20:48:52.086Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2326
File: emhttp/plugins/dynamix.vm.manager/nchan/vm_usage:93-93
Timestamp: 2025-08-10T20:48:52.086Z
Learning: In the unraid/webgui codebase, Squidly271 prefers the publish() function to internally handle parameter type overloading (e.g., accepting boolean true in place of buffer length 1) to avoid repetitive parameter specifications and maintain backwards compatibility. The publish() function in emhttp/plugins/dynamix/include/publish.php is designed to sort out these parameters internally rather than requiring explicit values at every call site.
Applied to files:
emhttp/plugins/dynamix.plugin.manager/scripts/plugin
📚 Learning: 2025-09-05T16:33:12.970Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2353
File: emhttp/plugins/dynamix/DashStats.page:2257-2257
Timestamp: 2025-09-05T16:33:12.970Z
Learning: In the Unraid webGUI Nchan system, publisher scripts (like "update_1") publish to normalized endpoint names (like "update1"). The Nchan header lists the script names to start, while JavaScript subscribes to the published endpoints. For example: Nchan="update_1" starts the script which calls publish_noDupe('update1', data), and JavaScript subscribes to '/sub/update1'. This is the intended design, not a mismatch.
Applied to files:
emhttp/plugins/dynamix.plugin.manager/scripts/plugin
📚 Learning: 2025-09-05T16:33:12.970Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2353
File: emhttp/plugins/dynamix/DashStats.page:2257-2257
Timestamp: 2025-09-05T16:33:12.970Z
Learning: In the Unraid webGUI Nchan system, the Nchan header declaration lists publisher scripts to start (e.g., "update_1"), while the JavaScript subscription endpoints use normalized names (e.g., "/sub/update1"). The publisher script "update_1" publishes to "/pub/update1" endpoint, creating a separation between script names and published endpoints. This is by design and not a mismatch.
Applied to files:
emhttp/plugins/dynamix.plugin.manager/scripts/plugin
📚 Learning: 2025-10-03T02:57:29.994Z
Learnt from: ljm42
Repo: unraid/webgui PR: 2414
File: etc/rc.d/rc.nginx:374-376
Timestamp: 2025-10-03T02:57:29.994Z
Learning: Repo unraid/webgui: In etc/rc.d/rc.nginx, maintainers prefer not to add explicit mv-failure checks or EXIT trap clearing around atomic writes in build_servers(), build_locations(), and build_ini(); treat mv failures (e.g., disk full/permissions) as non-recoverable and keep the implementation simple.
Applied to files:
emhttp/plugins/dynamix.plugin.manager/scripts/plugin
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (8)
emhttp/plugins/dynamix.plugin.manager/scripts/plugin (8)
26-36: LGTM! Clear documentation for the new download command.The usage documentation accurately describes the download-only functionality and clarifies that both install and download commands process FILE elements but differ in Run command execution.
214-215: LGTM! Appropriate hook skipping in download-only mode.Skipping pre/post hooks in download-only mode is correct since hooks are meant to run during actual install/update/remove operations, not during file staging.
Also applies to: 228-229
299-299: LGTM! Correctly adds global variable.Adding
$download_onlyto the global variables list is necessary for the download-only logic inside theplugin()function.
469-473: LGTM! Core download-only functionality implemented correctly.The Run command skipping logic is well-implemented with clear logging. It correctly handles all cases (Name, LOCAL, inline script) and continues processing remaining FILE elements.
514-518: LGTM! Clean normalization approach.The approach of normalizing the
downloadcommand to reuse theinstallflow while setting the$download_onlyflag is elegant and avoids code duplication.
534-534: LGTM! Improved code clarity with $script variable.Using the
$scriptvariable instead of$argv[0]improves readability and maintainability across the command resolution logic.Also applies to: 557-557, 585-585
618-628: LGTM! Clear messaging for download-only mode.The logic correctly distinguishes between download and install actions in user-facing messages. The
$forcedlogic (line 619) follows the intentional design where any option triggers forced mode.Based on learnings, the
$forcedvariable behavior is by design.
744-750: LGTM! Appropriate success messaging for both modes.The success messages correctly reflect the operation performed:
- "downloaded" vs "installed" for regular plugins
- "staged" vs "executed" for noInstall plugins
This provides clear feedback about what actually occurred in download-only mode.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
emhttp/plugins/dynamix.plugin.manager/scripts/plugin (1)
26-36: Good documentation for the new download command.The usage documentation clearly describes the new
downloadmethod and its behavior. The note about FILE element processing is helpful context.Consider clarifying that FILE elements are not just "downloaded" but are actually created/placed at their target locations—only the Run commands are skipped. This would help users understand that
downloadmode stages files in their final locations.Example clarification:
-Usage: plugin download PLUGIN-FILE [forced] - download plugin files without executing any Run commands defined for the install method +Usage: plugin download PLUGIN-FILE [forced] + stage plugin files at their target locations without executing any Run commands defined for the install method
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
emhttp/plugins/dynamix.plugin.manager/scripts/plugin(12 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: elibosley
Repo: unraid/webgui PR: 2464
File: emhttp/plugins/dynamix.plugin.manager/scripts/plugin:0-0
Timestamp: 2025-11-24T15:12:41.007Z
Learning: In emhttp/plugins/dynamix.plugin.manager/scripts/plugin, the $forced variable logic intentionally treats ANY option argument (beyond the explicitly handled ones like 'download-only', 'download', 'force', 'forced') as triggering forced install mode. This means that options like 'nchan' will cause $forced to be true. This is by design, even though it's unusual behavior.
📚 Learning: 2025-11-24T15:12:41.007Z
Learnt from: elibosley
Repo: unraid/webgui PR: 2464
File: emhttp/plugins/dynamix.plugin.manager/scripts/plugin:0-0
Timestamp: 2025-11-24T15:12:41.007Z
Learning: In emhttp/plugins/dynamix.plugin.manager/scripts/plugin, the $forced variable logic intentionally treats ANY option argument (beyond the explicitly handled ones like 'download-only', 'download', 'force', 'forced') as triggering forced install mode. This means that options like 'nchan' will cause $forced to be true. This is by design, even though it's unusual behavior.
Applied to files:
emhttp/plugins/dynamix.plugin.manager/scripts/plugin
📚 Learning: 2025-03-27T22:04:00.594Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2099
File: emhttp/plugins/dynamix.my.servers/include/activation-code-extractor.php:58-74
Timestamp: 2025-03-27T22:04:00.594Z
Learning: The file `emhttp/plugins/dynamix.my.servers/include/activation-code-extractor.php` is synced from a different repository, and modifications should not be suggested in this repository's context. Changes should be proposed in the source repository instead.
Applied to files:
emhttp/plugins/dynamix.plugin.manager/scripts/plugin
📚 Learning: 2025-09-05T23:34:04.847Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2355
File: emhttp/plugins/dynamix.plugin.manager/include/PluginHelpers.php:21-28
Timestamp: 2025-09-05T23:34:04.847Z
Learning: In the Unraid webGUI plugin system, the static $methods array in PluginHelpers.php contains only methods that should bypass caching and call the plugin script directly. Methods like 'checkall' are handled at the PHP wrapper level via recursive calls to the plugin() function, not as direct subcommands in the plugin script itself.
Applied to files:
emhttp/plugins/dynamix.plugin.manager/scripts/plugin
📚 Learning: 2025-08-13T03:23:31.900Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2333
File: emhttp/plugins/dynamix.docker.manager/DockerContainers.page:6-6
Timestamp: 2025-08-13T03:23:31.900Z
Learning: In unraid/webgui codebase, Squidly271 prefers to maintain existing naming conventions even when there are technical inconsistencies (like Nchan="docker_load" vs actual channel "dockerload") if they've been stable for a long time, to avoid causing confusion by changing long-standing patterns.
Applied to files:
emhttp/plugins/dynamix.plugin.manager/scripts/plugin
📚 Learning: 2025-08-10T20:48:52.086Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2326
File: emhttp/plugins/dynamix.vm.manager/nchan/vm_usage:93-93
Timestamp: 2025-08-10T20:48:52.086Z
Learning: In the unraid/webgui codebase, Squidly271 prefers the publish() function to internally handle parameter type overloading (e.g., accepting boolean true in place of buffer length 1) to avoid repetitive parameter specifications and maintain backwards compatibility. The publish() function in emhttp/plugins/dynamix/include/publish.php is designed to sort out these parameters internally rather than requiring explicit values at every call site.
Applied to files:
emhttp/plugins/dynamix.plugin.manager/scripts/plugin
📚 Learning: 2025-09-05T16:33:12.970Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2353
File: emhttp/plugins/dynamix/DashStats.page:2257-2257
Timestamp: 2025-09-05T16:33:12.970Z
Learning: In the Unraid webGUI Nchan system, publisher scripts (like "update_1") publish to normalized endpoint names (like "update1"). The Nchan header lists the script names to start, while JavaScript subscribes to the published endpoints. For example: Nchan="update_1" starts the script which calls publish_noDupe('update1', data), and JavaScript subscribes to '/sub/update1'. This is the intended design, not a mismatch.
Applied to files:
emhttp/plugins/dynamix.plugin.manager/scripts/plugin
📚 Learning: 2025-09-05T16:33:12.970Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2353
File: emhttp/plugins/dynamix/DashStats.page:2257-2257
Timestamp: 2025-09-05T16:33:12.970Z
Learning: In the Unraid webGUI Nchan system, the Nchan header declaration lists publisher scripts to start (e.g., "update_1"), while the JavaScript subscription endpoints use normalized names (e.g., "/sub/update1"). The publisher script "update_1" publishes to "/pub/update1" endpoint, creating a separation between script names and published endpoints. This is by design and not a mismatch.
Applied to files:
emhttp/plugins/dynamix.plugin.manager/scripts/plugin
📚 Learning: 2025-10-03T02:57:29.994Z
Learnt from: ljm42
Repo: unraid/webgui PR: 2414
File: etc/rc.d/rc.nginx:374-376
Timestamp: 2025-10-03T02:57:29.994Z
Learning: Repo unraid/webgui: In etc/rc.d/rc.nginx, maintainers prefer not to add explicit mv-failure checks or EXIT trap clearing around atomic writes in build_servers(), build_locations(), and build_ini(); treat mv failures (e.g., disk full/permissions) as non-recoverable and keep the implementation simple.
Applied to files:
emhttp/plugins/dynamix.plugin.manager/scripts/plugin
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (7)
emhttp/plugins/dynamix.plugin.manager/scripts/plugin (7)
214-215: Verify that hooks should be skipped in download-only mode.The logic skips both pre-hooks and post-hooks when
$download_onlyis true. This makes sense since download-only is a staging operation rather than an active install/remove/update event. However, please confirm this is the intended behavior—some monitoring or logging hooks might benefit from download notifications.Also applies to: 228-229
469-473: LGTM! Clean implementation of Run command skipping.The logic correctly identifies download-only mode and skips Run command execution while providing clear logging of what was skipped. The target identification handles all three file sources (Name, LOCAL, INLINE) appropriately.
503-523: Clean normalization of download to install flow.The variable initialization and argument parsing is well-structured:
- Extracts
$scriptfor consistent use in checkall/updateall/checkos- Correctly parses
$extra_argsand handles thenchanflag removal- Normalizes the
downloadmethod to reuse theinstallflow with a$download_onlyflagThis approach avoids code duplication while cleanly implementing the new download-only behavior.
539-540: Good refactoring for consistency.Using the
$scriptvariable instead of directly accessing$argv[0]in checkall, updateall, and checkos methods improves code consistency and maintainability.Also applies to: 562-563, 590-591
627-631: Clear messaging for download-only mode.The conditional messaging appropriately informs users when download-only mode is active and uses consistent action verbs throughout the operation.
747-754: Good semantic status messaging.The status messages appropriately distinguish between download and install operations:
- "downloaded" vs "installed" for plugins
- "staged" vs "executed" for scripts
Note that the plugin symlink is still created in download-only mode (line 746), meaning the plugin is registered as if installed. Please confirm this is intentional—it suggests that download-only mode stages files and registers the plugin but skips Run command execution.
299-299: LGTM!Adding
$download_onlyto the global variables inplugin()is necessary and follows the existing pattern.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
emhttp/plugins/dynamix.plugin.manager/scripts/plugin (1)
759-770: Critical: Prevent symlink creation in download-only mode.Creating the symlink in download-only mode marks the plugin as "installed" even though Run commands never executed. This breaks the post-upgrade installation flow:
- User runs
plugin downloadbefore OS upgrade- Symlink is created at line 762
- After OS upgrade, boot-time install detects the symlink (line 689)
- Install logic sees same file already installed (line 690) and exits (line 691)
- Run commands never execute—plugin remains incompletely installed
Apply this diff to skip symlink creation in download-only mode:
$target = "$boot/$plugin"; if (!plugin('noInstall', $plugin_file, $error)) { if ($target != $plugin_file) copy($plugin_file, $target); - symlink($target, $symlink); + if (!$download_only) { + symlink($target, $symlink); + } $status = $download_only ? 'downloaded' : 'installed';
♻️ Duplicate comments (1)
emhttp/plugins/dynamix.plugin.manager/scripts/plugin (1)
882-888: Critical: Prevent symlink creation in download-only mode.Same issue as the install method—the symlink at line 885 should not be created in download-only mode, otherwise the plugin won't properly install after OS upgrade.
Apply this diff:
unlink($symlink); $target = "$boot/$plugin"; copy($plugin_file, $target); -symlink($target, $symlink); +if (!$download_only) { + symlink($target, $symlink); +} $status = $download_only ? 'downloaded' : 'updated';
🧹 Nitpick comments (2)
emhttp/plugins/dynamix.plugin.manager/scripts/plugin (2)
665-665: Remove redundant global declaration.The
$check_versionvariable is already in global scope (initialized at line 516), making this declaration unnecessary.Apply this diff:
} - global $check_version; $min = plugin('min', $plugin_file, $error);
67-77: Document TARGET-VERSION parameter for update command.The code at line 525 supports the
TARGET-VERSIONparameter for both download and update methods, but the usage text only documents it for download (lines 79-87). Consider adding similar documentation here for consistency.Example addition after line 68:
Usage: plugin update PLUGIN update the plugin + TARGET-VERSION is optional and specifies the Unraid version to use for version compatibility + checks (Min/Max attributes). If omitted, the current Unraid version is used. + We look for the new plugin in /tmp/plugins/ directory. If found then we first execute the "install"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
emhttp/plugins/dynamix.plugin.manager/scripts/plugin(17 hunks)
🧰 Additional context used
🧠 Learnings (9)
📚 Learning: 2025-11-24T15:12:41.028Z
Learnt from: elibosley
Repo: unraid/webgui PR: 2464
File: emhttp/plugins/dynamix.plugin.manager/scripts/plugin:0-0
Timestamp: 2025-11-24T15:12:41.028Z
Learning: In emhttp/plugins/dynamix.plugin.manager/scripts/plugin, the $forced variable logic intentionally treats ANY option argument (beyond the explicitly handled ones like 'download-only', 'download', 'force', 'forced') as triggering forced install mode. This means that options like 'nchan' will cause $forced to be true. This is by design, even though it's unusual behavior.
Applied to files:
emhttp/plugins/dynamix.plugin.manager/scripts/plugin
📚 Learning: 2025-03-27T22:04:00.594Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2099
File: emhttp/plugins/dynamix.my.servers/include/activation-code-extractor.php:58-74
Timestamp: 2025-03-27T22:04:00.594Z
Learning: The file `emhttp/plugins/dynamix.my.servers/include/activation-code-extractor.php` is synced from a different repository, and modifications should not be suggested in this repository's context. Changes should be proposed in the source repository instead.
Applied to files:
emhttp/plugins/dynamix.plugin.manager/scripts/plugin
📚 Learning: 2025-09-05T23:34:04.847Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2355
File: emhttp/plugins/dynamix.plugin.manager/include/PluginHelpers.php:21-28
Timestamp: 2025-09-05T23:34:04.847Z
Learning: In the Unraid webGUI plugin system, the static $methods array in PluginHelpers.php contains only methods that should bypass caching and call the plugin script directly. Methods like 'checkall' are handled at the PHP wrapper level via recursive calls to the plugin() function, not as direct subcommands in the plugin script itself.
Applied to files:
emhttp/plugins/dynamix.plugin.manager/scripts/plugin
📚 Learning: 2025-08-13T03:23:31.900Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2333
File: emhttp/plugins/dynamix.docker.manager/DockerContainers.page:6-6
Timestamp: 2025-08-13T03:23:31.900Z
Learning: In unraid/webgui codebase, Squidly271 prefers to maintain existing naming conventions even when there are technical inconsistencies (like Nchan="docker_load" vs actual channel "dockerload") if they've been stable for a long time, to avoid causing confusion by changing long-standing patterns.
Applied to files:
emhttp/plugins/dynamix.plugin.manager/scripts/plugin
📚 Learning: 2025-08-10T20:48:52.086Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2326
File: emhttp/plugins/dynamix.vm.manager/nchan/vm_usage:93-93
Timestamp: 2025-08-10T20:48:52.086Z
Learning: In the unraid/webgui codebase, Squidly271 prefers the publish() function to internally handle parameter type overloading (e.g., accepting boolean true in place of buffer length 1) to avoid repetitive parameter specifications and maintain backwards compatibility. The publish() function in emhttp/plugins/dynamix/include/publish.php is designed to sort out these parameters internally rather than requiring explicit values at every call site.
Applied to files:
emhttp/plugins/dynamix.plugin.manager/scripts/plugin
📚 Learning: 2025-09-05T16:33:12.970Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2353
File: emhttp/plugins/dynamix/DashStats.page:2257-2257
Timestamp: 2025-09-05T16:33:12.970Z
Learning: In the Unraid webGUI Nchan system, publisher scripts (like "update_1") publish to normalized endpoint names (like "update1"). The Nchan header lists the script names to start, while JavaScript subscribes to the published endpoints. For example: Nchan="update_1" starts the script which calls publish_noDupe('update1', data), and JavaScript subscribes to '/sub/update1'. This is the intended design, not a mismatch.
Applied to files:
emhttp/plugins/dynamix.plugin.manager/scripts/plugin
📚 Learning: 2025-09-05T16:33:12.970Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2353
File: emhttp/plugins/dynamix/DashStats.page:2257-2257
Timestamp: 2025-09-05T16:33:12.970Z
Learning: In the Unraid webGUI Nchan system, the Nchan header declaration lists publisher scripts to start (e.g., "update_1"), while the JavaScript subscription endpoints use normalized names (e.g., "/sub/update1"). The publisher script "update_1" publishes to "/pub/update1" endpoint, creating a separation between script names and published endpoints. This is by design and not a mismatch.
Applied to files:
emhttp/plugins/dynamix.plugin.manager/scripts/plugin
📚 Learning: 2025-10-03T02:57:29.994Z
Learnt from: ljm42
Repo: unraid/webgui PR: 2414
File: etc/rc.d/rc.nginx:374-376
Timestamp: 2025-10-03T02:57:29.994Z
Learning: Repo unraid/webgui: In etc/rc.d/rc.nginx, maintainers prefer not to add explicit mv-failure checks or EXIT trap clearing around atomic writes in build_servers(), build_locations(), and build_ini(); treat mv failures (e.g., disk full/permissions) as non-recoverable and keep the implementation simple.
Applied to files:
emhttp/plugins/dynamix.plugin.manager/scripts/plugin
📚 Learning: 2025-09-03T08:33:04.940Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2352
File: emhttp/plugins/dynamix.plugin.manager/include/PluginHelpers.php:0-0
Timestamp: 2025-09-03T08:33:04.940Z
Learning: In the unraid/webgui plugin system, basename comparison for cache invalidation in dropPluginCache() is considered sufficient for their plugin naming conventions and use cases.
Applied to files:
emhttp/plugins/dynamix.plugin.manager/scripts/plugin
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
…option, allowing users to download plugin files without executing installation commands. Update usage instructions and adjust processing logic accordingly.
… a dedicated 'download' command. Adjust processing logic and improve logging for clarity during plugin installation and downloading.
…y' variable with 'download_mode'. Update related conditions for clarity and consistency in plugin installation and downloading behavior.
…load_only' variable with 'download_mode'. Update related conditions for clarity and consistency in plugin installation and downloading behavior." This reverts commit 3ac24e4.
…load_only' variable. Ensure pre and post hook execution respects the download-only mode, improving clarity and functionality during plugin installation and downloading.
…gs. Update logic to determine if a plugin is forced based solely on the presence of options, enhancing clarity in plugin processing.
…. Streamline the handling of options to enhance clarity and maintainability in plugin processing.
…djusted method flow to reuse installation steps for download requests, enhancing clarity and maintainability in plugin processing.
…gument parsing. Streamline the logic for determining forced plugin installations and improve clarity in command processing.
elibosley
force-pushed
the
feat/download-only-plugin-installs
branch
from
efa0685 to
1dd843d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
emhttp/plugins/dynamix.plugin.manager/scripts/plugin (2)
363-375: Consider logging skipped files even in download-only mode.When
$download_onlyis true, files that don't meet Min/Max version requirements are silently skipped (no log output). This could make troubleshooting difficult if a user expects certain files to be downloaded but they're filtered out by version checks.Consider always logging these skips, perhaps with a prefix like "download:" to maintain consistency with other download-only log messages.
Apply this diff to always log version-based skips:
$min = $file->attributes()->Min; - if ($min && version_compare($check_version,$min,'<')) { - if (!$download_only) { - write("plugin: skipping: ".basename($name)." - Unraid version too low, requires at least version $min\n"); - } + if ($min && version_compare($check_version,$min,'<')) { + write("plugin: skipping: ".basename($name)." - Unraid version too low, requires at least version $min\n"); continue; } $max = $file->attributes()->Max; - if ($max && version_compare($check_version,$max,'>')) { - if (!$download_only) { - write("plugin: skipping: ".basename($name)." - Unraid version too high, requires at most version $max\n"); - } + if ($max && version_compare($check_version,$max,'>')) { + write("plugin: skipping: ".basename($name)." - Unraid version too high, requires at most version $max\n"); continue; }
665-665: Remove redundant global declaration.The
global $check_version;declaration on line 665 is unnecessary because$check_versionis already defined at the script level (line 516) in the same scope. In PHP, variables defined at the script level are already global and don't need to be re-declared with theglobalkeyword within other script-level code blocks.The
globalkeyword is only needed when accessing script-level variables from within functions.Apply this diff to remove the redundant declaration:
} - // bergware - check Unraid version dependency (if present) - global $check_version; + // bergware - check Unraid version dependency (if present) $min = plugin('min', $plugin_file, $error);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
emhttp/plugins/dynamix.plugin.manager/scripts/plugin(17 hunks)
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: elibosley
Repo: unraid/webgui PR: 2464
File: emhttp/plugins/dynamix.plugin.manager/scripts/plugin:0-0
Timestamp: 2025-11-24T15:12:41.028Z
Learning: In emhttp/plugins/dynamix.plugin.manager/scripts/plugin, the $forced variable logic intentionally treats ANY option argument (beyond the explicitly handled ones like 'download-only', 'download', 'force', 'forced') as triggering forced install mode. This means that options like 'nchan' will cause $forced to be true. This is by design, even though it's unusual behavior.
📚 Learning: 2025-11-24T15:12:41.028Z
Learnt from: elibosley
Repo: unraid/webgui PR: 2464
File: emhttp/plugins/dynamix.plugin.manager/scripts/plugin:0-0
Timestamp: 2025-11-24T15:12:41.028Z
Learning: In emhttp/plugins/dynamix.plugin.manager/scripts/plugin, the $forced variable logic intentionally treats ANY option argument (beyond the explicitly handled ones like 'download-only', 'download', 'force', 'forced') as triggering forced install mode. This means that options like 'nchan' will cause $forced to be true. This is by design, even though it's unusual behavior.
Applied to files:
emhttp/plugins/dynamix.plugin.manager/scripts/plugin
📚 Learning: 2025-03-27T22:04:00.594Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2099
File: emhttp/plugins/dynamix.my.servers/include/activation-code-extractor.php:58-74
Timestamp: 2025-03-27T22:04:00.594Z
Learning: The file `emhttp/plugins/dynamix.my.servers/include/activation-code-extractor.php` is synced from a different repository, and modifications should not be suggested in this repository's context. Changes should be proposed in the source repository instead.
Applied to files:
emhttp/plugins/dynamix.plugin.manager/scripts/plugin
📚 Learning: 2025-09-05T23:34:04.847Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2355
File: emhttp/plugins/dynamix.plugin.manager/include/PluginHelpers.php:21-28
Timestamp: 2025-09-05T23:34:04.847Z
Learning: In the Unraid webGUI plugin system, the static $methods array in PluginHelpers.php contains only methods that should bypass caching and call the plugin script directly. Methods like 'checkall' are handled at the PHP wrapper level via recursive calls to the plugin() function, not as direct subcommands in the plugin script itself.
Applied to files:
emhttp/plugins/dynamix.plugin.manager/scripts/plugin
📚 Learning: 2025-08-13T03:23:31.900Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2333
File: emhttp/plugins/dynamix.docker.manager/DockerContainers.page:6-6
Timestamp: 2025-08-13T03:23:31.900Z
Learning: In unraid/webgui codebase, Squidly271 prefers to maintain existing naming conventions even when there are technical inconsistencies (like Nchan="docker_load" vs actual channel "dockerload") if they've been stable for a long time, to avoid causing confusion by changing long-standing patterns.
Applied to files:
emhttp/plugins/dynamix.plugin.manager/scripts/plugin
📚 Learning: 2025-08-10T20:48:52.086Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2326
File: emhttp/plugins/dynamix.vm.manager/nchan/vm_usage:93-93
Timestamp: 2025-08-10T20:48:52.086Z
Learning: In the unraid/webgui codebase, Squidly271 prefers the publish() function to internally handle parameter type overloading (e.g., accepting boolean true in place of buffer length 1) to avoid repetitive parameter specifications and maintain backwards compatibility. The publish() function in emhttp/plugins/dynamix/include/publish.php is designed to sort out these parameters internally rather than requiring explicit values at every call site.
Applied to files:
emhttp/plugins/dynamix.plugin.manager/scripts/plugin
📚 Learning: 2025-09-05T16:33:12.970Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2353
File: emhttp/plugins/dynamix/DashStats.page:2257-2257
Timestamp: 2025-09-05T16:33:12.970Z
Learning: In the Unraid webGUI Nchan system, publisher scripts (like "update_1") publish to normalized endpoint names (like "update1"). The Nchan header lists the script names to start, while JavaScript subscribes to the published endpoints. For example: Nchan="update_1" starts the script which calls publish_noDupe('update1', data), and JavaScript subscribes to '/sub/update1'. This is the intended design, not a mismatch.
Applied to files:
emhttp/plugins/dynamix.plugin.manager/scripts/plugin
📚 Learning: 2025-09-05T16:33:12.970Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2353
File: emhttp/plugins/dynamix/DashStats.page:2257-2257
Timestamp: 2025-09-05T16:33:12.970Z
Learning: In the Unraid webGUI Nchan system, the Nchan header declaration lists publisher scripts to start (e.g., "update_1"), while the JavaScript subscription endpoints use normalized names (e.g., "/sub/update1"). The publisher script "update_1" publishes to "/pub/update1" endpoint, creating a separation between script names and published endpoints. This is by design and not a mismatch.
Applied to files:
emhttp/plugins/dynamix.plugin.manager/scripts/plugin
📚 Learning: 2025-10-03T02:57:29.994Z
Learnt from: ljm42
Repo: unraid/webgui PR: 2414
File: etc/rc.d/rc.nginx:374-376
Timestamp: 2025-10-03T02:57:29.994Z
Learning: Repo unraid/webgui: In etc/rc.d/rc.nginx, maintainers prefer not to add explicit mv-failure checks or EXIT trap clearing around atomic writes in build_servers(), build_locations(), and build_ini(); treat mv failures (e.g., disk full/permissions) as non-recoverable and keep the implementation simple.
Applied to files:
emhttp/plugins/dynamix.plugin.manager/scripts/plugin
📚 Learning: 2025-09-03T08:33:04.940Z
Learnt from: Squidly271
Repo: unraid/webgui PR: 2352
File: emhttp/plugins/dynamix.plugin.manager/include/PluginHelpers.php:0-0
Timestamp: 2025-09-03T08:33:04.940Z
Learning: In the unraid/webgui plugin system, basename comparison for cache invalidation in dropPluginCache() is considered sufficient for their plugin naming conventions and use cases.
Applied to files:
emhttp/plugins/dynamix.plugin.manager/scripts/plugin
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (10)
emhttp/plugins/dynamix.plugin.manager/scripts/plugin (10)
79-87: LGTM! Clear documentation for the download command.The usage documentation accurately describes the download-only mode, including the optional TARGET-VERSION parameter and its purpose for preparing plugin files before an OS upgrade.
210-212: LGTM! Hooks appropriately skipped in download-only mode.Returning early when
$download_onlyis true ensures that pre-install and post-install hooks don't run during file-download-only operations, which is the correct behavior.
296-296: LGTM! Global variables properly declared.The
$download_onlyand$check_versionglobals are correctly declared for use throughout theplugin()function.
470-474: LGTM! Run commands properly skipped in download-only mode.The logic correctly bypasses script execution when
$download_onlyis true, while still logging the skip for debugging. The fallback to 'inline script' for unnamed files is a nice touch.
554-621: LGTM! Consistent script path handling.The refactoring to use the
$scriptvariable throughoutcheckall,updateall, andcheckosmethods improves consistency and maintainability. Error messages also properly reference{$script}.
642-646: LGTM! Clear messaging for download-only mode.The conditional logging and dynamic
$actionvariable provide clear feedback to users about the mode of operation, making the download-only behavior explicit and understandable.
763-770: LGTM! Status messaging accurately reflects download-only semantics.The conditional status messages ('downloaded'/'installed' for plugins, 'staged'/'executed' for scripts) accurately convey what operations were performed, providing clear user feedback.
832-836: LGTM! Consistent download-only integration in update method.The update method properly integrates download-only mode with the same messaging patterns as the install method, maintaining consistency across the codebase.
886-888: LGTM! Status messaging mirrors install method.The final status messages in the update method use the same pattern as the install method, maintaining consistency in how download-only operations are reported.
523-531: The usage is properly documented and displayed to users.The review comment expresses concern that argument order handling "might be acceptable if documented." Verification confirms it IS documented:
- Line 79 explicitly specifies the usage:
Usage: plugin download PLUGIN-FILE [TARGET-VERSION] [forced]- Lines 545-548 show the usage text is displayed to users when the script is called without arguments
- The code correctly implements this documented order (checking only the first
$extra_argselement)The behavior is intentional and matches the documented specification. No action is needed.
Likely an incorrect or invalid review comment.
Summary by CodeRabbit
New Features
Bug Fixes / Improvements
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.