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

Pipe should not fail and throw an exception when directory doesn't exist #38

Open
CedsTrash opened this issue Sep 28, 2024 · 1 comment
Assignees
Labels
bug Something isn't working

Comments

@CedsTrash
Copy link

Hi, some backup pipes throw an exception because the directory they are supposed to backup don't exist. For example, when the backup runs on a schedule, on a brand new Statamic installation where no user has been created yet, the "Users" pipe fails because the "users" folder ins't there just yet.

public function backup(Zipper $zip, Closure $next)
    {
        $usersDir = Stache::store('users')?->directory();

        if (!$usersDir) {
            throw new RuntimeException('Users directory not found');
        }

        $zip->addDirectory($usersDir, static::getKey());

        return $next($zip);
    }

It seems to me that the backup shouldn't necessarily fail in this case. It could simply notify (or ignore) the missing folder and continue to the next pipe.

Also, this logic doesn't exist on the "Content" pipe, the check isn't there and the exception isn't caught:

public function backup(Zipper $zip, Closure $next)
    {
        $contentPath = config('backup.content_path');

        $zip->addDirectory($contentPath, static::getKey());

        return $next($zip);
    }

I worked around it by cloning the different pipes to add this to their backup methods:

public function backup(Zipper $zip, Closure $next)
{
    $contentPath = config('backup.content_path');

    if (!File::isDirectory($contentPath)) {
        File::makeDirectory($contentPath);
    }

    $zip->addDirectory($contentPath, static::getKey());

    return $next($zip);
}

I understant that not every use case will need or accept that the folders are created when not found, but in my case, it is just a matter of time before they actually get created by Statamic.

Could we ignore missing directories by default, and add a flag somewhere to give the option to create them if necessary?

Thanks for your great work.

@NeoIsRecursive NeoIsRecursive added the bug Something isn't working label Sep 28, 2024
@NeoIsRecursive NeoIsRecursive self-assigned this Sep 28, 2024
@NeoIsRecursive
Copy link
Collaborator

Thanks for bringing this up!

It gets a little tricky here I think:

I can see it as if the directory we want to include in the backup doesn't exist then the backup should fail because we couldn't do what we wanted to, right? (we should fix so the pipes actually throw "good" exceptions if the directories don't exists etc.)

But in the case of the user pipe, since we get the directory from statamic, we can assume the directory should be be created (like you said, just a matter of time before Statamic does). Might be nice to do some refactoring so the content pipe gets its paths from the stache as well (will actually create a separate issue for this).

I will have to think about it some more, but I think I am leaning towards throwing instead of creating/skipping the directories, at least per default and have some config option to create instead.

Thanks again, and have a nice day :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants