Skip to content

Refactor Backupper and Updater as services and add more options to name backups#873

Open
giuscris wants to merge 2 commits into2.xfrom
feature/improved-backupper-updater
Open

Refactor Backupper and Updater as services and add more options to name backups#873
giuscris wants to merge 2 commits into2.xfrom
feature/improved-backupper-updater

Conversation

@giuscris
Copy link
Member

@giuscris giuscris commented Mar 8, 2026

This pull request refactors how the Backupper and Updater services are instantiated and used throughout the application, moving from manual instantiation to dependency injection via the service container. It also enhances the backup filename generation to be more flexible and informative, and updates the Updater class to accept runtime options for more granular control. These changes simplify controller and command code, improve maintainability, and increase the configurability of backup and update operations.

Dependency Injection & Service Container Integration:

  • Refactored the instantiation of Backupper and Updater across controllers and commands to use the service container, removing manual construction and related helper methods. This centralizes configuration and makes dependency management cleaner and more consistent. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13]

Backup Filename and Options Improvements:

  • Enhanced backup filename generation in Backupper to use an interpolated, customizable format with variables like hostname, site, context, version, and a random string, making backup files more descriptive and unique. The date format in filenames was also updated for better readability.
  • The Backupper::backup method now accepts optional name and hostname parameters for greater flexibility.

Updater Service Enhancements:

  • Updated the Updater class to accept runtime options for force, preferDistAssets, and cleanupAfterInstall in its checkUpdates and update methods, allowing for more dynamic update behavior. [1] [2] [3] [4] [5]
  • Improved version comparison logic in the updater to use the current release from the registry, ensuring correct update eligibility checks.

Codebase Simplification & Cleanup:

  • Removed redundant service loader definitions and helper methods for Backupper and Updater, further streamlining the codebase. [1] [2] [3] [4]

These changes collectively make the backup and update features more robust, maintainable, and user-friendly.

@giuscris giuscris added this to the 2.4.0 milestone Mar 8, 2026
@giuscris giuscris self-assigned this Mar 8, 2026
@giuscris giuscris added the enhancement New feature or request label Mar 8, 2026
@giuscris giuscris requested a review from Copilot March 8, 2026 13:46
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR moves Backupper and Updater to be resolved via the app service container (instead of manual instantiation) and expands runtime configurability for update and backup operations, including more descriptive backup naming.

Changes:

  • Register Backupper and Updater as container services and refactor controllers/commands to receive/resolve them via DI.
  • Extend Updater with runtime options (force, preferDistAssets, cleanupAfterInstall) and adjust version eligibility logic.
  • Enhance backup filename generation with interpolated variables (hostname/site/context/version/random) and update the default backup name format.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
formwork/src/Updater/Updater.php Adds runtime options to update/check flows; refactors release loading and version comparison logic.
formwork/src/Services/Loaders/PanelServiceLoader.php Removes local Updater service definition in favor of global service registration.
formwork/src/Panel/Controllers/UpdatesController.php Injects Backupper via DI for pre-update backups and uses DI-provided Updater.
formwork/src/Panel/Controllers/ToolsController.php Injects Backupper via DI for listing backups.
formwork/src/Panel/Controllers/BackupController.php Injects Backupper via DI for creating panel-triggered backups.
formwork/src/Commands/UpdatesCommand.php Switches to container-resolved Updater/Backupper and passes runtime flags as method options.
formwork/src/Commands/BackupCommand.php Switches to container-resolved Backupper and passes hostname at call time.
formwork/src/Cms/App.php Registers Backupper and Updater in the main container with config-derived options.
formwork/src/Backup/Backupper.php Adds interpolated, length-limited backup filenames and optional name/hostname parameters.
formwork/config/system.yaml Updates default backup naming template to include {{hostname}}.
Comments suppressed due to low confidence (1)

formwork/src/Updater/Updater.php:214

  • loadRelease() returns early once $this->release is set, even if a later call passes a different $preferDistAssets value (e.g. checkUpdates(force: true, preferDistAssets: false) after an earlier check). This makes the new runtime option ineffective within the same Updater instance. Track the previously-used preferDistAssets and reload when it changes (or remove the isset($this->release) cache / include the preference in the cache key).
    private function loadRelease(bool $preferDistAssets = true): void
    {
        if (isset($this->release)) {
            return;
        }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +56 to +57
$filename = rtrim(substr($name, 0, 75 - strlen($suffix)), '-_') . $suffix;

Copy link

Copilot AI Mar 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$filename is derived from interpolated input and then passed into FileSystem::joinPaths($path, $filename). Because joinPaths normalizes .. segments, a name containing path separators (e.g. "../") can escape the configured backup directory. Sanitize the interpolated name to a safe basename (strip / and \, collapse .., and ideally restrict to a whitelist of filename characters) before joining, and consider rejecting/normalizing absolute paths.

Suggested change
$filename = rtrim(substr($name, 0, 75 - strlen($suffix)), '-_') . $suffix;
// Sanitize name to a safe filename: remove path separators and restrict characters
$safeName = preg_replace('/[\/\\\\]+/', '-', $name);
$safeName = preg_replace('/[^A-Za-z0-9._-]/', '-', $safeName);
$filename = rtrim(substr($safeName, 0, 75 - strlen($suffix)), '-_') . $suffix;

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants