Skip to content

Add options config for backup/restore commands#14586

Closed
timkelty wants to merge 6 commits into4.9from
feature/db-command-options
Closed

Add options config for backup/restore commands#14586
timkelty wants to merge 6 commits into4.9from
feature/db-command-options

Conversation

@timkelty
Copy link
Copy Markdown
Contributor

@timkelty timkelty commented Mar 13, 2024

Description

The PR allows \craft\config\GeneralConfig::backupCommand and \craft\config\GeneralConfig::restoreCommand to be an options array to configure how the commands are generated.

Goals

  • make small tweaks easy (eg ignore a single table). Currently this can only be done via event.
  • make it easy to opt-in to postgres's archive format (possibly make this the default for v5?)
  • make it clear when things are going to be replaced or merged (eg ignore-tables)
  • provide a way to tap into command for modification
  • be non-breaking

Valid values include:

<?php
use craft\config\GeneralConfig;
use mikehaertl\shellcommand\Command as ShellCommand;

return GeneralConfig::create()
    ->restoreCommand([
        'archiveFormat' => true, // pgsql-only

        // example: add an arg
        'callback' => fn(ShellCommand $command) => $command->addArg('--verbose'),
    ])
    ->backupCommand([
        'ignoreTables' => ['foo', 'bar'],
        'archiveFormat' => true, // pgsql-only

        // example: remove an arg
        'callback' => function (ShellCommand $command) {
            $command->setArgs(str_replace('--dump-date', '', $command->getArgs()));

            return $command;
        },
    ]);

etc…

  • I ended up leveraging mikehaertl\shellcommand\Command because it was there and were already using it. Easier than stitching strings together.
  • To keep backward compat, ignoreTables can either come from the EVENT_BEFORE_CREATE_BACKUP or from the new ignoreTables option. If both are present, the event will take precedence.
  • The callback could easily be an event instead, but it seemed like it might be a pain to add an event listener just to add an additional arg.

@brandonkelly brandonkelly changed the base branch from develop to 4.9 March 14, 2024 01:17
@brandonkelly
Copy link
Copy Markdown
Member

The options array is keyed by the db driver name

This feels like unnecessary complexity to me. No project uses both.

@timkelty
Copy link
Copy Markdown
Contributor Author

timkelty commented Mar 14, 2024

@brandonkelly yep, already nixed it. Flipping to draft while I tweak some things.

@timkelty timkelty marked this pull request as draft March 14, 2024 01:22
@timkelty timkelty force-pushed the feature/db-command-options branch from fee907a to 6812e99 Compare March 14, 2024 12:08
@timkelty timkelty self-assigned this Mar 14, 2024
@timkelty timkelty marked this pull request as ready for review March 14, 2024 18:49
Comment thread src/db/pgsql/Schema.php
private function _pgpasswordCommand(): string
{
return Platform::isWindows() ? 'set PGPASSWORD="{password}" && ' : 'PGPASSWORD="{password}" ';
}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Moved to \craft\db\DbShellCommand

Comment thread src/db/mysql/Schema.php
FileHelper::writeToFile($this->tempMyCnfPath, $contents, ['append']);

return $this->tempMyCnfPath;
}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Moved to \craft\db\DbShellCommand

Comment thread src/db/mysql/Schema.php
' {database}' .
' >> "{file}"';

return $schemaDump . ' && ' . $dataDump;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Moved to \craft\db\mysql\BackupCommand

Comment thread src/db/mysql/Schema.php
return 'mysql' .
' --defaults-file="' . $this->_createDumpConfigFile() . '"' .
' {database}' .
' < "{file}"';
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Moved to \craft\db\mysql\RestoreCommand

Comment thread src/db/pgsql/Schema.php
' --no-acl' .
' --file="{file}"' .
' --schema={schema}' .
' ' . implode(' ', $ignoredTableArgs);
Copy link
Copy Markdown
Contributor Author

@timkelty timkelty Mar 14, 2024

Choose a reason for hiding this comment

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

Moved to \craft\db\pgsql\BackupCommand

Comment thread src/db/pgsql/Schema.php
' --port={port}' .
' --username={user}' .
' --no-password' .
' < "{file}"';
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Moved to \craft\db\pgsql\RestoreCommand

Comment thread src/db/DbShellCommand.php Outdated
@brandonkelly
Copy link
Copy Markdown
Member

@timkelty After reading #14648 (comment) I’m wondering if it would make more sense to just add a bunch of new top-level config settings for this. Brad is suggesting using a multi-env config in response, which we are sortof moving away from in favor of environment variables, but it doesn’t seem like it will be possible to use environment variables here as currently implemented. (Besides pulling them in manually from config/general.php via App::env().)

@timkelty
Copy link
Copy Markdown
Contributor Author

Closed in favor of #14897

@timkelty timkelty closed this Apr 29, 2024
@brandonkelly brandonkelly deleted the feature/db-command-options branch April 30, 2024 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants