spatie / laravel-backup

A package to backup your Laravel app
https://spatie.be/docs/laravel-backup
MIT License
5.66k stars 762 forks source link

New version need properties optional in previous package version #1813

Open mho22 opened 4 months ago

mho22 commented 4 months ago

With version 9.0.0 a lot of config values become mandatory in config file. In my case in previous version 8.8.1 I didn't need these properties :

backup.source.files.relative_path
backup.database_dump_filename_base
backup.database_dump_file_extension
backup.destination.compression_method
backup.destination.compression_level
backup.password
backup.encryption
backup.tries
backup.retry_delay

notifications.mail.to
notifications.slack.webhook
notifications.slack.channel
notifications.slack.username
notifications.slack.icon

Should this be a good idea to merge arrays with default config inside Config::fromArray in src/Config/Config.php file on line 24 :

 /** @param array<mixed> $data */
 public static function fromArray(array $data): self
 {
     $source = require realpath($raw = __DIR__.'/../config/backup.php') ?: $raw;

     return new self(
         backup: BackupConfig::fromArray(array_merge($source['backup'], $data['backup'])),
         notifications: NotificationsConfig::fromArray(array_merge($source['notifications'], $data['notifications'])),
         monitoredBackups: MonitoredBackupsConfig::fromArray(array_merge($source['monitor_backups'], $data['monitor_backups'])),
         cleanup: CleanupConfig::fromArray(array_merge($source['cleanup'], $data['cleanup']))
     );
 }

This is probably a bit tricky, but the idea to merge missing elements from default config into the current one should probably be the solution ?

Nielsvanpach commented 3 months ago

I suggest to update your own config file when upgrading to a new major, so they keep in sync.

https://github.com/spatie/laravel-backup/blob/main/config/backup.php

mho22 commented 3 months ago

Hi @Nielsvanpach,

This isn't really an "issue"—it's more of a suggestion. As you know it, in Laravel 11, when developers don't need to change the default values in a config file, it's not necessary to include those defaults in the project's config.

In Laravel Backup 8 with Laravel 11, if I wanted to modify just the backup.name, I could do so with a minimal config file :

config/backup.php

<?php

return [

    'backup' => [ 

        'name' => "Foo",

    ],
];

However, in Laravel Backup 9 with Laravel 11, to modify the backup.name, I have to include additional mandatory elements :

config/backup.php

<?php

return [

    'backup' => [

        'name' => "Foo",

        'source' => [

            'files' => [

                'include' => [
                    base_path(),
                ],

                'exclude' => [
                    base_path('vendor'),
                    base_path('node_modules'),
                ],

                'relative_path' => null,
            ],

            'databases' => [
                'mysql',
            ],
        ],

        'database_dump_filename_base' => 'database',

        'database_dump_file_extension' => '',

        'destination' => [

            'compression_method' => ZipArchive::CM_DEFAULT,

            'compression_level' => 9,
        ],

        'password' => env('BACKUP_ARCHIVE_PASSWORD'),

        'encryption' => 'default',

        'tries' => 1,

        'retry_delay' => 0,
    ],
];

Even though the rest of these properties are just defaults from the original laravel-backup/config/backup.php.

My suggestion is to allow the config/backup.php file to remain "clean," including only the configurations that need to be changed, rather than copying the entire file. I wanted to clarify this point before getting your thoughts on it.