Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New version need properties optional in previous package version #1813

Open
mho22 opened this issue Jul 8, 2024 · 2 comments
Open

New version need properties optional in previous package version #1813

mho22 opened this issue Jul 8, 2024 · 2 comments

Comments

@mho22
Copy link

mho22 commented Jul 8, 2024

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 ?

@mho22 mho22 changed the title Previous config lack of properties Previous config missing properties Jul 9, 2024
@mho22 mho22 changed the title Previous config missing properties New package version need properties optional in previous package version Jul 9, 2024
@mho22 mho22 changed the title New package version need properties optional in previous package version New version need properties optional in previous package version Jul 23, 2024
@Nielsvanpach
Copy link
Member

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
Copy link
Author

mho22 commented Aug 30, 2024

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.

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

No branches or pull requests

2 participants