fix(ZMSKVR): reenable dldb backups#2183
Open
Fabinatix97 wants to merge 9 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughDownloader now writes each output to per-file temp files, helpers prepare backups (no return value) and always move temp files into the destination and unlink temps; helper signatures adjusted and a new copy/unlink helper was added. Changes
Sequence Diagram(s)sequenceDiagram
participant Downloader as Downloader
participant Helpers as DldbHelpers
participant FS as Filesystem
Downloader->>FS: write downloaded content to temp file (tempnam)
Downloader->>Helpers: call checkAndCreateBackup(map filename=>tempPath)
Helpers->>FS: stat/examine existing destination files (md5/metadata)
alt backups required
Helpers->>FS: create backup files/directories
end
Helpers->>FS: copyDownloadedTempFilesToDestinationAndUnlink(map tempPath=>dest)
FS-->>Helpers: report write success/failure
Helpers->>FS: unlink temp files
FS-->>Downloader: final status/errors
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
zmsdldb/bin/dldb-helpers.php (1)
123-132:⚠️ Potential issue | 🟠 MajorThis shared helper now disables backups for
dldbget-munich.
zmsdldb/bin/dldbget-munich:99-108still passes final destination paths after writing the files. With the newmd5_file($destFile) !== md5_file($filePath)check, both operands point to the same file there, so$backupRequirednever becomestrueand that script stops creating backups entirely.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@zmsdldb/bin/dldb-helpers.php`:
- Around line 157-160: The temp file is being deleted unconditionally after a
failed copy, causing loss of the only downloaded payload; modify the logic
around the copy/unlink in the dldb-helpers.php routine so that
`@unlink`($tempFile) is only executed when the copy to $destFile succeeds (i.e.,
move the unlink into the success branch or return/exit on failure), and when
copy fails leave $tempFile in place (and log the error via $this->cli->red as
already done) so retries or inspection can use the downloaded file; refer to the
copy($tempFile, $destFile) call and the subsequent `@unlink`($tempFile) to locate
the change.
In `@zmsdldb/bin/dldbget`:
- Around line 107-116: file_put_contents() can return false so the script
currently adds $tempFile to $downloadedFiles even when the write failed; modify
the block around $newContent / $tempFile to check the return value of
file_put_contents($tempFile, $newContent) and only set
$downloadedFiles[$download['file']] = $tempFile on success, otherwise print an
error (using $cli->red or similar), unlink the failed $tempFile if created, and
continue the loop so downstream backup/copy logic never runs against an
unwritten temp file.
- Around line 119-123: The temp-file cleanup is skipped if
DldbHelpers::checkAndCreateBackup() throws; wrap the backup call in a
try/finally so copyDownloadedTempFilesToDestinationAndUnlink($downloadedFiles)
always runs. Specifically, in the dldbget flow where
$helpers->checkAndCreateBackup($downloadedFiles) is invoked, call that inside a
try block and place
$helpers->copyDownloadedTempFilesToDestinationAndUnlink($downloadedFiles) in the
finally block so temp files are removed even on backup-directory creation
errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6e1c2cd5-19b3-4d6f-9cfe-0dbbfb59bf80
📒 Files selected for processing (2)
zmsdldb/bin/dldb-helpers.phpzmsdldb/bin/dldbget
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@zmsdldb/bin/dldb-helpers.php`:
- Line 66: The current return (int) $rollbackDaySetting[0] silently coerces
invalid input to 0; update the logic around $rollbackDaySetting so you validate
that $rollbackDaySetting[0] is a numeric digit string (e.g. using ctype_digit or
a regex) before casting, and explicitly fall back (return null or a defined
default) when it fails validation instead of casting; locate the code returning
(int) $rollbackDaySetting[0] and replace it with a guarded check that
logs/handles invalid values and only casts when the value is fully numeric.
- Around line 155-156: In the foreach that builds $destFile from
$filenameToTempPath, normalize the key the same way checkAndCreateBackup() does
by using basename($filename) (and validate it's non-empty) before concatenating
with $this->destinationPath so writes and backups target the same sanitized
filename and prevent directory-traversal (e.g., "../") escaping the destination
directory; update references to $filename when computing $destFile and any
subsequent operations to use the sanitized basename variable.
- Around line 157-161: Replace the accidental shell-execution-style backticks
around the unlink call so PHP calls the function normally: locate the line
containing the backticked `@unlink` with the $tempFile variable (after the copy
failure handling) and change it to call `@unlink`($tempFile) so the temporary file
is removed without using shell execution syntax.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 03eb9d0a-32e3-48a2-985e-050c591068a1
📒 Files selected for processing (1)
zmsdldb/bin/dldb-helpers.php
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
zmsdldb/bin/dldbget-munich (1)
73-95: Consider using consistent error output style.The
writeJsonTempFile()helper uses plainechofor errors (lines 78, 84, 89), while the rest of the script usesprint($cli->red(...))for colored error output. This inconsistency reduces visual uniformity in error reporting.Note: Since
$cliis a global variable, it's not directly accessible inside the function without passing it as a parameter or usingglobal $cli. The current approach works but lacks visual consistency.♻️ Optional: Accept $cli as parameter for consistent error styling
-function writeJsonTempFile($data) +function writeJsonTempFile($data, $cli = null) { $json = json_encode($data, JSON_PRETTY_PRINT | JSON_UNESCAPED_UNICODE); if ($json === false) { - echo "Error: JSON encoding failed: " . json_last_error_msg() . "\n\n"; + $msg = "Error: JSON encoding failed: " . json_last_error_msg() . "\n\n"; + print($cli ? $cli->red($msg) : $msg); exit(1); } $tempFile = tempnam(sys_get_temp_dir(), 'dldbget_'); if ($tempFile === false) { - echo "Error: Failed to create temporary file\n\n"; + $msg = "Error: Failed to create temporary file\n\n"; + print($cli ? $cli->red($msg) : $msg); exit(1); } if (file_put_contents($tempFile, $json) === false) { - echo "Error: Failed to write temporary file\n\n"; + $msg = "Error: Failed to write temporary file\n\n"; + print($cli ? $cli->red($msg) : $msg); `@unlink`($tempFile); exit(1); } return $tempFile; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@zmsdldb/bin/dldbget-munich` around lines 73 - 95, The writeJsonTempFile() helper uses echo for error messages which is inconsistent with the rest of the script's colored error output; update writeJsonTempFile to accept a $cli parameter (or declare global $cli) and replace the echo error calls with print($cli->red(...)) messages, keep the same error texts and retain the existing temp file cleanup (`@unlink`) and exit(1) behavior, and update all call sites of writeJsonTempFile to pass the $cli instance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@zmsdldb/bin/dldbget-munich`:
- Around line 73-95: The writeJsonTempFile() helper uses echo for error messages
which is inconsistent with the rest of the script's colored error output; update
writeJsonTempFile to accept a $cli parameter (or declare global $cli) and
replace the echo error calls with print($cli->red(...)) messages, keep the same
error texts and retain the existing temp file cleanup (`@unlink`) and exit(1)
behavior, and update all call sites of writeJsonTempFile to pass the $cli
instance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cdd9cbfd-78da-44d3-9c9b-5a31ae7b899e
📒 Files selected for processing (3)
zmsdldb/bin/dldb-helpers.phpzmsdldb/bin/dldbgetzmsdldb/bin/dldbget-munich
🚧 Files skipped from review as they are similar to previous changes (1)
- zmsdldb/bin/dldb-helpers.php
…mskvr-reenable-dldb-backups
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request Checklist (Feature Branch to
next):nextBranch in meinen Feature-Branch gemergt.Summary by CodeRabbit
Bug Fixes
Refactor