spatie / laravel-settings

Store strongly typed application settings
MIT License
1.2k stars 110 forks source link

v3 Upgrade Guide Missing Steps #217

Closed ashleyshenton closed 1 year ago

ashleyshenton commented 1 year ago

I upgraded to the v3 release today but had issues with the database repository. The issue in particular was a missing default value for the locked column.

image

I had a look at the diff between 3.0.0 and 2.8.3 and noticed that the migration has changed. The changes add a default value of false to the locked column. There is another change involving adding a unique index. This will also cause issues for those updating as the v3 release has changed to use upserts which depend on a unique constraint. This needs adding to the upgrade guide to make anyone upgrading aware.

I've used this in my project.

<?php

use Illuminate\Database\Migrations\Migration;
use Illuminate\Database\Schema\Blueprint;
use Illuminate\Support\Facades\Schema;

return new class extends Migration
{
    /**
     * Run the migrations.
     */
    public function up(): void
    {
        Schema::table('settings', function (Blueprint $table): void {
            $table->boolean('locked')->default(false)->change();

            $table->unique(['group', 'name']);

//            $table->dropIndex(['group']);
        });
    }

    /**
     * Reverse the migrations.
     */
    public function down(): void
    {
        Schema::table('settings', function (Blueprint $table): void {
            $table->boolean('locked')->default(null)->change();

            $table->dropUnique(['group', 'name']);

//            $table->index('group');
        });
    }
};

I would also advise that the original index is dropped, as the new unique one covers all queries and is used over the old one. I know this thanks to @aarondfrancis "left to right, no skipping". This isn't completely necessary though so is commented out in the above snippet.

Here are some examples from a project where I use this package.

image

image

Notice how the composite unique index is used even when querying on group alone. I've looked through the database repository and all queries are written in a way that this index will be used.

rubenvanassche commented 1 year ago

Hi @ashleyshenton,

Thanks for the heads up, I've released v3 too soon, woops 😬

I'll merge your PR in an instant

As for the group index, I don't think we ever had such an index in the default migrations so I'll move it out of your PR, I guess you've added it yourself?

ashleyshenton commented 1 year ago

Hi @rubenvanassche thank you for replying.

No it came as part of the package when I installed. It's still there in the latest v2 release and the current v3.

https://github.com/spatie/laravel-settings/blob/2.8.3/database/migrations/create_settings_table.php.stub#L14

https://github.com/spatie/laravel-settings/blob/3.0.0/database/migrations/create_settings_table.php.stub#LL14C18-L14C18

rubenvanassche commented 1 year ago

Oh good point, damn completely looked over that function. I'll fix the guide

ashleyshenton commented 1 year ago

Haha no worries. Thanks for accepting my PR.

rubenvanassche commented 1 year ago

No problem, time for vacation for me 🏖️