Dev-new-fixes-22 #108
Dev-new-fixes-22 #108
Conversation
Signed-off-by: DaRK AnGeL <[email protected]>
Signed-off-by: DaRK AnGeL <[email protected]>
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughReads Unraid version from /var/local/emhttp/var.ini, adds version-gated UI elements and a "Move now" action, introduces a separate mover tuning cron (mover.tuning.cron) and management paths, and selects the appropriate mover binary (/usr/local/sbin/mover.old vs /usr/local/sbin/mover) based on Unraid version for manual and cron runs. Changes
Sequence DiagramsequenceDiagram
autonumber
participant UI as Browser (Mover.tuning.page)
participant API as Server (updateCron.php / mover.php)
participant FS as Filesystem (/etc/cron.d, /var/run)
participant Exec as Executor (moveNow.php / mover binary)
UI->>API: POST settings {cronEnabled, cron, tune_cronSchedule, cmdStartTuneMover?}
API->>API: `@parse_ini_file`("/var/local/emhttp/var.ini") → determine version
alt version ≥ 7.2.1
API->>FS: write/update `mover.tuning.cron` (make_tune_cron)
API->>FS: ensure `mover.cron` exists/updated (make_unraid_cron) if shared schedule used
else version < 7.2.1
API->>FS: write/update `mover.cron` (make_cron)
end
Note over UI,Exec: "Move now" action or cron triggers execution
UI->>Exec: POST cmdStartTuneMover (or cron triggers)
Exec->>Exec: parse var.ini and version_compare
alt version < 7.2.1
Exec->>Exec: run `/usr/local/sbin/mover.old` (start)
else version ≥ 7.2.1
Exec->>Exec: run `/usr/local/sbin/mover` (start)
end
Exec->>API: emit logs/status
API->>UI: return status/result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
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 |
Summary of ChangesHello @masterwishx, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/Mover.tuning.page(3 hunks)source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/default.cfg(1 hunks)source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/mover.php(3 hunks)source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php (1)
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/mover.php (1)
logger(11-18)
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/mover.php (1)
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php (1)
logger(17-24)
🪛 PHPMD (2.15.0)
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php
29-29: Avoid unused local variables such as '$var'. (undefined)
(UnusedLocalVariable)
⏰ 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: Codacy Static Code Analysis
🔇 Additional comments (13)
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/default.cfg (1)
51-51: LGTM!The addition of the
moverTuneCronconfiguration parameter is appropriate and follows the existing pattern.source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php (6)
6-6: LGTM!Parsing the var.ini file provides access to system version information needed for version-based conditional logic throughout the file.
14-15: LGTM!The configuration reading logic correctly uses the null coalescing operator with a fallback chain to
shareMoverSchedule.
34-35: LGTM!The cron file generation and persistence logic is correct, using the appropriate path and format consistent with the existing
mover.cronpattern.
86-96: Conditional logic structure is sound.The handling of Mover Tuning cron updates follows the same pattern as the existing force cron logic and correctly creates/updates or removes the cron file based on the POST value. Note that the effectiveness depends on fixing the bugs in
make_tune_cron()flagged earlier.
29-32: Fix variable name inconsistency in condition and assignment.The code checks
$var['shareMoverSchedule']but assigns to$vars['shareMoverSchedule']— these are different variables. Verify which variable should be used. Additionally, confirm that modifying$vars(whether fromparse_ini_file()or another source) actually persists to the config file, or if an additional save operation is required.
40-45: Version-based mover selection is correct.The logic appropriately selects
mover.oldfor Unraid versions < 7.2.1 andmoverfor newer versions. Unraid 7.2.1 introduced changes to the mover binary (removal of the -e option, behavior tied to WebGUI), and the system creates a backup asmover.oldduring updates. This versioning strategy is correctly implemented and consistently applied across the plugin.source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/Mover.tuning.page (2)
11-11: LGTM!Parsing the var.ini file is necessary for the version-based conditional rendering added later in the file.
53-56: LGTM!The JavaScript correctly captures the new
tune_cronScheduleinput and includes it in the POST request toupdateCron.php.source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/mover.php (4)
93-97: LGTM!The version-based mover binary selection is consistent with the changes in
updateCron.phpand correctly handles the Unraid version differences.
124-130: LGTM!The version-based branching for the default move button correctly mirrors the logic on lines 93-97 and ensures consistent behavior across the plugin.
135-135: LGTM!The updated message correctly clarifies that it's the "Mover Tuning schedule" being disabled, which improves clarity for users.
144-144: LGTM!The updated message to "Starting Mover Tuning ..." appropriately reflects the plugin's identity and helps users distinguish plugin actions from system mover actions.
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/Mover.tuning.page
Show resolved
Hide resolved
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php
Outdated
Show resolved
Hide resolved
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new "Mover Tuning schedule" feature, allowing a custom cron schedule for the Mover Tuning plugin, specifically for Unraid versions 7.2.1 and newer. The changes affect the UI, backend PHP scripts, and the main mover script.
I've identified a few issues that should be addressed:
- A critical typo in
updateCron.phpthat would prevent a key part of the new feature from working correctly. - A high-severity issue in
Mover.tuning.pagedue to a conflictingnameattribute in a form input, which could lead to incorrect settings being saved. - Some instances of code duplication in
mover.phpandupdateCron.phpthat can be refactored for better maintainability.
I've provided specific suggestions to resolve these issues. Addressing them will improve the robustness and quality of the code.
| function make_tune_cron() | ||
| { | ||
| global $vars; | ||
| if (!empty($var['shareMoverSchedule'])) { |
There was a problem hiding this comment.
There is a typo in the variable name. It should be $vars instead of $var. Due to this typo, the condition !empty($var['shareMoverSchedule']) will likely not evaluate as intended, and the Unraid mover schedule may not be disabled correctly.
if (!empty($vars['shareMoverSchedule'])) {| <div markdown="1" id="moverTuningSettings"> | ||
| <?php if (version_compare($vars['version'], '7.2.1', '>=')): ?> | ||
| _(Mover Tuning schedule)_: | ||
| : <input type='text' id='tune_cronSchedule' name='cron' size='1' class='tune_mycron' placeholder='0 */4 * * *' value='<?=htmlspecialchars($cfg['moverTuneCron'])?>'> |
There was a problem hiding this comment.
The new input field for "Mover Tuning schedule" has name='cron', which is the same as the name attribute of the existing input field for "Cron Schedule to force move all of files" on line 336. Using the same name for multiple form inputs can cause unpredictable behavior, potentially resulting in only one of the values being saved upon form submission. To prevent this conflict, the name attribute should be unique. Given that the value is loaded from $cfg['moverTuneCron'], the name should likely be moverTuneCron.
: <input type='text' id='tune_cronSchedule' name='moverTuneCron' size='1' class='tune_mycron' placeholder='0 */4 * * *' value='<?=htmlspecialchars($cfg['moverTuneCron'])?>'>
| if (version_compare($vars['version'], '7.2.1', '<')) { | ||
| logger("ionice $ioLevel nice -n $niceLevel /usr/local/sbin/mover.old $options"); | ||
| passthru("ionice $ioLevel nice -n $niceLevel /usr/local/sbin/mover.old $options"); | ||
| } else { | ||
| logger("ionice $ioLevel nice -n $niceLevel /usr/local/sbin/mover $options"); | ||
| passthru("ionice $ioLevel nice -n $niceLevel /usr/local/sbin/mover $options"); | ||
| } |
There was a problem hiding this comment.
This block of code contains duplication. The correct mover binary path is already determined and stored in the $mover_str variable on lines 90-98. Instead of repeating the version_compare logic, you can reuse $mover_str. This will make the code more maintainable and reduce redundancy.
logger("ionice $ioLevel nice -n $niceLevel $mover_str $options");
passthru("ionice $ioLevel nice -n $niceLevel $mover_str $options");| if (version_compare($vars['version'], '7.2.1', '<')) { | ||
| $cronFile = "# Generated schedule for forced move\n" . trim($_POST['cron']) . " /usr/local/sbin/mover.old start |& logger -t move\n\n"; | ||
| } else { | ||
| $cronFile = "# Generated schedule for forced move\n" . trim($_POST['cron']) . " /usr/local/sbin/mover start |& logger -t move\n\n"; | ||
| } |
There was a problem hiding this comment.
The code for generating the cron file content is duplicated within the if/else block. You can determine the mover binary path first based on the version, and then construct the $cronFile string once. This will make the code cleaner and easier to maintain.
$mover_binary = version_compare($vars['version'], '7.2.1', '<') ? '/usr/local/sbin/mover.old' : '/usr/local/sbin/mover';
$cronFile = "# Generated schedule for forced move\n" . trim($_POST['cron']) . " $mover_binary start |& logger -t move\n\n";Signed-off-by: DaRK AnGeL <[email protected]>
Signed-off-by: DaRK AnGeL <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/mover.php (1)
6-6: Consider adding null check for parse_ini_file failure.Same issue as in
moveNow.php- if the INI file is missing/unreadable,$varswill befalse. While the code may tolerate this in some paths, explicit handling prevents PHP warnings and makes the failure mode clear.
♻️ Duplicate comments (2)
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/mover.php (1)
131-137: Reuse$mover_strinstead of duplicating version logic.As previously noted, the mover binary path is already determined at lines 100-104. Additionally, the indentation inside the if/else blocks is inconsistent.
- if (version_compare($vars['version'], '7.2.1', '<')) { - logger("ionice $ioLevel nice -n $niceLevel /usr/local/sbin/mover.old $options"); - passthru("ionice $ioLevel nice -n $niceLevel /usr/local/sbin/mover.old $options"); - } else { - logger("ionice $ioLevel nice -n $niceLevel /usr/local/sbin/mover $options"); - passthru("ionice $ioLevel nice -n $niceLevel /usr/local/sbin/mover $options"); - } + logger("ionice $ioLevel nice -n $niceLevel $mover_str $options"); + passthru("ionice $ioLevel nice -n $niceLevel $mover_str $options");source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php (1)
41-45: Reduce code duplication in version-based cron generation.As previously noted, the only difference between branches is the mover binary path. Consider refactoring:
function make_cron() { global $vars; - if (version_compare($vars['version'], '7.2.1', '<')) { - $cronFile = "# Generated schedule for forced move\n" . trim($_POST['cron']) . " /usr/local/sbin/mover.old start |& logger -t move\n\n"; - } else { - $cronFile = "# Generated schedule for forced move\n" . trim($_POST['cron']) . " /usr/local/sbin/mover start |& logger -t move\n\n"; - } + $version = $vars['version'] ?? '0.0.0'; + $mover = version_compare($version, '7.2.1', '<') ? '/usr/local/sbin/mover.old' : '/usr/local/sbin/mover'; + $cron = isset($_POST['cron']) ? trim($_POST['cron']) : ''; + $cronFile = "# Generated schedule for forced move\n{$cron} {$mover} start |& logger -t move\n\n"; file_put_contents("/boot/config/plugins/ca.mover.tuning/mover.cron", $cronFile); }This also adds null-coalescing for version safety and validates
$_POST['cron'].
🧹 Nitpick comments (1)
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php (1)
6-6: Handle potential parse failure for var.ini.If
/var/local/emhttp/var.inidoesn't exist or is malformed,$varswill befalse, causing issues when accessing$vars['version']or$vars['shareMoverSchedule']later. Consider adding a fallback:-$vars = @parse_ini_file("/var/local/emhttp/var.ini"); +$vars = @parse_ini_file("/var/local/emhttp/var.ini") ?: [];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/Mover.tuning.page(5 hunks)source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/moveNow.php(1 hunks)source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/mover.php(4 hunks)source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/Mover.tuning.page
🧰 Additional context used
🪛 GitHub Check: Codacy Static Code Analysis
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php
[warning] 6-6: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L6
The use of function parse_ini_file() is discouraged
[failure] 34-34: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L34
Detected usage of a possibly undefined superglobal array index: $_POST['tune_cron']. Use isset() or empty() to check the index exists before using it
[warning] 34-34: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L34
Direct use of $_POST Superglobal detected.
[failure] 34-34: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L34
Processing form data without nonce verification.
[warning] 41-41: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L41
Implicit true comparisons prohibited; use === TRUE instead
[failure] 44-44: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L44
Detected usage of a possibly undefined superglobal array index: $_POST['cron']. Use isset() or empty() to check the index exists before using it
[warning] 44-44: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L44
Direct use of $_POST Superglobal detected.
[failure] 44-44: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L44
Processing form data without nonce verification.
[failure] 87-87: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L87
Detected usage of a possibly undefined superglobal array index: $_POST['tune_cron']. Use isset() or empty() to check the index exists before using it
[warning] 87-87: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L87
Direct use of $_POST Superglobal detected.
[failure] 87-87: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L87
Processing form data without nonce verification.
[failure] 89-89: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L89
Detected usage of a possibly undefined superglobal array index: $_POST['tune_cron']. Use isset() or empty() to check the index exists before using it
[warning] 89-89: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L89
Direct use of $_POST Superglobal detected.
[warning] 89-89: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L89
Operator != prohibited; use !== instead
[failure] 89-89: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L89
Processing form data without nonce verification.
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/moveNow.php
[warning] 3-3: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/moveNow.php#L3
The use of function parse_ini_file() is discouraged
[warning] 4-4: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/moveNow.php#L4
Implicit true comparisons prohibited; use === TRUE instead
[failure] 7-7: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/moveNow.php#L7
System program execution function exec() detected with dynamic parameter
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/mover.php
[warning] 60-60: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/mover.php#L60
Implicit true comparisons prohibited; use === TRUE instead
[warning] 61-61: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/mover.php#L61
Direct use of $_POST Superglobal detected.
[warning] 61-61: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/mover.php#L61
Implicit true comparisons prohibited; use === TRUE instead
[failure] 61-61: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/mover.php#L61
Processing form data without nonce verification.
[failure] 136-136: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/mover.php#L136
Executing non-constant commands.
[failure] 136-136: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/mover.php#L136
Executing non-constant commands.
[failure] 136-136: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/mover.php#L136
System program execution function passthru() detected with dynamic parameter
⏰ 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: Codacy Static Code Analysis
🔇 Additional comments (3)
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/mover.php (2)
100-104: LGTM!Version-aware mover binary selection is cleanly implemented here, setting
$mover_strfor reuse in subsequent code paths.
141-143: LGTM!Log message updates appropriately reflect the "Mover Tuning" plugin branding.
Also applies to: 151-151
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php (1)
14-15: LGTM!Good use of null coalescing to provide sensible defaults, falling back to Unraid's shareMoverSchedule when the plugin's moverTuneCron isn't configured.
| $vars = @parse_ini_file("/var/local/emhttp/var.ini"); | ||
| if (version_compare($vars['version'], '7.2.1', '<')) { | ||
| exec("/usr/local/sbin/mover.old start >> /var/log/syslog &", $output, $retval); | ||
| } else { | ||
| exec("/usr/local/sbin/mover start >> /var/log/syslog &", $output, $retval); | ||
| } |
There was a problem hiding this comment.
Add null check for parse_ini_file failure.
If /var/local/emhttp/var.ini is missing or unreadable, $vars will be false, and accessing $vars['version'] will trigger a PHP warning. While version_compare(null, '7.2.1', '<') happens to return true (falling back to mover.old), this should be explicit.
$vars = @parse_ini_file("/var/local/emhttp/var.ini");
+if (!$vars || !isset($vars['version'])) {
+ exec("/usr/local/sbin/mover.old start >> /var/log/syslog &", $output, $retval);
+ exit;
+}
if (version_compare($vars['version'], '7.2.1', '<')) {
exec("/usr/local/sbin/mover.old start >> /var/log/syslog &", $output, $retval);
} else {
exec("/usr/local/sbin/mover start >> /var/log/syslog &", $output, $retval);
}📝 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.
| $vars = @parse_ini_file("/var/local/emhttp/var.ini"); | |
| if (version_compare($vars['version'], '7.2.1', '<')) { | |
| exec("/usr/local/sbin/mover.old start >> /var/log/syslog &", $output, $retval); | |
| } else { | |
| exec("/usr/local/sbin/mover start >> /var/log/syslog &", $output, $retval); | |
| } | |
| $vars = @parse_ini_file("/var/local/emhttp/var.ini"); | |
| if (!$vars || !isset($vars['version'])) { | |
| exec("/usr/local/sbin/mover.old start >> /var/log/syslog &", $output, $retval); | |
| exit; | |
| } | |
| if (version_compare($vars['version'], '7.2.1', '<')) { | |
| exec("/usr/local/sbin/mover.old start >> /var/log/syslog &", $output, $retval); | |
| } else { | |
| exec("/usr/local/sbin/mover start >> /var/log/syslog &", $output, $retval); | |
| } |
🧰 Tools
🪛 GitHub Check: Codacy Static Code Analysis
[warning] 3-3: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/moveNow.php#L3
The use of function parse_ini_file() is discouraged
[warning] 4-4: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/moveNow.php#L4
Implicit true comparisons prohibited; use === TRUE instead
[failure] 7-7: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/moveNow.php#L7
System program execution function exec() detected with dynamic parameter
🤖 Prompt for AI Agents
In source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/moveNow.php
around lines 3 to 8, add an explicit null/false check after parse_ini_file to
handle failure: test that $vars is an array (or not false) and that a 'version'
key exists, then set a safe default (e.g. null or '0.0.0') or log an error; use
that vetted value in version_compare so you don't index into a non-array and so
behavior is explicit (fallback to using mover.old when version is missing or
below 7.2.1). Ensure any error logging or default choice is minimal and keeps
the same exec calls unchanged.
| // For Unraid v7.2.1+, use $_POST for pressed move now button in plugin page | ||
| else if (version_compare($vars['version'], '7.2.1', '>=')) { | ||
| if (isset($_POST['cmdStartTuneMover'])) { | ||
| $args[0] = 'start'; | ||
| $option1 = $args[0]; | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider adding nonce verification for POST handling.
The $_POST['cmdStartTuneMover'] parameter is used without CSRF/nonce verification. While Unraid provides authentication, a logged-in user could be tricked into triggering the mover via a malicious page. If Unraid provides a nonce mechanism (e.g., via the Wrappers include), consider using it.
If nonce verification isn't available in this plugin context, at minimum validate that the request came from the expected source or document why CSRF protection isn't applicable here.
🧰 Tools
🪛 GitHub Check: Codacy Static Code Analysis
[warning] 60-60: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/mover.php#L60
Implicit true comparisons prohibited; use === TRUE instead
[warning] 61-61: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/mover.php#L61
Direct use of $_POST Superglobal detected.
[warning] 61-61: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/mover.php#L61
Implicit true comparisons prohibited; use === TRUE instead
[failure] 61-61: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/mover.php#L61
Processing form data without nonce verification.
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php
Outdated
Show resolved
Hide resolved
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php
Outdated
Show resolved
Hide resolved
Signed-off-by: DaRK AnGeL <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php (2)
26-37:make_tune_crondoes not actually disable the Unraid mover schedule.
$varsis just the in‑memory result ofparse_ini_file. Changing$vars['shareMoverSchedule']here:if (!empty($vars['shareMoverSchedule'])) { // Disable Unraid mover $vars['shareMoverSchedule'] = ""; }does not persist anything back to Unraid’s configuration, so the base mover schedule will not be disabled despite the comment. If disabling the Unraid mover is intended, you need to write the setting back via the appropriate Unraid mechanism (e.g., updating the relevant config file or API); otherwise, this block is misleading and can be removed.
34-36: Guard and normalizetune_cronPOST handling to avoid notices and redundant trimming.You already normalize
$_POST['tune_cron']inmake_tune_cron, but the comparison/update block still accesses$_POST['tune_cron']directly and repeatedly. If the field is missing (e.g., when this script is hit from another context), you’ll get undefined‑index notices, and the change‑detection logic may behave unexpectedly.You can centralize and safely reuse the normalized value like this:
function make_tune_cron() { - $tuneCron = isset($_POST['tune_cron']) ? trim($_POST['tune_cron']) : ''; + $tuneCron = isset($_POST['tune_cron']) ? trim($_POST['tune_cron']) : ''; $cronTuneFile = "# Generated schedule for Mover Tuning move\n" . $tuneCron . " /usr/local/emhttp/plugins/ca.mover.tuning/age_mover start |& logger -t move\n\n"; file_put_contents("/boot/config/plugins/ca.mover.tuning/mover.tune.cron", $cronTuneFile); } -// Handle Mover Tuning custom cron schedule -if ($cfg_moverTuneCron != $_POST['tune_cron']) { - - if (trim($_POST['tune_cron']) != "") { +// Handle Mover Tuning custom cron schedule +$postTuneCron = isset($_POST['tune_cron']) ? trim($_POST['tune_cron']) : ''; +if ($cfg_moverTuneCron !== $postTuneCron) { + if ($postTuneCron !== '') { make_tune_cron(); logger("Mover Tuning cron schedule updated successfully."); } else {This avoids undefined‑index notices, keeps comparisons consistent (trimmed on both sides), and tightens the equality check with
!==.Also applies to: 86-96
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/mover.php (1)
59-65: Starting mover based on raw POST flag lacks CSRF protection.For Unraid ≥ 7.2.1 you now treat a
cmdStartTuneMoverPOST flag as astartoption:else if (version_compare($vars['version'], '7.2.1', '>=')) { if (isset($_POST['cmdStartTuneMover'])) { $args[0] = 'start'; $option1 = $args[0]; } }Since this directly triggers a mover run, consider adding whatever nonce/CSRF mechanism Unraid or
Wrappers.phpprovides (or otherwise constraining the origin) so a logged‑in user can’t be tricked into starting the mover via a malicious page.
🧹 Nitpick comments (1)
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php (1)
6-6: Config and version parsing look reasonable; optional robustness improvement.Using
@parse_ini_file(... ) ?: []plusmoverTuneCronfalling back to$vars['shareMoverSchedule']gives sensible defaults and keeps comparisons trimmed. If you ever need to debug misconfigurations, consider logging whenparse_ini_filefails instead of fully suppressing errors, but the current behavior is functionally fine.Also applies to: 14-15
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/Mover.tuning.page(5 hunks)source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/mover.php(4 hunks)source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/Mover.tuning.page
🧰 Additional context used
🧬 Code graph analysis (2)
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/mover.php (1)
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php (1)
logger(17-24)
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php (1)
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/mover.php (1)
logger(11-18)
🪛 GitHub Check: Codacy Static Code Analysis
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/mover.php
[warning] 60-60: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/mover.php#L60
Implicit true comparisons prohibited; use === TRUE instead
[warning] 61-61: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/mover.php#L61
Direct use of $_POST Superglobal detected.
[warning] 61-61: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/mover.php#L61
Implicit true comparisons prohibited; use === TRUE instead
[failure] 61-61: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/mover.php#L61
Processing form data without nonce verification.
[failure] 132-132: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/mover.php#L132
System program execution function passthru() detected with dynamic parameter
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php
[warning] 6-6: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L6
Implicit true comparisons prohibited; use === TRUE instead
[warning] 29-29: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L29
Operator ! prohibited; use === FALSE instead
[failure] 34-34: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L34
$_POST['tune_cron'] not unslashed before sanitization. Use wp_unslash() or similar
[warning] 34-34: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L34
Direct use of $_POST Superglobal detected.
[warning] 34-34: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L34
Implicit true comparisons prohibited; use === TRUE instead
[warning] 36-36: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L36
The use of function file_put_contents() is discouraged
[warning] 43-43: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L43
Implicit true comparisons prohibited; use === TRUE instead
[failure] 44-44: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L44
$_POST['cron'] not unslashed before sanitization. Use wp_unslash() or similar
[warning] 44-44: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L44
Direct use of $_POST Superglobal detected.
[warning] 44-44: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L44
Implicit true comparisons prohibited; use === TRUE instead
[failure] 87-87: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L87
Detected usage of a possibly undefined superglobal array index: $_POST['tune_cron']. Use isset() or empty() to check the index exists before using it
[warning] 87-87: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L87
Direct use of $_POST Superglobal detected.
[failure] 87-87: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L87
Processing form data without nonce verification.
[failure] 89-89: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L89
Detected usage of a possibly undefined superglobal array index: $_POST['tune_cron']. Use isset() or empty() to check the index exists before using it
[warning] 89-89: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L89
Direct use of $_POST Superglobal detected.
[warning] 89-89: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L89
Operator != prohibited; use !== instead
[failure] 89-89: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L89
Processing form data without nonce verification.
⏰ 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: Codacy Static Code Analysis
🔇 Additional comments (3)
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php (1)
41-45: Version‑aware mover selection and cron file generation look good.Deriving
$versionfrom$vars, picking the correct mover binary viaversion_compare, and constructing$cronFileonce using a trimmed$_POST['cron']makes this path clearer and avoids the earlier duplication.source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/mover.php (2)
100-104: Version‑aware mover binary selection and reuse of$mover_strlook correct.Selecting
/usr/local/sbin/mover.oldfor versions< 7.2.1and/usr/local/sbin/moverotherwise, then reusing$mover_strin the button path avoids duplicatedversion_comparelogic and keeps the ionice/nice calls consistent:if ($cfg['movenow'] == "yes") { $mover_str = "/usr/local/emhttp/plugins/ca.mover.tuning/age_mover"; } else { if (version_compare($vars['version'], '7.2.1', '<')) { $mover_str = "/usr/local/sbin/mover.old"; } else { $mover_str = "/usr/local/sbin/mover"; } } ... logger("ionice $ioLevel nice -n $niceLevel $mover_str $options"); passthru("ionice $ioLevel nice -n $niceLevel $mover_str $options");This matches the PR’s goal of version‑aware mover selection.
Also applies to: 131-132
137-137: Updated log messages align with tuning semantics.Changing the messages to:
logger("Mover Tuning schedule disabled"); ... logger("Starting Mover Tuning ...");clarifies in syslog that these actions belong to the Mover Tuning plugin rather than the stock mover, which should help debugging.
Also applies to: 146-146
Signed-off-by: DaRK AnGeL <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php (1)
100-102: Missingisset()checks for$_POST['tune_cron'].Despite past review comments indicating this was addressed, the code still accesses
$_POST['tune_cron']directly on lines 100 and 102 without checking if the key exists. This will cause undefined index notices when the form field isn't submitted. Additionally, use strict comparison (!==) instead of loose comparison (!=).Based on learnings from past review comments that this should have been addressed.
Apply this diff:
+$postTuneCron = isset($_POST['tune_cron']) ? $_POST['tune_cron'] : ''; + // Handle Mover Tuning custom cron schedule -if ($cfg_moverTuneCron != $_POST['tune_cron']) { +if ($cfg_moverTuneCron !== $postTuneCron) { - if (trim($_POST['tune_cron']) != "") { + if (trim($postTuneCron) !== "") { make_tune_cron(); logger("Mover Tuning cron schedule updated successfully."); } else {
🧹 Nitpick comments (2)
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php (2)
47-49: Validate cron expression format.The code doesn't validate that
$tuneCroncontains a valid cron expression before writing it to the cron file. An invalid cron expression could cause the cron job to fail silently.Consider adding basic validation:
$tuneCron = isset($_POST['tune_cron']) ? trim($_POST['tune_cron']) : ''; + // Basic validation: cron should have 5 time fields + if (!empty($tuneCron) && !preg_match('/^[\d\*\/\-,]+ [\d\*\/\-,]+ [\d\*\/\-,]+ [\d\*\/\-,]+ [\d\*\/\-,]+$/', $tuneCron)) { + logger("Invalid cron format provided for Mover Tuning: " . $tuneCron); + return false; + } $cronTuneFile = "# Generated schedule for Mover Tuning move\n" . $tuneCron . " /usr/local/emhttp/plugins/ca.mover.tuning/age_mover start |& logger -t move\n\n"; file_put_contents("/boot/config/plugins/ca.mover.tuning/mover.tuning.cron", $cronTuneFile);
54-58: Good version-aware mover selection.The logic correctly selects the appropriate mover binary based on the Unraid version, maintaining backward compatibility with pre-7.2.1 versions.
Optionally, add the same cron format validation suggested for
make_tune_cron()to ensure consistency:$cron = isset($_POST['cron']) ? trim($_POST['cron']) : ''; + if (!empty($cron) && !preg_match('/^[\d\*\/\-,]+ [\d\*\/\-,]+ [\d\*\/\-,]+ [\d\*\/\-,]+ [\d\*\/\-,]+$/', $cron)) { + logger("Invalid cron format provided: " . $cron); + return false; + } $cronFile = "# Generated schedule for forced move\n{$cron} {$mover} start |& logger -t move\n\n";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php (1)
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/mover.php (1)
logger(11-18)
🪛 GitHub Check: Codacy Static Code Analysis
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php
[warning] 6-6: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L6
Implicit true comparisons prohibited; use === TRUE instead
[failure] 47-47: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L47
$_POST['tune_cron'] not unslashed before sanitization. Use wp_unslash() or similar
[warning] 47-47: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L47
Direct use of $_POST Superglobal detected.
[warning] 47-47: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L47
Implicit true comparisons prohibited; use === TRUE instead
[warning] 56-56: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L56
Implicit true comparisons prohibited; use === TRUE instead
[failure] 57-57: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L57
$_POST['cron'] not unslashed before sanitization. Use wp_unslash() or similar
[warning] 57-57: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L57
Direct use of $_POST Superglobal detected.
[warning] 57-57: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L57
Implicit true comparisons prohibited; use === TRUE instead
[failure] 57-57: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L57
Processing form data without nonce verification.
[warning] 100-100: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L100
Direct use of $_POST Superglobal detected.
[warning] 100-100: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L100
Operator != prohibited; use !== instead
[failure] 100-100: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L100
Processing form data without nonce verification.
[warning] 102-102: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L102
Operator != prohibited; use !== instead
[failure] 102-102: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L102
Processing form data without nonce verification.
⏰ 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: Codacy Static Code Analysis
🔇 Additional comments (2)
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php (2)
6-7: LGTM!The INI file parsing with error suppression and fallback to an empty array is appropriate for handling potential file read failures.
15-16: LGTM!The fallback chain using null coalescing provides a sensible default hierarchy for the mover tuning cron configuration.
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php
Outdated
Show resolved
Hide resolved
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php
Show resolved
Hide resolved
Signed-off-by: DaRK AnGeL <[email protected]>
Signed-off-by: DaRK AnGeL <[email protected]>
Signed-off-by: DaRK AnGeL <[email protected]>
Signed-off-by: DaRK AnGeL <[email protected]>
Signed-off-by: DaRK AnGeL <[email protected]>
Signed-off-by: DaRK AnGeL <[email protected]>
Signed-off-by: DaRK AnGeL <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php (1)
117-127: Validate$_POST['tune_cron']before access.Lines 118 and 120 access
$_POST['tune_cron']without checking if the key exists, which can cause undefined index notices when the form field isn't submitted.// Handle Mover Tuning custom cron schedule -if ($cfg_moverTuneCron != $_POST['tune_cron']) { - - if (trim($_POST['tune_cron']) != "") { +$postTuneCron = isset($_POST['tune_cron']) ? $_POST['tune_cron'] : ''; +if ($cfg_moverTuneCron !== $postTuneCron) { + if (trim($postTuneCron) !== "") { make_tune_cron(); logger("Mover Tuning cron schedule updated successfully."); } else { @unlink("/boot/config/plugins/ca.mover.tuning/mover.tuning.cron"); logger("Mover Tuning cron schedule removed."); } }This also uses strict comparison (
!==) per static analysis hints.
🧹 Nitpick comments (3)
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php (3)
39-46: Remove unusedglobal $varsdeclaration.The
global $varsdeclaration on line 41 is never used withinmake_tune_cron(). Static analysis confirms this as dead code.// Mover Tuning cron for unraid v7.2.1+ function make_tune_cron() { - global $vars; - $tuneCron = isset($_POST['tune_cron']) ? trim($_POST['tune_cron']) : ''; $cronTuneFile = "# Generated schedule for Mover Tuning move:\n" . $tuneCron . " /usr/local/emhttp/plugins/ca.mover.tuning/age_mover start |& logger -t move\n\n"; file_put_contents("/boot/config/plugins/ca.mover.tuning/mover.tuning.cron", $cronTuneFile); }
27-36: Simplify redundant isset check.Line 32's
isset()is redundant because line 31 already verifies with!empty()(which implies the key exists).function make_unraid_cron() { global $vars; if (!empty($vars['shareMoverSchedule'])) { - $moverCron = isset($vars['shareMoverSchedule']) ? trim($vars['shareMoverSchedule']) : ''; + $moverCron = trim($vars['shareMoverSchedule']); $cronMoverFile = "# Generated mover schedule:\n" . $moverCron . " /usr/local/sbin/mover start |& logger -t move\n\n"; file_put_contents("/boot/config/plugins/dynamix/mover.cron", $cronMoverFile); } }
76-115: Consider movingglobal $varsto top of script scope.The
global $varsdeclaration on line 78 inside the conditional block works but is unconventional. Since$varsis already defined at script scope (line 6), this declaration is redundant and can be removed.// Check if value was changed if ($cfg_moverDisabled != $_POST["ismoverDisabled"]) { - global $vars; // If mover schedule is disabled if ($_POST['ismoverDisabled'] == "yes") {Note: The filename mismatch from past reviews has been correctly fixed - all references now consistently use
mover.tuning.cron.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/Mover.tuning.page(6 hunks)source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php (1)
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/mover.php (1)
logger(11-18)
🪛 GitHub Check: Codacy Static Code Analysis
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php
[warning] 6-6: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L6
Implicit true comparisons prohibited; use === TRUE instead
[warning] 31-31: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L31
Operator ! prohibited; use === FALSE instead
[failure] 43-43: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L43
$_POST['tune_cron'] not unslashed before sanitization. Use wp_unslash() or similar
[failure] 43-43: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L43
Processing form data without nonce verification.
[warning] 53-53: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L53
Implicit true comparisons prohibited; use === TRUE instead
[failure] 54-54: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L54
$_POST['cron'] not unslashed before sanitization. Use wp_unslash() or similar
[warning] 54-54: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L54
Direct use of $_POST Superglobal detected.
[warning] 54-54: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L54
Implicit true comparisons prohibited; use === TRUE instead
[failure] 54-54: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L54
Processing form data without nonce verification.
[warning] 118-118: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L118
Direct use of $_POST Superglobal detected.
[failure] 118-118: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L118
Processing form data without nonce verification.
[failure] 120-120: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L120
Detected usage of a possibly undefined superglobal array index: $_POST['tune_cron']. Use isset() or empty() to check the index exists before using it
[warning] 120-120: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L120
Direct use of $_POST Superglobal detected.
[warning] 120-120: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L120
Operator != prohibited; use !== instead
🪛 PHPMD (2.15.0)
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php
41-41: Avoid unused local variables such as '$vars'. (undefined)
(UnusedLocalVariable)
⏰ 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: Codacy Static Code Analysis
🔇 Additional comments (5)
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/Mover.tuning.page (4)
64-66: LGTM!The
startTuneMover()function correctly POSTs to the mover.php endpoint with the appropriate parameter.
52-58: LGTM!The
updateCron()function correctly captures the newtune_cronScheduleinput and includes it in the POST payload.
136-195: LGTM!The version-gated warning notices and the localStorage-based dismissal logic are well implemented. The breaking changes notice provides clear guidance to users about the behavioral changes in Unraid 7.2.1+.
218-226: LGTM!The version-gated Mover Tuning schedule input is correctly implemented with proper escaping (
htmlspecialchars) and a uniquename='moverTuneCron'attribute (addressing the previously flagged duplicate name issue).source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php (1)
49-57: LGTM!The
make_cron()function is well-implemented with proper null coalescing for version fallback and version-aware mover binary selection.
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/Mover.tuning.page
Show resolved
Hide resolved
Signed-off-by: DaRK AnGeL <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php (3)
26-36: Consider adding error handling for file_put_contents.
file_put_contents()can fail silently. Consider checking the return value and logging failures for better debugging.function make_unraid_cron() { global $vars; if (!empty($vars['shareMoverSchedule'])) { $moverCron = trim($vars['shareMoverSchedule']); $cronMoverFile = "# Generated mover schedule:\n" . $moverCron . " /usr/local/sbin/mover start |& logger -t move\n\n"; - file_put_contents("/boot/config/plugins/dynamix/mover.cron", $cronMoverFile); + if (file_put_contents("/boot/config/plugins/dynamix/mover.cron", $cronMoverFile) === false) { + logger("Error: Failed to write mover.cron file."); + } } }
76-76: Redundant global declaration.
$varsis already in the file's global scope (defined at line 6). Thisglobal $varsinside the function body is unnecessary.if ($cfg_moverDisabled != $_POST["ismoverDisabled"]) { - global $vars; // If mover schedule is disabled
86-94: Reconsider "Error" log level for expected states.Logging "Error: Mover Tuning cron file does not exist" when disabling the schedule is misleading—the file not existing is a valid state (nothing to remove). Consider changing to a debug-level message or removing it entirely.
if (version_compare($vars['version'], '7.2.1', '>=')) { // Check if the file exists if (file_exists("/boot/config/plugins/ca.mover.tuning/mover.tuning.cron")) { @unlink("/boot/config/plugins/ca.mover.tuning/mover.tuning.cron"); logger("Mover Tuning schedule disabled successfully."); - } else { - logger("Error: Mover Tuning cron file does not exist"); } }source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/Mover.tuning.page (1)
184-196: Consider checking element existence before accessing.If the notice element is hidden server-side (version < 7.2.1), the JavaScript still attempts to access it. While this currently works because the element exists but is hidden, a safer approach would check for the element first.
<script> -document.getElementById('close_mover_notice').addEventListener('click', function() { +var closeBtn = document.getElementById('close_mover_notice'); +if (closeBtn) { + closeBtn.addEventListener('click', function() { var notice = document.getElementById('mover_tuning_breaking_notice'); notice.style.display = 'none'; localStorage.setItem('mover_notice_closed', 'yes'); -}); + }); +} // Hide automatically if previously closed if (localStorage.getItem('mover_notice_closed') === 'yes') { var notice = document.getElementById('mover_tuning_breaking_notice'); - notice.style.display = 'none'; + if (notice) notice.style.display = 'none'; } </script>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/Mover.tuning.page(6 hunks)source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php (1)
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/mover.php (1)
logger(11-18)
🪛 GitHub Check: Codacy Static Code Analysis
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php
[warning] 6-6: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L6
Implicit true comparisons prohibited; use === TRUE instead
[warning] 31-31: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L31
Operator ! prohibited; use === FALSE instead
[warning] 34-34: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L34
The use of function file_put_contents() is discouraged
[warning] 41-41: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L41
Implicit true comparisons prohibited; use === TRUE instead
[warning] 43-43: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L43
The use of function file_put_contents() is discouraged
[warning] 51-51: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L51
Implicit true comparisons prohibited; use === TRUE instead
[failure] 52-52: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L52
$_POST['cron'] not unslashed before sanitization. Use wp_unslash() or similar
[warning] 52-52: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L52
Direct use of $_POST Superglobal detected.
[failure] 52-52: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L52
Processing form data without nonce verification.
[warning] 81-81: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L81
The use of function unlink() is discouraged
[warning] 88-88: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L88
Implicit true comparisons prohibited; use === TRUE instead
[warning] 88-88: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L88
The use of function file_exists() is discouraged
[warning] 89-89: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L89
The use of function unlink() is discouraged
[warning] 97-97: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L97
The use of function file_exists() is discouraged
⏰ 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: Codacy Static Code Analysis
🔇 Additional comments (7)
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php (3)
6-6: LGTM on the defensive INI parsing.The use of
@parse_ini_file()with?: []fallback is appropriate here—it prevents warnings when the file doesn't exist and ensures$varsis always an array.
46-55: LGTM on version-aware mover binary selection.The version comparison logic correctly selects the legacy
mover.oldfor versions below 7.2.1 and the newmoverfor 7.2.1+. The $_POST handling now properly uses isset with fallback.
115-125: LGTM on the Mover Tuning cron schedule handling.The implementation properly validates
$_POST['tune_cron']with isset, uses strict comparison (!==), and handles both creation and removal flows correctly. This addresses the previous review feedback.source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/Mover.tuning.page (4)
34-35: LGTM on the version-gated button variable.
$showMoverButtonis now properly defined using version comparison with a safe fallback. This addresses the previous review concern about the undefined variable.
137-144: LGTM on the Unraid mover schedule warning.The warning block is properly version-gated and displays relevant scheduling conflict information to users.
219-227: Form field name doesn't match what updateCron() sends.The input has
name='moverTuneCron'butupdateCron()(line 55-58) reads from#tune_cronScheduleand posts astune_cron. The form submission viaaction="/update.php"will save tomoverTuneCronin the config, but the AJAX call poststune_cron. This works because:
- Form submit →
update.phpsavesmoverTuneCronto config- AJAX
updateCron()→ sendstune_crontoupdateCron.phpThe dual-path is intentional but could be confusing. Consider adding a brief comment.
786-789: LGTM on the Move now button implementation.The button is properly version-gated using
$showMoverButton, disabled during mover runs, and shows appropriate status. Thelocation.reload()timing issue was noted earlier in thestartTuneMover()review.
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/Mover.tuning.page
Outdated
Show resolved
Hide resolved
| function startTuneMover() { | ||
| $.post("/plugins/ca.mover.tuning/mover.php", { cmdStartTuneMover: 1 }); | ||
| } |
There was a problem hiding this comment.
location.reload() may fire before POST completes.
The startTuneMover() function triggers an async POST but location.reload() is called synchronously in the onclick handler (line 787). This could reload the page before the server processes the request.
Consider using a callback or promise to reload after the POST completes:
function startTuneMover() {
-$.post("/plugins/ca.mover.tuning/mover.php", { cmdStartTuneMover: 1 });
+$.post("/plugins/ca.mover.tuning/mover.php", { cmdStartTuneMover: 1 }, function() {
+ location.reload();
+});
}Then update line 787:
-<input type="button" name="StartTuneMover" value="_(Move now)_" onclick="startTuneMover(); location.reload()" ...>
+<input type="button" name="StartTuneMover" value="_(Move now)_" onclick="startTuneMover()" ...>📝 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.
| function startTuneMover() { | |
| $.post("/plugins/ca.mover.tuning/mover.php", { cmdStartTuneMover: 1 }); | |
| } | |
| function startTuneMover() { | |
| $.post("/plugins/ca.mover.tuning/mover.php", { cmdStartTuneMover: 1 }, function() { | |
| location.reload(); | |
| }); | |
| } |
🤖 Prompt for AI Agents
In
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/Mover.tuning.page
around lines 65-67 (and update the onclick at line 787), the startTuneMover()
function issues an async POST but the page is reloaded synchronously in the
onclick handler; change startTuneMover() to wait for the POST to complete and
then reload the page (use the $.post promise callback or $.ajax success/done to
call location.reload()), and remove the immediate synchronous location.reload()
from the onclick so the reload happens only after the POST finishes.
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php
Show resolved
Hide resolved
Signed-off-by: DaRK AnGeL <[email protected]>
Signed-off-by: DaRK AnGeL <[email protected]>
Signed-off-by: DaRK AnGeL <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php (2)
26-38: Validate cron expression format before writing.The function writes
$moverCrondirectly to the cron file without validating it's a well-formed cron expression. Malformed entries could break the mover schedule.Consider adding validation:
function make_unraid_cron() { global $vars; if (!empty($vars['shareMoverSchedule'])) { $moverCron = trim($vars['shareMoverSchedule']); + // Basic cron validation: 5 or 6 space-separated fields + if (!preg_match('/^(\S+\s+){4,5}\S+$/', $moverCron)) { + logger("Error: Invalid cron format in shareMoverSchedule: $moverCron"); + return; + } $cronMoverFile = "# Generated mover schedule:\n" . $moverCron . " /usr/local/sbin/mover start |& logger -t move\n\n"; if (file_put_contents("/boot/config/plugins/dynamix/mover.cron", $cronMoverFile) === false) { logger("Error: Failed to write mover.cron file."); } } }
40-52: Validate cron expression format before writing.Similar to
make_unraid_cron(), this function writes the cron schedule without validating the format, risking malformed cron entries.Apply validation:
function make_tune_cron() { global $cfg_moverTuneCron; $tuneCron = isset($_POST['tune_cron']) ? trim($_POST['tune_cron']) : $cfg_moverTuneCron; if (empty($tuneCron)) { return; // Nothing to write } + // Basic cron validation: 5 or 6 space-separated fields + if (!preg_match('/^(\S+\s+){4,5}\S+$/', $tuneCron)) { + logger("Error: Invalid cron format in tune_cron: $tuneCron"); + return; + } $cronTuneFile = "# Generated schedule for Mover Tuning move:\n" . $tuneCron . " /usr/local/emhttp/plugins/ca.mover.tuning/age_mover start |& logger -t move\n\n"; if (file_put_contents("/boot/config/plugins/ca.mover.tuning/mover.tuning.cron", $cronTuneFile) === false) { logger("Error: Failed to write mover.tuning.cron file."); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
plugins/ca.mover.tuning.plg(3 hunks)source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/Mover.tuning.page(6 hunks)source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/Mover.tuning.page
🧰 Additional context used
🧬 Code graph analysis (1)
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php (1)
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/mover.php (1)
logger(11-18)
🪛 GitHub Check: Codacy Static Code Analysis
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php
[warning] 6-6: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L6
Implicit true comparisons prohibited; use === TRUE instead
[warning] 31-31: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L31
Operator ! prohibited; use === FALSE instead
[warning] 34-34: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L34
The use of function file_put_contents() is discouraged
[failure] 44-44: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L44
$_POST['tune_cron'] not unslashed before sanitization. Use wp_unslash() or similar
[warning] 44-44: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L44
Direct use of $_POST Superglobal detected.
[warning] 44-44: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L44
Implicit true comparisons prohibited; use === TRUE instead
[warning] 45-45: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L45
Implicit true comparisons prohibited; use === TRUE instead
[failure] 60-60: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L60
$_POST['cron'] not unslashed before sanitization. Use wp_unslash() or similar
[warning] 60-60: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L60
Direct use of $_POST Superglobal detected.
[warning] 95-95: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L95
Implicit true comparisons prohibited; use === TRUE instead
[warning] 97-97: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L97
The use of function file_exists() is discouraged
[warning] 98-98: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L98
The use of function unlink() is discouraged
[warning] 106-106: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L106
Operator ! prohibited; use === FALSE instead
[warning] 106-106: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L106
The use of function file_exists() is discouraged
⏰ 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: Codacy Static Code Analysis
🔇 Additional comments (4)
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php (2)
6-6: LGTM: Defensive INI parsing and config initialization.The use of error suppression and null coalescing provides graceful fallback when the var.ini file is missing or shareMoverSchedule is undefined.
Also applies to: 14-15
124-136: LGTM: Version-gated Mover Tuning cron management.The logic correctly handles creating, updating, and removing the Mover Tuning cron schedule based on configuration changes, with appropriate version gating for Unraid 7.2.1+.
plugins/ca.mover.tuning.plg (2)
390-409: LGTM: Clear breaking-changes communication.The pre-install notice effectively communicates the behavioral changes in Unraid 7.2.1+, helping users understand the separation between the built-in mover and Mover Tuning.
438-472: LGTM: Proper version-aware mover backup.The version detection and comparison logic correctly determines when to back up the built-in mover script. The
version_ge()function properly handles semantic versioning, and the conditional backup ensures compatibility across Unraid versions.
| // Cron for forced move (unraid mover) | ||
| function make_cron() | ||
| { | ||
| $cronFile = "# Generated schedule for forced move\n" . trim($_POST['cron']) . " /usr/local/sbin/mover.old start |& logger -t move\n\n"; | ||
| file_put_contents("/boot/config/plugins/ca.mover.tuning/mover.cron", $cronFile); | ||
| global $vars; | ||
| $version = $vars['version'] ?? '0.0.0'; | ||
| $mover = version_compare($version, '7.2.1', '<') ? '/usr/local/sbin/mover.old' : '/usr/local/sbin/mover'; | ||
| $cron = isset($_POST['cron']) ? trim($_POST['cron']) : ''; | ||
| $cronFile = "# Generated schedule for forced move:\n{$cron} {$mover} start |& logger -t move\n\n"; | ||
| if (file_put_contents("/boot/config/plugins/ca.mover.tuning/mover.cron", $cronFile) === false) { | ||
| logger("Error: Failed to write forced mover.cron file."); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Prevent writing empty cron schedules and validate format.
The function can write an empty cron line when $_POST['cron'] is not set, and lacks format validation.
Apply these fixes:
function make_cron()
{
global $vars;
$version = $vars['version'] ?? '0.0.0';
$mover = version_compare($version, '7.2.1', '<') ? '/usr/local/sbin/mover.old' : '/usr/local/sbin/mover';
$cron = isset($_POST['cron']) ? trim($_POST['cron']) : '';
+ if (empty($cron)) {
+ logger("Error: No cron schedule provided for forced move.");
+ return;
+ }
+ // Basic cron validation: 5 or 6 space-separated fields
+ if (!preg_match('/^(\S+\s+){4,5}\S+$/', $cron)) {
+ logger("Error: Invalid cron format: $cron");
+ return;
+ }
$cronFile = "# Generated schedule for forced move:\n{$cron} {$mover} start |& logger -t move\n\n";
if (file_put_contents("/boot/config/plugins/ca.mover.tuning/mover.cron", $cronFile) === false) {
logger("Error: Failed to write forced mover.cron file.");
}
}🧰 Tools
🪛 GitHub Check: Codacy Static Code Analysis
[failure] 60-60: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L60
$_POST['cron'] not unslashed before sanitization. Use wp_unslash() or similar
[warning] 60-60: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L60
Direct use of $_POST Superglobal detected.
🤖 Prompt for AI Agents
In
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php
around lines 54 to 65, the function make_cron currently allows writing an empty
or malformed cron line; validate and reject bad input before writing: trim and
require non-empty $_POST['cron'], validate format with a cron regex or simple
split to ensure 5 space-separated fields (or use a stricter pattern to allow
ranges/steps/wildcards), log an error and do not write the file if validation
fails, otherwise build the cronFile and write it as before; ensure
file_put_contents errors are logged and return/exit appropriately so no
empty/invalid cron is saved.
| if (!file_exists("/boot/config/plugins/dynamix/mover.cron")) { | ||
| make_unraid_cron(); | ||
| logger("Mover schedule enabled successfully."); | ||
| } else { | ||
| logger("Error: Mover cron file does not exist"); | ||
| logger("Error: Unable to create mover cron file — schedule not updated."); | ||
| } | ||
| if (version_compare($vars['version'], '7.2.1', '>=')) { | ||
| // Check if the file exists | ||
| if (!file_exists("/boot/config/plugins/ca.mover.tuning/mover.tuning.cron")) { | ||
| make_tune_cron(); | ||
| logger("Mover Tuning schedule enabled successfully."); | ||
| } else { | ||
| logger("Error: Unable to create Mover Tuning cron file — schedule not updated."); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix backwards enable logic and error messaging.
The enable flow logs an error when the cron file already exists, but this is not an error condition. During enable, if the file exists, it means the schedule is already set up (success) or should be refreshed. The current logic prevents updating an existing schedule.
Apply this fix to handle both cases correctly:
} else {
// If mover schedule is enabled
- if (!file_exists("/boot/config/plugins/dynamix/mover.cron")) {
- make_unraid_cron();
+ make_unraid_cron();
+ if (file_exists("/boot/config/plugins/dynamix/mover.cron")) {
logger("Mover schedule enabled successfully.");
} else {
- logger("Error: Unable to create mover cron file — schedule not updated.");
+ logger("Error: Failed to create mover cron file.");
}
if (version_compare($vars['version'], '7.2.1', '>=')) {
- // Check if the file exists
- if (!file_exists("/boot/config/plugins/ca.mover.tuning/mover.tuning.cron")) {
- make_tune_cron();
+ make_tune_cron();
+ if (file_exists("/boot/config/plugins/ca.mover.tuning/mover.tuning.cron")) {
logger("Mover Tuning schedule enabled successfully.");
} else {
- logger("Error: Unable to create Mover Tuning cron file — schedule not updated.");
+ logger("Error: Failed to create Mover Tuning cron file.");
}
}
}🧰 Tools
🪛 GitHub Check: Codacy Static Code Analysis
[warning] 106-106: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L106
Operator ! prohibited; use === FALSE instead
[warning] 106-106: source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php#L106
The use of function file_exists() is discouraged
🤖 Prompt for AI Agents
In
source/ca.mover.tuning/usr/local/emhttp/plugins/ca.mover.tuning/updateCron.php
around lines 106–121, the enable flow currently logs an error when the cron file
already exists; instead treat an existing file as a successful enabled state (or
refresh it). Change the else branches so that if the cron file already exists
you either (a) log a success message like "Mover schedule already enabled." /
"Mover Tuning schedule already enabled." or (b) call the corresponding
make_unraid_cron()/make_tune_cron() to refresh and then log success; do not log
an error when the file exists. Ensure both mover.cron and mover.tuning.cron
follow the same corrected logic.
Signed-off-by: DaRK AnGeL [email protected]
Summary by CodeRabbit
New Features
Bug Fixes / UX
✏️ Tip: You can customize this high-level summary in your review settings.