MokoConsulting/MokoSuiteBackup
1
0
Fork 0
Code Issues 28 Pull Requests Releases 2 Wiki Activity

Consolidate backup entry points and retention into shared helpers #230

New Issue
Open
opened 2026-07-06 01:32:51 +00:00 by jmiller · 2 comments
jmiller commented 2026-07-06 01:32:51 +00:00
Owner
Copy Link
Copy Source

Goal

"All backup functions should be in a shared helper if they aren't." Audit found duplicated logic across the codebase:

Retention (two implementations, both local-only)

  • Engine/RetentionManager::prune() — runs after each backup.
  • plg_system_mokosuitebackup doCleanup() / deleteBackupRecord() — hourly admin-side sweep, re-implementing the same age+count prune plus its own file-delete copy. It also applied the global max_age_days / max_backups fallback that RetentionManager did not.

Uploader factory (three copies)

  • createUploaderFromParams() duplicated verbatim in BackupEngine and SteppedBackupEngine, and needed a third time by retention for remote deletion.

Backup entry points (three bypassing the shared service)

BackupRunner is the shared synchronous entry point, but the backend BackupsController, the API BackupsController, and the legacy cli/mokosuitebackup.php each instantiated BackupEngine directly — losing the normalized complete/warning/fail status.

Changes (in progress)

  • New Engine/RemoteUploaderFactory::create() — single home for the type→class map + key-prefixing; both engines now delegate to it.
  • RetentionManager is now the single retention authority: accepts global-default fallback, adds pruneOrphans(), and deletes from remotes. The plugin's doCleanup() delegates to it and its duplicate deleteBackupRecord() is removed.
  • Backend/API/legacy-CLI backup triggers now route through BackupRunner.

(The dashboard AJAX SteppedBackupEngine interactive path is intentionally left separate.)

https://claude.ai/code/session_01WbGBN9VyRK61zczYWcCQ2i

## Goal "All backup functions should be in a shared helper if they aren't." Audit found duplicated logic across the codebase: ### Retention (two implementations, both local-only) - `Engine/RetentionManager::prune()` — runs after each backup. - `plg_system_mokosuitebackup` `doCleanup()` / `deleteBackupRecord()` — hourly admin-side sweep, re-implementing the same age+count prune plus its own file-delete copy. It also applied the **global** `max_age_days` / `max_backups` fallback that `RetentionManager` did not. ### Uploader factory (three copies) - `createUploaderFromParams()` duplicated verbatim in `BackupEngine` and `SteppedBackupEngine`, and needed a third time by retention for remote deletion. ### Backup entry points (three bypassing the shared service) `BackupRunner` is the shared synchronous entry point, but the backend `BackupsController`, the API `BackupsController`, and the legacy `cli/mokosuitebackup.php` each instantiated `BackupEngine` directly — losing the normalized complete/warning/fail status. ## Changes (in progress) - New `Engine/RemoteUploaderFactory::create()` — single home for the type→class map + key-prefixing; both engines now delegate to it. - `RetentionManager` is now the **single retention authority**: accepts global-default fallback, adds `pruneOrphans()`, and deletes from remotes. The plugin's `doCleanup()` delegates to it and its duplicate `deleteBackupRecord()` is removed. - Backend/API/legacy-CLI backup triggers now route through `BackupRunner`. (The dashboard AJAX `SteppedBackupEngine` interactive path is intentionally left separate.) https://claude.ai/code/session_01WbGBN9VyRK61zczYWcCQ2i
jmiller commented 2026-07-06 01:32:53 +00:00
Author
Owner
Copy Link
Copy Source

Branch created: feature/230-consolidate-backup-entry-points-and-rete

git fetch origin
git checkout feature/230-consolidate-backup-entry-points-and-rete
Branch created: [`feature/230-consolidate-backup-entry-points-and-rete`](https://git.mokoconsulting.tech/MokoConsulting/MokoSuiteBackup/src/branch/feature/230-consolidate-backup-entry-points-and-rete) ```bash git fetch origin git checkout feature/230-consolidate-backup-entry-points-and-rete ```
jmiller commented 2026-07-06 01:36:27 +00:00
Author
Owner
Copy Link
Copy Source

Done in #231, merged to dev. RemoteUploaderFactory now owns the type→class map (both engines delegate); RetentionManager is the single retention authority (global-default fallback + pruneOrphans()) with the plugin delegating to it; backend/API/legacy-CLI backup triggers route through BackupRunner. Dashboard SteppedBackupEngine interactive path left separate by design.

Done in #231, merged to `dev`. `RemoteUploaderFactory` now owns the type→class map (both engines delegate); `RetentionManager` is the single retention authority (global-default fallback + `pruneOrphans()`) with the plugin delegating to it; backend/API/legacy-CLI backup triggers route through `BackupRunner`. Dashboard `SteppedBackupEngine` interactive path left separate by design.
Sign in to join this conversation.
Labels
Clear labels
component: admin
Admin UI and views
 component: api
REST API
 component: engine
Backup/restore engine
 component: remote
Remote storage (FTP/GDrive)
 component: scheduler
Scheduled tasks
bug
Something is not working
 duplicate
This issue or pull request already exists
 enhancement
New feature
 help wanted
Need some help
 invalid
Something is wrong
 question
More information is needed
 wontfix
This won't be fixed
No labels
Priority Medium
Type Feature
Milestone
No items
No Milestone
Assignees
Clear assignees
jmiller (Jonathan Miller)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: MokoConsulting/MokoSuiteBackup#230
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?