[4.x] Optionally delete storage after tenant deletion

[lukinovec]

The PR adds a test in which I propose a way of deleting the tenant's storage after the tenant's deletion (#226). Apart from the addition of the config key, nothing new is really implemented – I only tested the solution, and I'll wait for @stancl's feedback (not sure if it's the best way or where's the best place to put the listener).

The storage directory would get deleted on the TenantDeleted event:

Event::listen(TenantDeleted::class, function(TenantDeleted $event) {
    if (config('tenancy.filesystem.delete_storage_after_tenant_deletion')) {
        File::deleteDirectory($event->tenant->run(fn () => storage_path()));
    }
});

[codecov-commenter]

Codecov Report

Merging #938 (99c071d) into master (62d19c5) will decrease coverage by 0.50%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master     #938      +/-   ##
============================================
- Coverage     88.01%   87.51%   -0.51%     
- Complexity      400      412      +12     
============================================
  Files           110      113       +3     
  Lines          1110     1145      +35     
============================================
+ Hits            977     1002      +25     
- Misses          133      143      +10     

Impacted Files	Coverage Δ
src/Features/CrossDomainRedirect.php	100.00% <100.00%> (ø)
src/Listeners/DeleteTenantStorage.php	100.00% <100.00%> (ø)
src/Tenancy.php	92.45% <100.00%> (ø)
src/helpers.php	55.55% <100.00%> (-2.34%)	⬇️
src/Commands/Run.php	100.00% <0.00%> (ø)
src/Database/Models/ImpersonationToken.php	100.00% <0.00%> (ø)
src/Middleware/InitializeTenancyByPath.php	100.00% <0.00%> (ø)
src/Exceptions/StatefulGuardRequiredException.php	100.00% <0.00%> (ø)
...trappers/Integrations/ScoutTenancyBootstrapper.php	0.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[stancl]

And the event listener can probably go into the TenancyServiceProvider. Or do you think we could make this an optional feature, and move the logic there?

So the config option would have a note that you need to enable the feature.

Not sure if separating this into a feature is good, but it's a potential way to find a good place for the event listener.

[lukinovec]

And the event listener can probably go into the TenancyServiceProvider. Or do you think we could make this an optional feature, and move the logic there?

So the config option would have a note that you need to enable the feature.

Not sure if separating this into a feature is good, but it's a potential way to find a good place for the event listener.

This doesn't feel like an optional feature in the same way as user impersonation, tenant config, etc., so I'm not sure about making this a feature too.

Maybe we could put the logic in a job and add that job to TenantDeleted's job pipeline in the TenancyServiceProvider stub's events()? We could also remove the config key and just let the users enable/disable this by adding/removing the job from the pipeline

[stancl]

Yeah, that sounds like a good idea — get rid of the config key, and just make it a commented out listener in the TenancyServiceProvider stub. That would solve the problem with putting the listener somewhere perfectly.

And yeah, it isn't an optional feature of the same "category" as the other ones.

So in the test, you'd just enable the listener using Event::listen() instead of setting config keys.

[stancl]

I'm thinking if we even need the new events now? If the developers are already adding this by interacting with listeners, and the class can be easily extended, then the events may be unnecessary?

[lukinovec]

I'm thinking if we even need the new events now? If the developers are already adding this by interacting with listeners, and the class can be easily extended, then the events may be unnecessary?

I see, you're right. I removed the events.

[stancl]

I think this should be changed (in all places) to use the deleting tenant event. It's similar to what we discussed — while hooking into TenantDeleted does work, it feels wrong to be initializing tenancy for a tenant that doesn't exist anymore.

[stancl]

Can you create a TODO in the docs repo so that we document this in v4?