laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.55k stars 11.03k forks source link

Problem modifying column type from text to mediumText or longText #21847

Closed antonioribeiro closed 5 years ago

antonioribeiro commented 7 years ago

Description:

Changing columns from text to mediumText or longText changes to the wrong size.

Text is supposed to be 65536 chars long, but the sources are getting information from static::calculateDoctrineTextLength($fluent['type']);:

Which sets text (default) as 255, which is the string length in Laravel:

/**
 * Calculate the proper column length to force the Doctrine text type.
 *
 * @param  string  $type
 * @return int
 */
protected static function calculateDoctrineTextLength($type)
{
    switch ($type) {
        case 'mediumText':
            return 65535 + 1;
        case 'longText':
            return 16777215 + 1;
        default:
            return 255 + 1;
    }
}

Then for doctrine, mediumText is 65536 bytes, which is Text and so on.

So this migration:

    Schema::table('ci_runs', function (Blueprint $table) {
        $table->mediumText('log')->change();
    });

Changes a text column to the very same text length.

Dumping the results from

image

I get

image

Which is also odd, since Doctrine is basically adding one byte to the columns length.

And looking at the table structure in SequelPro, this is before:

image

This is after a migration to longText (went to the biggest to test it):

Schema::table('ci_runs', function (Blueprint $table) {
        $table->longText('log')->change();
});

image

And this is what happens if I manually modify the column:

alter table ci_runs modify column log longtext;

image

antonioribeiro commented 7 years ago

Creating it as mediumText:

image

Works fine. So this is how I'm doing it:

public function migrateUp()
{
    Schema::table('ci_runs', function (Blueprint $table) {
        $table->mediumText('log_new')->nullable();
    });

    Database::statement('update ci_runs set log_new = log;');

    Schema::table('ci_runs', function (Blueprint $table) {
        $table->dropColumn('log');
        $table->renameColumn('log_new', 'log');
    });
}

To get

image

To have that column in the correct order, I changed it to

$table->mediumText('log_new')->nullable()->after('was_ok');
Dimimo commented 7 years ago

Very good point. That explains why intervention/imagecache can't hold larger pictures when the cache system is set to database and the cache.value column is set to mediumtext. Me too had to manually intervene in the database to override the Laravel suggested migration change. It's an issue/bug that is there since at least L5.1.

Your solution only works on a test server as log will lose all existing values. A luxury some may not have.

antonioribeiro commented 7 years ago

@Dimimo It will loose nothing, it creates the new column, copies old logs to it, then drops the old one and renames it back to log

sisve commented 7 years ago

First off, it isn't Doctrine that is adding the +1 values, that's still in Laravel's code base.

The calculations have been done this way for over three years, since at least Laravel 5.0 / 2014. I tracked it back to https://github.com/laravel/framework/commit/24828f0735459921f5466e893c6bf11013a13250 but that may not be the first occurrence.

Changing these now will introduce breaking changes to migrations that has worked for a very long time. Every column someone has changed to these types in the last three years will suddenly be changed differently when the migrations are reran in development environments, putting the local environment out-of-sync with production.

My views on any changes to the migration system is as my views to any changes to migrations that have been pushed from the local machine; it should never happen. Migrations need to be stable. Being stable over time and releases is more important, according to me, than being correct.

There are already a lot of red boxes in the migration documentation about weird cases and unsupported things. We could document this behavior in another red box.

paulofreitas commented 7 years ago

I've found that changing any text column type to another text column type does not work at all, even changing Illuminate\Database\Schema\Grammars\RenameColumn::calculateDoctrineTextLength(). It works only when changing from/to a non-text column.

Unfortunately a proper fix to this would require mapping every type not supported by Doctrine. This seems to be the same issue from #8840. I haven't found yet, but maybe there's an easier way to fake those types to force Doctrine to change the columns correctly. 🤔

cbotsikas commented 6 years ago

This is not an issue of Laravel. dbal has the issue. It ignores the TextType length property when comparing the columns so it doesn't know it actually changed in order to generate the appropriate DDL. You can find more info at https://github.com/doctrine/dbal/issues/2566#issuecomment-395938146. Hopefully it will be sorted out soon, you are more than welcome to jump in the discussion. @antonioribeiro, i like your workaround (https://github.com/laravel/framework/issues/21847#issuecomment-340048091), looks cleaner that using Alter db statements.

driesvints commented 5 years ago

Since this seems to be an issue with dbal I'm going to close this here.

88chico commented 5 years ago

Another workaround that I`ve found. change the column to string (VARCHAR) type with the length of 70000 (or bigger than 65536) like this: $table->string('data', 70000)->change(); since the limit of varchar in doctrine/dbal is 65535, it change it to mediumtext. on the same way if you need longtext you can put the length as 16777216 or bigger. (see MySqlPlatform->getClobTypeDeclarationSQL function)

I know it is not elegant or pretty, but it is a working workaround..

uphlewis commented 5 years ago

@88chico You're my hero

ryancwalsh commented 5 years ago

@88chico Thank you! I wish "doctrine/dbal" would fix this bug reported >2 years ago, but until they do, your hack seemed to work for my purposes.

bhulsman commented 5 years ago

Check solution proposed here: https://github.com/doctrine/dbal/issues/2566#issuecomment-480217999, very nice one!

jeromejaglale commented 4 years ago

The solution mentioned above by @bhulsman is the easiest workaround: making a change to the column's comment triggers the type change:

// up
$table->mediumText('log')->comment(' ')->change();

// down
$table->mediumText('log')->comment('')->change();