Skip to content

Conversation

@aerni
Copy link
Contributor

@aerni aerni commented Feb 6, 2025

This PR adds the ability for developers to extend the InstallEloquentDriver command with their own repositories. This is useful for addons that provide their own Stache and Eloquent stores and want to hook into the existing installation command without having to write their own. This is also great for end-users as they now have one single command to migrate all their data.

How it works

Each repository class is a self-contained command responsible for its migration logic. To create a repository command, you should extend the abstract InstallEloquentRepository class. The repository commands are then bundled and executed by the parent InstallEloquent command. Registering your own repository command is as easy as adding this to a service provider:

use Statamic\Console\Commands\InstallEloquent;
use App\Console\Commands\InstallEloquentAdvancedSeo;

public function register(): void
{
    InstallEloquent::register(InstallEloquentAdvancedSeo::class);
}

First proposed approach (not used anymore)

This can be used as follows in the boot method of a Service Provider.

Add a repository with a title and hasBeenMigrated boolean.

InstallEloquentDriver::addRepository('advanced_seo', [
    'title' => 'Advanced SEO',
    'hasBeenMigrated' => config('statamic.eloquent-driver.advanced_seo.driver') === 'eloquent',
]);

Add a migrate method using macros:

InstallEloquentDriver::macro('migrateAdvancedSeo', function () {
    spin(
        callback: function () {
            $this->runArtisanCommand('vendor:publish --tag=statamic-eloquent-advanced-seo-migrations');
            $this->runArtisanCommand('migrate');

            $this->switchToEloquentDriver('advanced_seo');
        },
        message: 'Migrating Advanced SEO...'
    );

    $this->infoMessage('Configured Advanced SEO');

    if ($this->shouldImport('Advanced SEO defaults')) {
        spin(
            callback: fn () => $this->runArtisanCommand('statamic:eloquent:import-advanced-seo'),
            message: 'Importing existing Advanced SEO defaults...'
        );

        $this->infoMessage('Imported existing Advanced SEO defaults');
    }
});

@duncanmcclean
Copy link
Member

duncanmcclean commented Feb 11, 2025

I'll need to have a think about this.

I don't love how you need to register the repository first, then macro the logic into the class separately. It feels a bit weird to me. Maybe we could do something like this instead:

InstallEloquentDriver::addRepository(
    name: 'Advanced SEO",
    hasBeenMigrated: config('statamic.eloquent-driver.advanced_seo.driver') === 'eloquent',
    callback: function () {
        // Your logic...
    }
);

However, since the command is pretty tied to the Eloquent Driver right now, I'm a little bit aprehensive about it being extended by addon developers when their stuff isn't part of the official Eloquent Driver. Maybe it needs renaming? 🤔

Anyways, I'll have a think about this as it's something I'd probably use in my addons if it made its way into Core.

@aerni
Copy link
Contributor Author

aerni commented Feb 11, 2025

I agree that the implementation isn't optimal as it stands. What you're suggesting with the callback function is what I had in the beginning. But the issue is that this won't allow using existing methods from the command class, like $this->runArtisanCommand('migrate').

I think a better approach would be a modular class-based command. Each repository would be represented by a class with its handle, name, and migration logic. And the InstallEloquentDriver command would just bundle them up. This should allow for maximum extensibility and keep things clean. Happy to whip something up if you agree on the direction.

However, since the command is pretty tied to the Eloquent Driver right now, I'm a little bit aprehensive about it being extended by addon developers when their stuff isn't part of the official Eloquent Driver. Maybe it needs renaming? 🤔

I don't see this as an issue. To me, the Eloquent Driver is simply a vessel for defining existing Stache stores that can be moved to Eloquent. I don't make the mental cut of "Oh, this is only for Statamic related Eloquent stuff".

However, I do think there is room for improvement. Like, I don't like how the eloquent-driver.php config is responsible for setting the driver. It would make more sense to me if there was a global stores.php config that handles setting the desired driver. And the eloquent-driver.php and stache.php config files are responsible for configuring the individual stores. Something along these lines:

return [

    /*
    |--------------------------------------------------------------------------
    | Store Drivers
    |--------------------------------------------------------------------------
    |
    | Here you may define the drivers you want to use for each store.
    | Supported drivers: "file", "eloquent"
    |
    */

    'drivers' => [
        'collections' => 'file',
        'entries' => 'eloquent',
    ],

];

@aerni aerni marked this pull request as draft February 14, 2025 00:11
@aerni
Copy link
Contributor Author

aerni commented Feb 14, 2025

I whipped up a quick proof of concept for a repository-based installation command. For now, this covers collections, collection_trees, and entries. I created a new InstallEloquent command that and reverted the changes to the InstallEloquentDriver command so that you can compare them side by side.

How it works

Each repository class is a self-contained command responsible for its migration logic. To create a repository command, you should extend the abstract InstallEloquentRepository class. The repository commands are then bundled and executed by the parent InstallEloquent command. Registering your own repository command is as easy as adding this to a service provider:

use Statamic\Console\Commands\InstallEloquent;
use App\Console\Commands\InstallEloquentAdvancedSeo;

public function register(): void
{
    InstallEloquent::register(InstallEloquentAdvancedSeo::class);
}

@duncanmcclean
Copy link
Member

Thanks for this pull request!

I actually quite like the approach you've taken around each "thing" being its own class.

However, this change overlaps with some potential improvements we're thinking about for Statamic 6 (or shortly afterwards) around data storage for addons, and making it easier to store "things" in flat files and in a database.

To avoid us merging this before we have a chance to properly think through the whole addon data storage story, I'm going to close this pull request for now and recommend providing your own command instead.

Thanks for all your work on this though.

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

Successfully merging this pull request may close these issues.

2 participants