Make Archive->clear_directory dynamic or hookable

[schoenkaft]

Saw the clear_directory method in the Archive still checks is if the name of the directory is static. Which only works when you call it like that. So that whole check should go if we want this function to work with any name.

The issue is that it will remove any directory inputted as the Local Directory in the configuration. If you make a stupid mistake, this could wipe your whole system.

So:

• Should we just wipe it, and hope no one makes a mistake?
• Should we keep it as is, and only wipe it for people who call it static (which is a bit lame, but until now that was kind of fine).
• Should we let it be explicitly called by a filter or action? In that case it might still have the same flaw as the first option.
• Only wipe it when options are set via 'enforced settings'? Which are not implemented yet.
• ...

@harmenjanssen @HammenWS any thoughts?

[harmenjanssen]

I would just wipe it, honestly. I was thinking of checking whether the directory lives at least inside the root of the project, but that might even be too restrictive. For instance, when deploying with Capistrano there's often a shared folder outside the releases folder. If you would want to put the static files over there, the check would be too restrictive.

I dunno. We could also use the configured folder as the target location where we programmatically put the static folder. In that case we don't have to delete a directory based on user input, but delete the directory that we ourselves create within the given directory. That sounds quite safe, right?

[schoenkaft]

I would just wipe it, honestly. I was thinking of checking whether the directory lives at least inside the root of the project, but that might even be too restrictive. For instance, when deploying with Capistrano there's often a shared folder outside the releases folder. If you would want to put the static files over there, the check would be too restrictive.

Well, my point being that I could see someone test driving this thing without properly filling in all Simply Static setting properly. There are no real warning that this plugin deletes 'that' directory, and I don't thing we will have those (like implicitly deleting /, which is properly fine without warnings I think, although root might be required).

I could see the 'is this part of the project' check working though, since we're (as in 'us') always building inside the project itself. And in theory you might be able to 'unfollow' a symbolic link, given that it's linked. If it's not part of the project and not linked, then you'd have a problem indeed. Not ideal, but a possible solution.

I dunno. We could also use the configured folder as the target location where we programmatically put the static folder. In that case we don't have to delete a directory based on user input, but delete the directory that we ourselves create within the given directory. That sounds quite safe, right?

Hmm, interesting approach. Although it would be a bit weird that we're not generating the files in the specific folder you specify. It could also be a temp directory which is moved to the 'specified directory', but then you would have a temp directory (by the real plugin) and a temp directory (by us), and then a final directory which isn't necessarily the same as the temp one since it only adds what's been built, and not deleting the 'final destination directory'.

---

I still think maybe explicitly the task makes sure people are (more) aware of the fact that it will delete that folder.

define('SIMPLY_STATIC_DEPLOY_CONFIG', [
    'aws' => [
        '...' => '...',
    ],
    'url' => env('AWS_SITE_WEBSITE_URL'),
    'cleanup' => true,
]);

Although looking at how we've done stuff previously, that should be:

add_filter('grrr_simply_static_deploy_cleanup', function (bool $value) {
    return true;
});

I'm still not convinced. It's pretty huge thing to do, especially since it's so implicit. We know it happens, but someone else will not. So explicitly choosing to do so just feels saver.

Maybe @HammenWS can be the final judge?

[harmenjanssen]

I agree, it's definitely a huge thing. But that's why I think the plugin should only ever delete things it creates itself. That would be totally safe.
But I get how it gets tangled a bit because we're also acting independently from Simply_Static.

Actually, that makes me wonder: why do we delete stuff? Since the generation is part of Simply_Static's responsibility, why do we take it upon ourselves to delete the static site? (or am I misremembering what gets deleted?)

[HammenWS]

I think that everything we do that differs from the simply static plugin (like deleting files generated by that plugin) should be explicitly asked for.

And to ask your question @harmenjanssen, the comment of the clear_directory function can answer that ;)

Clear the current static site directory, to make sure deleted pages are not deployed again, and potentialy overwriting redirects.

[HammenWS]

And I wonder... When you use the cleanup. Does that mean you cannot use the different tasks ('generate', 'deploy', 'clear cloudfront')?

[schoenkaft]

And I wonder... When you use the cleanup. Does that mean you cannot use the different tasks ('generate', 'deploy', 'clear cloudfront')?

Not sure what you mean? It's invoked before the 'generate' task, so that task starts with a clean slate.

[HammenWS]

@schoenkaft Ah! My bad. I thought it was done after the deploy task. Before generating makes more sense.