Purge database connection only when migrating or seeding multiple tenants

[Sti3bas]

Current implementation of tenancy:migrate and tenancy:seed commands purges the database connection no matter of how many tenants are migrated or seeded. It means that it can’t be used with SQLite in-memory databases and that makes testing more complex.

This PR updates tenancy:migrate and tenancy:seed commands to only purge the database connection when multiple tenants are migrated or seeded, which makes TenantAwareTestCase more simple and elegant compared to the older version (#627 (comment)):

class TenantAwareTestCase extends TestCase
{
    use RefreshDatabase;

    protected $hostname = 'test.test.test';

    protected function setUp()
    {
        parent::setUp();

        app('tenancy.db.drivers')->put('sqlite', SqliteDriver::class);

        config(['tenancy.db.tenant-division-mode' => 'bypass']);

        $website = new Website;        
        app(WebsiteRepository::class)->create($website);
        
        $hostname = new Hostname;
        $hostname->fqdn = $this->hostname;
        $hostname = app(HostnameRepository::class)->create($hostname);
        app(HostnameRepository::class)->attach($hostname, $website);
        
        app()->singleton(CurrentHostname::class, function () use ($hostname) {
            return $hostname;
        });

        (new RouteProvider(app()))->boot();
    }
}

[bkintanar]

Thank you for your time in writing this PR. I will try to have time to check on this. But all seems to be green and ready to go. Just need to double check. 👍

Cheers!

[fletch3555]

Instead of passing the full collection (thus creating a full copy in memory), why don't we make it a boolean?

[Sti3bas]

@fletch3555 I think making it a boolean would make processHandle method too much concentrated to this issue. Maybe passing chunkSize integer would be a better idea? What do you think?

[fletch3555]

I mean, its already specific to this issue, right? All it's checking is if the count > 1 then purging. Would make sense to simply pass in $shouldPurge as a boolean and leave the decision on why elsewhere

[Sti3bas]

I mean processHandle method is not responsible for managing the connection and it should know nothing about it, so adding connection specific logic seems not right to me. All it does is running the provided callback on the chunks of tenants. So passing the information about the chunk seems more clean to me:

$this->processHandle(function (Website $website, $chunkSize) {
    $this->connection->set($website);

    parent::handle();

    if ($chunkSize > 1) {
        $this->connection->purge();
    }
});

[luceos]

Fletch is right though. You're sending an argument into a method that will possibly increase the memory footprint. Either we move the purge out of the processHandle or we use a boolean argument (bool $purge = true).

What do you think?

[Sti3bas]

@luceos I agree with Fletch on memory footprint issue, but I don't agree with his proposed solution. Moving connection handling out of closure seems like a better way to solve this. I've just updated the code. What do you think?

[Sti3bas]

Hey @luceos and @fletch3555, are there still any problems with this PR?

[luceos]

This is okay as is, but please fix the use of $websiteB in the tests, there's no reason to use a variable like that in the scope of the test functions, $website will suffice.

Thanks for your patience, I love your parent::handle() fallback, great idea! 👏

[luceos]

🎆 thank you! Time to tag a new version soon 👍