spatie / laravel-settings

Store strongly typed application settings
MIT License
1.23k stars 114 forks source link

Column change from `json` to `longText` does not remove json_valid() #209

Closed Muffinman closed 1 year ago

Muffinman commented 1 year ago

Description:

It seems that when adding this package to a fresh L10 install, it causes the following column change migration to behave oddly.

I believe this is not specifically a bug of this repo, but ultimately caused by the requirement of doctrine/dbal. Maybe this needs a L10 specific release without this requirement (since it's not needed now)? Or it needs a version bump?

I'm adding a migration to encrypt some previously json columns as below:

Column created as json in earlier migration:

return new class extends Migration
{
    public function up(): void
    {
        Schema::create('test_models', function (Blueprint $table) {
            $table->id();
            $table->json('settings')->nullable();
            $table->timestamps();
        });
    }
}

Then altered to be longText so I can encrypt them in DB:

return new class extends Migration
{
    public function up(): void
    {
        Schema::table('test_models', function (Blueprint $table) {
            $table->longText('settings')->nullable()->change();
        });
    }
}

After this change(), the DB still has the json_valid() checks assigned to the column, meaning encrypted data fails to insert.

Steps To Reproduce:

  1. Clone example repo containing sample migrations: https://github.com/Muffinman/json-valid-example
git clone https://github.com/Muffinman/json-valid-example
cd json-valid-example
composer install
  1. Migrate fresh DB
    php artisan migrate:fresh

(DB Correctly altered and json_valid check is removed.)

CREATE TABLE `test_models` (
  `id` bigint(20) unsigned NOT NULL AUTO_INCREMENT,
  `settings` longtext DEFAULT NULL,
  `created_at` timestamp NULL DEFAULT NULL,
  `updated_at` timestamp NULL DEFAULT NULL,
  PRIMARY KEY (`id`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci;
  1. Install spatie/laravel-settings and try again:
    composer require spatie/laravel-settings
    php artisan migrate:fresh

This does not remove json_valid from the schema:

CREATE TABLE `test_models` (
  `id` bigint(20) unsigned NOT NULL AUTO_INCREMENT,
  `settings` longtext CHARACTER SET utf8mb4 COLLATE utf8mb4_bin DEFAULT NULL CHECK (json_valid(`settings`)),
  `created_at` timestamp NULL DEFAULT NULL,
  `updated_at` timestamp NULL DEFAULT NULL,
  PRIMARY KEY (`id`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci;
Muffinman commented 1 year ago

Also I did start off writing this bug report in laravel/framework, because I kind of feel that this should work find with doctrine/dbal installed, it's even mentioned in the upgrade guide, and installed 3.6.1 - which should be a supported version.

Muffinman commented 1 year ago

I found a solution here: https://github.com/laravel/framework/issues/46467

I'll leave this issue open though, because I'm not sure if this package needs DBAL at all on 10.x

rubenvanassche commented 1 year ago

All tests are green without doctrine so let's remove it.