Add `prefix` helper method to SettingsRepositoryInterface

[tobyzerner]

Just a random syntactical idea. Instead of:

// constructor
$this->settings = $settings;

// body
$this->settings->get('flarum-auth-facebook.app_id');
$this->settings->get('flarum-auth-facebook.app_secret');

We could use:

// constructor
$this->settings = $settings->withPrefix('flarum-auth-facebook');

// body
$this->settings->get('app_id');
$this->settings->get('app_secret');

Also, for extension prefixes, we should probably make the best practice vendor.package, i.e. flarum.auth-facebook instead of flarum-auth-facebook.

[franzliedke]

That's a great idea!

[tobyzerner]

Questions:

1. What should the method be called? prefix or withPrefix or setPrefix or forExtension?
2. Should the repository be immutable like PSR-7? i.e. should the prefix method return a cloned repository with the prefix set, or should it just set a prefix on the current repository? I guess question 1 affects this too - withPrefix would imply that it's immutable, whereas setPrefix would not.

[franzliedke]

I'd go for immutability, therefore probably withPrefix. The great thing: We can implement a separate class that implements the SettingsRepositoryInterface. This class will decorate another repo instance and simply pass through all methods, but with the appropriate prefix...

[tobyzerner]

Awesome :D

[luceos]

This class will decorate another repo instance and simply pass through all methods, but with the appropriate prefix...

Love that idea! And this will help extension devs a lot, now it's up to a dev to decide on the setting prefix.

[tobyzerner]

I'm wondering if there's a way we can automate this, i.e. automatically set it up so that the SettingsRepository is passed in with the prefix derived from the extension's composer.json.

And I'm also wondering if there are other areas we want to do this to reduce inter-extension conflicts, e.g. automatically force tables created in migrations to have a prefix, or columns added to a core table to have a prefix.

[luceos]

@tobscure maybe we can add methods to get/set config variables for an extension in the Extension object and require that object on specific events. This should assist on further automation.

I also prefer that extension conflict prevention actually. In fact with migrations it shouldn't be too hard to accomplish right away.

[tobyzerner]

Here's my idea for a new migrations API which auto-prefixes stuff.

Format

Migration files are formatted just like the new extension API, i.e. they return arrays of "tasks". But instead of each item in the array being a callable, it can be one of three things:

• An instance of Flarum\Database\MigrationInterface which has up and down methods, or
• An array containing up and down closures, or
• A single closure (equivalent to having an up migration and no down one).

Whichever format, the callables will have their dependencies injected when run (including an instance of the Extension class, which is used to generate the appropriate table/column prefixes).

Flarum\Migration classes are similar in nature to Extenders – building upon the current migration shortcuts implemented by @franzliedke.

return [
    Flarum\Migration\Table::create('tags', function (Blueprint $table) {
        // tags table blueprint
        // the table name is automatically prefixed as `tobscure_tags_tags`
    }),

    Flarum\Migration\Table::discussions()
        ->addColumn('example', ['string', 'nullable' => true]), // `tobscure_tags_example`

    Flarum\Migration\Table::custom('tags') // `tobscure_tags_tags`
        ->changeColumn('slug', ['string' => 255], ['string' => 100])
        ->addUnique('slug')
        ->blueprint([ // for anything more complex
            'up' => function (Blueprint $table) {},
            'down' => function (Blueprint $table) {}
        ]),

    Flarum\Migration\Table::external('tobscure_likes_something'), // don't add the prefix

    Flarum\Migration\Settings::add([ // write settings only if they're not already set
        'max_primary_tags' => '1', // `tobscure.tags.max_primary_tags` etc.
        'min_primary_tags' => '1',
        'max_secondary_tags' => '3',
        'min_secondary_tags' => '0'
    ]),

    Flarum\Migration\Settings::overwrite([ // overwrite settings
        'core.forum_title' => 'Your Forum, Now With Tags' // don't add the prefix
    ]),

    function (Builder $schema) {
        // do something
    },

    [
        'up' => function (Builder $schema) {
            // do something
        },
        'down' => function (Builder $schema) {
            // undo something
        }
    ]
];

Using the prefixes in extension code is pretty straight-forward, since we make the Extension instance available as $this. Any custom models must be registered by the extension in order for the extension's table prefix to be automatically applied to that model's queries:

// bootstrap.php

return [  
    Flarum\Extend\ApiResource::discussion()
        ->attributes(function ($model) {
            return ['example' => $model[$this->column('example')]];
        }),

    Flarum\Extend\Model::register(Tag::class)
];

Structure

Our current approach is to break migrations up into timestamped files for each and every change (including those during development, i.e. in-between releases) – like Laravel and RoR does. However, this results in a pretty ugly database history being included in the project, like this. Even mistakes/changes made during development in between releases are included and everything must be run upon installation, which seems silly.

What if we at least consolidate the migrations into just one new file per release? And there's no real need to timestamp them, we can just name them as sequential numbers, like:

000001.php
000002.php
000003.php

Alternatively...

What if we dropped the concept of chronological database migrations altogether and used esoTalk's "magic migrations": http://esotalk.org/docs/database#schema

The idea is that you simply define your whole table blueprint in one place (example), making changes to it as necessary. Then whenever an upgrade is performed, the app works out the differences between the defined blueprint and the current state of the database, and automatically applies the necessary changes to get the database up-to-date.

We'd still need to provide the ability to run some sort of per-version upgrade logic before/after the "magic migrations" are run (example).

[tobyzerner]

Been mulling this over some more, and I have changed my mind and think we should keep the migration structure as it is for now. Contrary to what I said earlier, I think having one migration file per release is actually more arbitrary than one per action, doesn't offer any real tangible benefit, and will probably just cause pain for contributors during development. And same with sequential numbers, as we have to arbitrarily set an upper limit rather than being limited by calendar time. Not to mention the pain it'll cause with the upgrade from the current version. Let's just continue to do what has worked well for Laravel/RoR/etc for so long.

The proposed format, on the other hand, we can still look at implementing. But this is low priority, because in any case, we can maintain BC with the current ['up' => ..., 'down' => ...] format.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. We do this to keep the amount of open issues to a manageable minimum. In any case, thanks for taking an interest in this software and contributing by opening the issue in the first place!