laravel / framework

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

[Bug] Schema builder - renameColumn fails on table with enum columns #1186

Closed itsgoingd closed 11 years ago

itsgoingd commented 11 years ago

Example code:

Schema::create('presentations', function($table)
{
...
    $table->enum('repeat_unit', array('DAY', 'WEEK', 'MONTH', 'YEAR'))->default('DAY');
...
});

Schema::table('presentations', function($table)
{
    $table->renameColumn('created', 'created_at');
});

Result:

[Doctrine\DBAL\DBALException] Unknown database type enum requested, Doctrine\DBAL\Platforms\MySqlPlatform may not support it.

taylorotwell commented 11 years ago

This would be a Doctrine DBAL issue. We may just need to document that this is the case until (and if) they implement something upstream.

taylorotwell commented 11 years ago

Updated documentation.

shnhrrsn commented 11 years ago

The docs aren't correct, they say renaming enum columns isn't support, but this actually happens when renaming any column on a table that contains an enum column.

It looks like the "workaround" for this is to just tell Doctrine an enum is a string: http://wildlyinaccurate.com/doctrine-2-resolving-unknown-database-type-enum-requested

I've attempted to do this in Laravel by doing the following, but the issue still occurs:

$platform = Schema::getConnection()->getDoctrineSchemaManager()->getDatabasePlatform();
$platform->registerDoctrineTypeMapping('enum', 'string');

I'm not entirely sure how Laravel interacts with Doctrine so I might grabbing the wrong platform instance (or adding it too late?). Any insight on how to make this work inside of Laravel?

danharper commented 11 years ago

Has anyone had any luck with a workaround on this? Causing some major headaches.

iflow commented 11 years ago

Workaround, during development (if there exists no important data): Drop the column and add a new one.

public function up()
{
    Schema::table('users', function(Blueprint $table)
    {
        $table->dropColumn('name');
    });

    Schema::table('users', function(Blueprint $table)
    {
        $table->text('username');
    });
}
alexw23 commented 10 years ago

Any resolution to this issue?

ejunker commented 10 years ago

+1 just ran into this issue. I know it is a Doctrine DBAL issue but I wonder if there is something Laravel can do to provide a workaround.

alexw23 commented 10 years ago

I think doctrine is gone in 4.1 :-)

On 22 Nov 2013, at 2:01 am, Eric Junker notifications@github.com wrote:

+1 just ran into this issue. I know it is a Doctrine DBAL issue but I wonder if there is something Laravel can do to provide a workaround.

— Reply to this email directly or view it on GitHub.

patrickheeney commented 10 years ago

Couldn't laravel leave in this functionality but throw an exception if Doctrine is not loaded? I personally do not need it so I would prefer to not to include doctrine for this one feature. Another suggestion is why not just extend schema to add column renaming which uses doctrine if you need it? It seems like some others would be interested and could help maintain the package?

alexw23 commented 10 years ago

I have a feeling it's been replaced using DB commands in 4.1

On 22 Nov 2013, at 6:27 am, Patrick Heeney notifications@github.com wrote:

Couldn't laravel leave in this functionality but throw an exception if Doctrine is not loaded? I personally do not need it so I would prefer to not to include doctrine for this one feature. Another suggestion is why not just extend schema to add column renaming which uses doctrine if you need it? It seems like some others would be interested and could help maintain the package?

— Reply to this email directly or view it on GitHub.

alexw23 commented 10 years ago

Yeah it's still in 4.1

https://github.com/laravel/framework/blob/master/src/Illuminate/Database/Schema/Blueprint.php

On 22 Nov 2013, at 6:27 am, Patrick Heeney notifications@github.com wrote:

Couldn't laravel leave in this functionality but throw an exception if Doctrine is not loaded? I personally do not need it so I would prefer to not to include doctrine for this one feature. Another suggestion is why not just extend schema to add column renaming which uses doctrine if you need it? It seems like some others would be interested and could help maintain the package?

— Reply to this email directly or view it on GitHub.

crynobone commented 10 years ago

@patrickheeney

Couldn't laravel leave in this functionality but throw an exception if Doctrine is not loaded?

It has been moved to suggest https://github.com/laravel/framework/blob/master/composer.json#L90. If anyone need it, you just update your app composer.json to include it and it would be available.

patrickheeney commented 10 years ago

@crynobone I thought I saw something like that. Thanks for the information.

reedmaniac commented 10 years ago

Ran into this today. It seems that every call of Scheme doing a renameColumn will create an entirely new DoctrineConnection(). That means we cannot simply do a registerDoctrineTypeMapping() globally for enum. So, your options are to either add 'enum' => 'string' directly to Doctrine\DBAL\Platforms\MySqlPlatform's initializeDoctrineTypeMappings() or modify Illuminate\Database\Connection's getDoctrineSchemaManager() to look like this:

public function getDoctrineSchemaManager()
{
    $schema = $this->getDoctrineDriver()->getSchemaManager($this->getDoctrineConnection());
    $schema->getDatabasePlatform()->registerDoctrineTypeMapping('enum', 'string');
    return $schema;
}

It seems extremely unlikely that Doctrine is going to change, so I can only hope that @taylorotwell might think about adding this change in to Laravel. The database I am using is legacy, but I think I am going to try and move it away from enum.

taylorotwell commented 10 years ago

I don't get why everyone suggests just register the mapping as string. Wouldn't that ruin your enum column when you rename another column on the table?

reedmaniac commented 10 years ago

Ran a test with an additional update and it seems to do nothing to the enum() in MySQL. Doctrine seems to be using these type mappings primarily in _getPortableTableColumnDefinition. You likely know Doctrine a bit better than I, but with this I can at least get my migrations working.

upngo commented 10 years ago

As @shnhrrsn pointed out, the docs are wrong and still haven't changed (7 months after being reported, and it's a fairly common issue for anyone new to laravel). screen shot 2014-02-24 at 12 34 48 pm The issue is renaming ANY column on a table that has an enum.

upngo commented 10 years ago

https://github.com/laravel/framework/pull/2133

doctrine thus renaming columns has been removed from laravel core so this probably isn't going to get fixed.

Found another interesting case, if you create a migration which converts enums to strings (raw queries) and then use Schema to rename columns in that table (which now has no enums) then you can run this migration back and forth using artisan migrate(:rollback) but if you run the migration in pretend mode artisan migrate --pretend then the doctrine error above will come up. I assume it's because it ignores the first query changing the enums, then evaluates what the second query would do (rename a column on a table that still has enums!) and complains.

debiprasad commented 10 years ago

For those who are looking for an workaround, I have a better solution. Instead of using Schema, use Query Builder and write SQL Query for those rename operations.

Gorzas commented 9 years ago

Is there any improvement in this issue? I'm having the same problem here. :(

kebian commented 9 years ago

I'm new to Laravel and I'm currently scratching my head over, what I presume, is the same issue. In my case the datatype it's complaining about is a 'point' which isn't even the field I'm trying to rename.

upngo commented 9 years ago

@kebian the docs are still incorrect, reading my last comments you'll see that you can't rename any columns on tables which contain enums

kebian commented 9 years ago

@upngo That does indeed seem to be the case, however my table doesn't contain any enum columns but does contain a geospatial "point" field. Perhaps the bug is with any non-standard field types.

simonhamp commented 9 years ago

I've noticed a lot of these issues are specifically related to using the renameColumn() method. However, it's also an issue when using change() (yes, even when the column being changed is not an ENUM).

Seems the most convenient way around this (but by no means the best way) is to drop the column and re-create... unless/until Laravel is updated with a work-around or Doctrine gets updated.

upngo commented 9 years ago

@simonhamp alternative to dropping (and losing all rows) would be to just use a raw query in whatever db language you're using

@kebian yes this is the case, if you see the comment at the top of the page from @shnhrrsn it's that DBAL doesnt know enum and probably not point either, the work around was to tell mysql it's just a string.

This issue isn't going anywhere though -> unsubscribing.

simonhamp commented 9 years ago

@upngo good point. Will do that in future

malhal commented 9 years ago

Hit this issue today on Laravel 5 and dbal 2.5.1, can't call change on any column if my table has any enum or point columns in it.

pinkal-vansia commented 9 years ago

This is something I could not believe at first. But thanks to Paul Bill I did it as following

public function up()
{
    Schema::table('projects', function(Blueprint $table)
    {
           DB::statement('ALTER TABLE projects CHANGE slug url VARCHAR(200)');
    });
}

public function down()
{
    Schema::table('projects', function(Blueprint $table)
    {
           DB::statement('ALTER TABLE projects CHANGE url slug VARCHAR(200)');
    });
}
eckenroed commented 9 years ago

Follow up to this problem as I had it too...

I figured out fairly simple solution at least to my case.

As @malcolmhall said, it was not letting me update ANY columns in my table if that table had an enum in it.

If I moved the renaming unto it's own function instead of trying to include it with the rest of my migration it worked. For example....

Changed A Migration Like This

public function up()
{
   Schema::table('mytable', function(Blueprint $table) {
      $table->dropColumn('randomColumn');
      $table->string('newColumn', 25)->after('id');
      $table->renameColumn('oldColumnName', 'newColumnName');
   });
}

To this

public function up()
{
   Schema::table('mytable', function(Blueprint $table) {
      $table->dropColumn('randomColumn');
      $table->string('newColumn', 25)->after('id');
   });

   Schema::table('mytable', function(Blueprint $table) {
      $table->renameColumn('oldColumnName', 'newColumnName');
   });

}

By moving the renaming of columns to it's own function it seemed to work. The crazy part is NONE of the fields in my example are enum fields, but since the table contains enum fields I was getting this error.

emir commented 9 years ago

When I tried to use change got same issue on Laravel 5 and doctrine/dbal ~2.5.0. But I solved the problem with using raw query like this:

DB::statement("ALTER TABLE table_name MODIFY COLUMN column_name ENUM('Here','is','choices')");
endel commented 9 years ago

I've fixed this for hook with a pretty ugly workaround: https://github.com/doubleleft/hook/blob/2ffde1352649513919b51bb591f0668f70a37925/src/Database/Schema/Builder.php#L397-L435

Basically I'm creating a new class extending the EnumType with the allowed options for the migration, and registering it into Doctrine.

Hope it helps!

SteveEdson commented 8 years ago

Still hitting this issue 3 years later, having to use the manual DB statement solution which is a shame.

crynobone commented 8 years ago

3 years later and Doctrine hasn't fix this issue which is a shame.

diogogomeswww commented 8 years ago

My simple solution was to use raw SQL.

public function up()
{
    DB::statement('ALTER TABLE kois ALTER doitsu SET DEFAULT 0');
    DB::statement('ALTER TABLE kois ALTER male SET DEFAULT 0');
}
rohansahai commented 8 years ago

running into the same issue with a json blob on the table :(

Zerquix18 commented 8 years ago

still no fix...

t202wes commented 8 years ago

@taylorotwell

bickerstoff commented 8 years ago

Just to clarify @reedmaniac 's path, I changed it here: vendor/doctrine/dbal/lib/Doctrine/DBAL/Platforms/MySqlPlatform.php

dreferreira commented 8 years ago

I too just came into this issue 2 days ago. ENUM is widely used and I can't, in any way shape or form, understand why someone would think it should not be supported. Nor did I realize this was an issue that clearly causing some issues and has been sat on for over 3 years.... However I've come across a few resources that could possibly be the fix for this issue!

http://stackoverflow.com/questions/37793349/error-on-php-artisan-migrate-laravel http://docs.doctrine-project.org/projects/doctrine-orm/en/latest/cookbook/mysql-enums.html

Hopefully one of these are helpful!

peppy commented 8 years ago

Is there a reason this isn't getting fixed? These kinds of 3-year-still-not-fixed bugs are just insane. I'm pretty sure using enum in a table is not a rare case.

themsaid commented 8 years ago

@peppy if you have a fix in mind you can open a PR.

henritoivar commented 8 years ago

This worked for me in laravel 5.2 with doctrine/dbal@^2.5 . When you have an enum on your table and you want to change any of the columns on the table you will have to:

 public function up()
    {
       Schema::getConnection()->getDoctrineSchemaManager()->getDatabasePlatform()->registerDoctrineTypeMapping('enum', 'string');

        Schema::table('jobs', function(Blueprint $table)
        {
            $table->decimal('latitude', 10, 6)->nullable()->change();
        });
    }
cmosguy commented 7 years ago

@henritoivar well this solved my issue! what a crazy hack.

jorygeerts commented 7 years ago

@crynobone Can you explain why you feel Doctrine should fix this? Laravel's Blueprint class offers both a enum() and a renameColumn() method, yet they cannot be used in combination.

crynobone commented 7 years ago

Can you explain why you feel Doctrine should fix this?

My response is to response to "Still hitting this issue 3 years later, having to use the manual DB statement solution which is a shame.", Don't just put the blame on Laravel when it's been 3 years and Doctrine still hasn't solved it.

Besides that, monkey-patching is never a good solution (which was questioned by Taylor), and Taylor himself mention this at the very first reply.

renameColumn() highly depends on Doctrine, enum() doesn't.

ethaizone commented 7 years ago

If you come across this comment, I think this will help you.

You must have same question as I and other think about this problem and don't want to use DB statement. I'm in same boat as you so I search all in google and found solution that I think it is best practise right now. I use Laravel 5.3. Current DBAL hash some update and depend on @henritoivar solution only is not work. DBAL will throw error that you need to define type in DBAL. They need sql processor.

First. Please read all this page. http://doctrine-orm.readthedocs.io/projects/doctrine-orm/en/latest/cookbook/mysql-enums.html This is doc that talk about why doctrine don't implement about enum on it.

Second. I will keep it short. This is my implement.

<?php

use Illuminate\Support\Facades\Schema;
use Illuminate\Database\Schema\Blueprint;
use Illuminate\Database\Migrations\Migration;
use Doctrine\DBAL\Types\Type;

class AlterCartCustomersMakeGenderNullable extends Migration
{
    /**
     * Run the migrations.
     *
     * @return void
     */
    public function up()
    {
        Schema::getConnection()->getDoctrineSchemaManager()->getDatabasePlatform()->registerDoctrineTypeMapping('enum', 'string');

        Type::addType('enum', \Extended\Doctrine\DBAL\Types\GenderEnumType::class);

        Schema::table('cart_customers', function(Blueprint $table) {
            // add nullable
            // must call change to tell we change field attribute
            $table->enum('gender', ['male', 'female'])->nullable()->change();
        });
    }

    /**
     * Reverse the migrations.
     *
     * @return void
     */
    public function down()
    {
        Schema::getConnection()->getDoctrineSchemaManager()->getDatabasePlatform()->registerDoctrineTypeMapping('enum', 'string');

        Type::addType('enum', \Extended\Doctrine\DBAL\Types\GenderEnumType::class);

        Schema::table('cart_customers', function(Blueprint $table) {
            // revert nullable
            $table->enum('gender', ['male', 'female'])->nullable(false)->change();
        });
    }
}
<?php

namespace Extended\Doctrine\DBAL\Types;

use Doctrine\DBAL\Types\Type;
use Doctrine\DBAL\Platforms\AbstractPlatform;

class EnumType extends Type
{
    protected $name;
    protected $values = array();

    public function getSQLDeclaration(array $fieldDeclaration, AbstractPlatform $platform)
    {
        $values = array_map(function($val) { return "'".$val."'"; }, $this->values);

        return "ENUM(".implode(", ", $values).")";
    }

    public function convertToPHPValue($value, AbstractPlatform $platform)
    {
        return $value;
    }

    public function convertToDatabaseValue($value, AbstractPlatform $platform)
    {
        if (!in_array($value, $this->values)) {
            throw new \InvalidArgumentException("Invalid '".$this->name."' value.");
        }
        return $value;
    }

    public function getName()
    {
        return $this->name;
    }

    public function requiresSQLCommentHint(AbstractPlatform $platform)
    {
        return true;
    }
}
<?php

namespace Extended\Doctrine\DBAL\Types;

class GenderEnumType extends EnumType
{
    protected $values = array('male', 'female');
}

After you saw these files. You may found it look stupid but this is solution right now. Problem is DBAL don't receive another attribute so I cannot pass enum values to it.

gabykant commented 7 years ago

Still not solved ? Oups I have to switch from ENUM to int with a default value set to 0

zschuessler commented 7 years ago

Can someone managing this GitHub repository please summarize the current state of this issue?

My own opinion is that sure, monkey patching isn't desirable, but it's quite often necessary in our field. Newcomers to Laravel are hitting this problem in large numbers. This is bad for the reputation of the product.

Yes Doctrine should fix it, but it has been three years. Now it falls on the Laravel framework to create an intuitive development experience for its users.

(PS - this is an amazing framework and thank you everyone for your contributions to it. I'm sure we can find a solution for this simple problem.)

taylorotwell commented 7 years ago

@zschuessler feel free to make a PR to fix the issue if you can do it.

macpatel commented 7 years ago

wow! I didn't knew that its an 3 years old issue until i faced the issue myself.