Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MYSQL 8 Migrating text to alternative text type does not change column #46492

Closed
liamh101 opened this issue Mar 16, 2023 · 13 comments
Closed

MYSQL 8 Migrating text to alternative text type does not change column #46492

liamh101 opened this issue Mar 16, 2023 · 13 comments

Comments

@liamh101
Copy link
Contributor

  • Laravel Version: 10.3.3
  • PHP Version: 8.2.3
  • Database Driver & Version: MYSQL 8.0.32
  • doctrine/dbal Version: 3.6.1

Description:

The column type change will not be made during a text column's migration to medium or long text. Keeping the column as a text type.

Stepping through the process, Illuminate\Database\Schema\Grammars\ChangeColumn class line 44 $tableDiff->isEmpty() returns true.

Not sure what other columns this affects. This bug occurred during my migration process to Laravel 9 to 10.

Steps To Reproduce:

  1. Set MySQL config in database.php as follows (based on MySQL8 docker image):
        'mysql' => [
            'driver' => 'mysql',
            'url' => null,
            'host' => 'mysql',
            'port' => '3306',
            'database' => 'test',
            'username' => 'root',
            'password' => 'root',
            'unix_socket' => '',
            'charset' => 'utf8mb4',
            'collation' => 'utf8mb4_unicode_ci',
            'prefix' => '',
            'prefix_indexes' => true,
            'strict' => true,
            'engine' => null,
            'options' => extension_loaded('pdo_mysql') ? [
                PDO::MYSQL_ATTR_SSL_CA => null,
                PDO::MYSQL_ATTR_SSL_VERIFY_SERVER_CERT => 0,
            ] : [],
        ],
  1. Create a migration that creates a table with three text columns.
  2. Create another migration changing these columns:
   public function up(): void
   {
       Schema::table('tableName', static function (Blueprint $table) {
           $table->mediumText('col1')->change();
           $table->longText('col2')->change();
           $table->mediumText('col3')->change();
       });
   }
  1. Run migrations.
  2. Columns are still text type.
@liamh101
Copy link
Contributor Author

liamh101 commented Mar 16, 2023

Here's a repo that reproduces the error: https://github.com/liamh101/laravel-column-bug

It's around doctrine/dbal as without it, everything is fine, and the types change as expected.

@ankurk91
Copy link
Contributor

See #45487

@liamh101
Copy link
Contributor Author

liamh101 commented Mar 17, 2023

Thanks for the response. I'll remove dbal from our codebase and check if it doesn't affect anything else.

However, I think this problem should be fixed. On closer inspection, what I believe is happening is the current type, and the new type is checked using an enum in the dbal library. However, any text column generates the same enum. Meaning the migration thinks nothing has changed.

If there's no movement by the end of today, I'll spend some time and get a fix for this over the weekend.

Edit: Some people may find dbal still included in their project as it's a third-party requirement of barryvdh/laravel-ide-helper and other popular laravel packages.

@driesvints
Copy link
Member

@hafezdivandari what are your thoughts here?

@hafezdivandari
Copy link
Contributor

hafezdivandari commented Mar 17, 2023

@driesvints the issue seems to be behavioral change on doctrine/dbal 3.x, unlike 2.x, it seems that DBAL doesn't change the column if the type is not changed (although the column length has changed). Check the test changes on PR #45289

I've added a test for changing an integer column to text (with doctrine/dbal) on PR #44101 and it works fine, but changing text to mediumText won't work as the OP said.

@liamh101 you have 2 options:

  1. Remove doctrine/dbal package.
  2. or If you have to keep this package for any reason as you mentioned, just call Schema::useNativeSchemaOperationsIfPossible() method within the boot method of your App\Providers\AppServiceProvider class or within your migrations files to be able to use native schema operations instead.

@driesvints
Copy link
Member

@hafezdivandari thank you!

@liamh101 does any of that work for you?

@liamh101
Copy link
Contributor Author

@driesvints

Removing the DBAL won't work as some child dependencies may require it (like barryvdh/laravel-ide-helper for dev).

Schema::useNativeSchemaOperationsIfPossible() does work. However, I'll write and PR a fix for this when you have DBAL without the schema setting change.

This bug will significantly affect multi-tenanted applications, where migrations are often run for new clients. This could potentially go under the radar and break their systems without realising it.

@liamh101
Copy link
Contributor Author

@hafezdivandari @driesvints spoke too soon.

It does fix the text type bug. However, it has introduced another bug in which a nullable column was not set or reverted. I need to investigate exactly what's happening. Would you like me to append this issue or create a new one?

@hafezdivandari
Copy link
Contributor

hafezdivandari commented Mar 17, 2023

@liamh101 Would you please check the L10 documentation on modifying column (it's different than L9), or check the related PR #45487 for more details on the topic.

When modifying a column, you must explicitly include all of the modifiers you want to keep on the column definition - any missing attribute will be dropped.

@liamh101
Copy link
Contributor Author

@hafezdivandari Ah right, apologies, can't migrate to the new schema operations then. As this would break in multiple places without rewriting a lot of my migrations.

Both of those solutions first provided won't work for the time being.

@hafezdivandari
Copy link
Contributor

hafezdivandari commented Mar 17, 2023

@liamh101 no problem, as I said you may call that function only on your last migration only:

public function up(): void
{
    Schema::useNativeSchemaOperationsIfPossible()

    Schema::table('tableName', static function (Blueprint $table) {
        $table->mediumText('col1')->nullable()->change();
    });

    Schema::useNativeSchemaOperationsIfPossible(false);
}

Unfortunately this bug on doctrine/dbal is not easy to fix on Laravel side (maybe not possible). Maybe you want to report this issue on their repo instead?

@driesvints
Copy link
Member

I think, from reading up here, there's not much we can do ourselves, sorry. Feel free to either switch to non-DBAL and modify your code as instructed by @hafezdivandari above or open an issue on the DBAL repo. Thanks

@mreduar
Copy link

mreduar commented May 29, 2023

I've also recently run into this problem with a composer update I've done in a multitenant project, all the tests and seeders broke because the column can't be changed anymore, in my case I was trying to change from an enum to a string, but it failed. it was left as enum, that was what broke it until i saw the PR with native support and tied the threads.

Doing what @hafezdivandari suggests

Schema::useNativeSchemaOperationsIfPossible()

// migrations here

Schema::useNativeSchemaOperationsIfPossible(false);

Solve the problem. thanks for that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants