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

[2.10.0] Do not add CHARACTER SET for some column types #3714

Closed
williamdes opened this issue Nov 4, 2019 · 34 comments
Closed

[2.10.0] Do not add CHARACTER SET for some column types #3714

williamdes opened this issue Nov 4, 2019 · 34 comments

Comments

@williamdes
Copy link
Contributor

williamdes commented Nov 4, 2019

Bug Report

Q A
BC Break no
Version 2.10.0

Summary

I use Laravel and when composer did the update from 2.9.2 to 2.10.0 our CI broke

Current behaviour

Generate:

ALTER TABLE xxxx CHANGE mycolName mycolName INT UNSIGNED CHARACTER SET utf8mb4 DEFAULT NULL COLLATE `utf8mb4_unicode_ci

How to reproduce

Using Laravel

<?php

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

class AlterMyColName extends Migration
{
    /**
     * Run the migrations.
     *
     * @return void
     */
    public function up()
    {
        Schema::table('xxxx', function (Blueprint $table) {
            $table->integer('mycolName')->unsigned()->nullable()->change();
        });
    }

    /**
     * Reverse the migrations.
     *
     * @return void
     */
    public function down()
    {
        Schema::table('xxxx', function (Blueprint $table) {
            $table->string('mycolName')->nullable()->change();
        });
    }
}

Expected behaviour

Do not ADD CHARACTER SET
Generate:

ALTER TABLE xxxx CHANGE mycolName mycolName INT UNSIGNED DEFAULT NULL COLLATE `utf8mb4_unicode_ci

More

Introduced by : #3418 ?

SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'CHARACTER SET utf8mb4 DEFAULT NULL COLLATE `utf8mb4_unicode_ci`' at line 1 (SQL: ALTER TABLE xxxx CHANGE mycolName mycolName INT UNSIGNED CHARACTER SET utf8mb4 DEFAULT NULL COLLATE `utf8mb4_unicode_ci`)
 Illuminate\Database\QueryException  : SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'CHARACTER SET utf8mb4 DEFAULT NULL COLLATE `utf8mb4_unicode_ci`' at line 1 (SQL: ALTER TABLE xxxx CHANGE mycolName mycolName INT UNSIGNED CHARACTER SET utf8mb4 DEFAULT NULL COLLATE `utf8mb4_unicode_ci`)
  at vendor/laravel/framework/src/Illuminate/Database/Connection.php:664
    660|         // If an exception occurs when attempting to run a query, we'll format the error
    661|         // message to include the bindings with SQL, which will make this exception a
    662|         // lot more helpful to the developer instead of just the database's errors.
    663|         catch (Exception $e) {
  > 664|             throw new QueryException(
    665|                 $query, $this->prepareBindings($bindings), $e
    666|             );
    667|         }
    668|
  Exception trace:
  1   Doctrine\DBAL\Driver\PDOException::("SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'CHARACTER SET utf8mb4 DEFAULT NULL COLLATE `utf8mb4_unicode_ci`' at line 1")
      vendor/doctrine/dbal/lib/Doctrine/DBAL/Driver/PDOConnection.php:63
  2   PDOException::("SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'CHARACTER SET utf8mb4 DEFAULT NULL COLLATE `utf8mb4_unicode_ci`' at line 1")
      vendor/doctrine/dbal/lib/Doctrine/DBAL/Driver/PDOConnection.php:61
  3   PDO::prepare("ALTER TABLE xxxx CHANGE mycolName mycolName INT UNSIGNED CHARACTER SET utf8mb4 DEFAULT NULL COLLATE `utf8mb4_unicode_ci`", [])
      vendor/doctrine/dbal/lib/Doctrine/DBAL/Driver/PDOConnection.php:61
  4   Doctrine\DBAL\Driver\PDOConnection::prepare("ALTER TABLE xxxx CHANGE mycolName mycolName INT UNSIGNED CHARACTER SET utf8mb4 DEFAULT NULL COLLATE `utf8mb4_unicode_ci`")
      vendor/laravel/framework/src/Illuminate/Database/Connection.php:452
  5   Illuminate\Database\Connection::Illuminate\Database\{closure}("ALTER TABLE xxxx CHANGE mycolName mycolName INT UNSIGNED CHARACTER SET utf8mb4 DEFAULT NULL COLLATE `utf8mb4_unicode_ci`", [])
      vendor/laravel/framework/src/Illuminate/Database/Connection.php:657
  6   Illuminate\Database\Connection::runQueryCallback("ALTER TABLE xxxx CHANGE mycolName mycolName INT UNSIGNED CHARACTER SET utf8mb4 DEFAULT NULL COLLATE `utf8mb4_unicode_ci`", [], Object(Closure))
      vendor/laravel/framework/src/Illuminate/Database/Connection.php:624
  7   Illuminate\Database\Connection::run("ALTER TABLE xxxx CHANGE mycolName mycolName INT UNSIGNED CHARACTER SET utf8mb4 DEFAULT NULL COLLATE `utf8mb4_unicode_ci`", [], Object(Closure))
      vendor/laravel/framework/src/Illuminate/Database/Connection.php:459
  8   Illuminate\Database\Connection::statement("ALTER TABLE xxxx CHANGE mycolName mycolName INT UNSIGNED CHARACTER SET utf8mb4 DEFAULT NULL COLLATE `utf8mb4_unicode_ci`")
      vendor/laravel/framework/src/Illuminate/Database/Schema/Blueprint.php:97
  9   Illuminate\Database\Schema\Blueprint::build(Object(Illuminate\Database\MySqlConnection), Object(Illuminate\Database\Schema\Grammars\MySqlGrammar))
      vendor/laravel/framework/src/Illuminate/Database/Schema/Builder.php:278
@williamdes
Copy link
Contributor Author

cc @altertable

@Ocramius
Copy link
Member

Ocramius commented Nov 4, 2019

Not really a BC break, but rather a regression.

@morozov morozov added this to the 2.10.1 milestone Nov 4, 2019
@ABartelt
Copy link

ABartelt commented Nov 4, 2019

Our ci also broke this day with the same issue metioned above while executing:
$table->dateTime('date_of_birth')->change()->nullable()
which was not nullable before mirgrating. Maybe this issue affects more types than just numeric columns.
We worked around with explicit version set for dbal to 2.9.2 in composer.json.

@williamdes williamdes changed the title [2.10.0] Do not add CHARACTER SET for INT column [2.10.0] Do not add CHARACTER SET for some column types Nov 4, 2019
@morozov
Copy link
Member

morozov commented Nov 4, 2019

@williamdes, @ABartelt could you clarify where the column charset comes from? According to the logic introduced in #3418,

if (isset($tableColumn['characterset'])) {
$column->setPlatformOption('charset', $tableColumn['characterset']);
}

The corresponding SQL is generated only if the charset is specified on the column. And I don't see code in the DBAL that would define any default charset.

Can this issue be reproduced using only the DBAL APIs, without any wrapper libraries?

@cyhii
Copy link
Contributor

cyhii commented Nov 5, 2019

When generating alter table SQL by Laravel migration, the column diff is calculated as Laravel Blueprint describes:

  1. get changed column (return \Doctrine\DBAL\Schema\Column), this step only return a type-changed column

https://github.com/illuminate/database/blob/e0d5ca153f0204c5312465109246bcf03b7fa603/Schema/Grammars/ChangeColumn.php#L105-L107

  1. set changed attributes one by one

https://github.com/illuminate/database/blob/e0d5ca153f0204c5312465109246bcf03b7fa603/Schema/Grammars/ChangeColumn.php#L81-L90

Now see the bugged migration, The Blueprint
$table->integer('mycolName')->unsigned()->nullable()->change();
said one column named 'mycolName' should be changed to unsigned integer, that's OK. But when it's original type was varchar/text, there is a implicit change: the charset definition should be removed. Neither Laravel nor Doctrine handled this.

I think there are two ways to fix this, one is add extra checks in \Doctrine\DBAL\Schema\Table::changeColumn, another is modify Laravel migrate, add checks after changed attributes are set. I prefer the second one.

@morozov
Copy link
Member

morozov commented Nov 5, 2019

Thank you @altertable for the explanation.

I think there are two ways to fix this, one is add extra checks in \Doctrine\DBAL\Schema\Table::changeColumn(), another is modify Laravel migrate, add checks after changed attributes are set. I prefer the second one.

In order to make the proper decision, we need a scenario that reproduces the issue using only the DBAL API.

@andrey-helldar
Copy link

Changing my json field also produces an error like that of the author.

My code:

public function up()
{
    Schema::table('meta_tags', function (Blueprint $table) {
        $table->json('meta_keywords')->change();
    });
}

public function down()
{
    Schema::table('meta_tags', function (Blueprint $table) {
        $table->text('meta_keywords')->change();
    });
}

@Ocramius
Copy link
Member

Ocramius commented Nov 6, 2019

As mentioned above, we need this to be laid out in DBAL API only (no laravel) to identify the issue.

@cyhii
Copy link
Contributor

cyhii commented Nov 7, 2019

Just one sample test:

    public function testAlterTableChangeColumnType(): void
    {
        $table = new Table('alter_table_change_column_type');
        $table->addColumn('foo', 'string')->setPlatformOption('charset', 'ascii');


        $diffTable  = clone $table;
        $diffTable->changeColumn('foo', [
            'type' => Type::getType(Types::INTEGER)
        ]);

        $comparator = new Comparator();
        $diff = $comparator->diffTable($table, $diffTable);

        self::assertNotNull($diff);

        self::assertEquals(
            ['ALTER TABLE alter_table_change_column_type CHANGE foo foo INT NOT NULL'],
            $this->platform->getAlterTableSQL($diff)
        );
    }

This will cause failures:

1) Doctrine\Tests\DBAL\Platforms\MariaDb1027PlatformTest::testAlterTableChangeColumnType
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    0 => 'ALTER TABLE alter_table_change_column_type CHANGE foo foo INT NOT NULL'
+    0 => 'ALTER TABLE alter_table_change_column_type CHANGE foo foo INT CHARACTER SET ascii NOT NULL'
 )

2) Doctrine\Tests\DBAL\Platforms\MySQL57PlatformTest::testAlterTableChangeColumnType
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    0 => 'ALTER TABLE alter_table_change_column_type CHANGE foo foo INT NOT NULL'
+    0 => 'ALTER TABLE alter_table_change_column_type CHANGE foo foo INT CHARACTER SET ascii NOT NULL'
 )

3) Doctrine\Tests\DBAL\Platforms\MySqlPlatformTest::testAlterTableChangeColumnType
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    0 => 'ALTER TABLE alter_table_change_column_type CHANGE foo foo INT NOT NULL'
+    0 => 'ALTER TABLE alter_table_change_column_type CHANGE foo foo INT CHARACTER SET ascii NOT NULL'
 )

@lcobucci
Copy link
Member

lcobucci commented Nov 7, 2019

@altertable you're providing the charset there by configuring the platform options. What happens if you remove that?

@cyhii
Copy link
Contributor

cyhii commented Nov 7, 2019

@lcobucci The test will pass if remove the platform options.

@nicolasbuch
Copy link

Experiencing the same issue when trying to change a column data type.
Works with doctrine/dbal 2.9.3 but breaks with 2.10.0

Schema::table('users', function (Blueprint $table) {
    $table->string('avatar');
});
Schema::table('users', function (Blueprint $table) {
    $table->integer('avatar')->change();
});

@lcobucci
Copy link
Member

@nicolasbuch IMHO the problem is that the tool you're using is sending wrong information to DBAL. That should be fixed by such tool.

@cdrum
Copy link

cdrum commented Nov 26, 2019

Same issue with us - Laravel 5.8 & mysql 5.7 and we have a migration from months ago changing a column type to JSON. On DBAL v2.9.2, it was working fine, but upgrading to 2.10, breaks at the loading of this migration (and during phpunit runs since the entire DB is reconstructed)

@Ocramius
Copy link
Member

Taking feedback from:

$table->addColumn('foo', 'string')->setPlatformOption('charset', 'ascii'); should also be reverted when changing column type (by downstream tooling) by calling ->setPlatformOption('charset', null);.

Closing here as invalid, since, while it is indeed true that DBAL is creating INT UNSIGNED CHARACTER SET utf8mb4, it is merely following instructions about the given Column definition.

@Ocramius Ocramius self-assigned this Nov 26, 2019
@Ocramius Ocramius removed this from the 2.10.1 milestone Nov 26, 2019
@smartman
Copy link

I dont think this issue is invalid.

I have Laravel migration from text to json. It works perfectly in 2.9.3 and 2.10.0 fails. This means that I am stuck with older dbal version.

Schema::table('products', function (Blueprint $table) {
            $table->json("name")->change();            
        });

Doctrine\DBAL\Driver\PDOException: SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'CHARACTER SET utf8mb4 NOT NULL, CHANGE name name JSON CHARACTER SET ut' at line 1

@mfn
Copy link

mfn commented Nov 28, 2019

Laravel […] I am stuck with older dbal version

Technically, not really. You can always use unprepared to work this around if you don't want to get stuck.

@lcobucci
Copy link
Member

lcobucci commented Nov 28, 2019

Folks, Laravel is using DBAL in the wrong way and relying on a bug. Once the bug got fixed this problem started to happen, hence it being invalid.

I really understand your frustration but the message we're trying to convey here is that it's Laravel's responsibility to generate the correct object to be sent to DBAL.

It seems like https://github.com/laravel/framework/blob/1bbe5528568555d597582fdbec73e31f8a818dbc/src/Illuminate/Database/Schema/Grammars/ChangeColumn.php#L125 is the culprit.

So, please send a PR there to solve this once and for all 😁

@JohnyProkie
Copy link

I'm just going to happily link these two threads together, so both sides are informed and we can move further.
laravel/framework#30539

Sorry I'm not going to contribute directly by merge request, I'm afraid this would result into a terrible mess.

Cheers and thank you!

@vipul-vinsol
Copy link

I changed my version to 2.9.2 but still getting this issue while changing from string to unsignedBigInteger. Can someone please help or explain how can I fix this issue

@NikoGrano
Copy link

doctrine/dbal v2.10.0
Symfony 4.4

'CREATE TABLE security_users (id INT AUTO_INCREMENT NOT NULL, username VARCHAR(255) CHARACTER SET utf8 NOT NULL COLLATE `utf8_unicode_ci`, email VARCHAR(255) CHARACTER SET utf8 NOT NULL COLLATE `utf8_unicode_ci`, password VARCHAR(255) CHARACTER SET utf8 DEFAULT \'NULL\' COLLATE `utf8_unicode_ci`, salt VARCHAR(255) CHARACTER SET utf8 DEFAULT \'NULL\' COLLATE `utf8_unicode_ci`, roles JSON CHARACTER SET utf8mb4 NOT NULL COLLATE `utf8mb4_bin` COMMENT \'(DC2Type:json_array)\', PRIMARY KEY(id)) DEFAULT CHARACTER SET utf8 COLLATE `utf8_unicode_ci` ENGINE = InnoDB COMMENT = \'\' '

Generated with doctrine:migrations:dump-schema. Dies with error when running.

SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 'CHARACTER SET utf8mb4 NOT NULL COLLATE `utf8mb4_bin` COMMENT '(DC2Type:json_arra' at line 1

@stevenbuehner
Copy link

I solved my errors in the migration files adding ->charset(null).

My new line now looks like this: $table->json('column_name')->charset(null)->nullable()->change();. Finally the errors are gone 😌

@iraklisg
Copy link

iraklisg commented Feb 5, 2020

I do confirm the issue still exists in Laravel 6.x and that is pretty confusing 😕 :

When I was trying to refresh my migrations I got stuck with the following error:

SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'CHARACTER SET utf8 NOT NULL COLLAT
E utf8_unicode_ci' at line 1 (SQL: ALTER TABLE foo CHANGE foo_type verifiable_type INT UNSIGNED CHARACTER SET utf8 NOT NULL COLLATE utf8_unicode_ci)

which was caused by the following piece of code:

    public function down()
    {
        Schema::connection(env('DB_CONNECTION_FOO'))
            ->table('foo', function (Blueprint $table) {
                $table->unsignedInteger('foo_type')->change();
            });
    }

Previously I did not have any issues and I haven't made any changes to migrations since the last time they successfully run

The workaround suggested by @stevenbuehner ->charset(null) solved the problem and I was able to run my migrations again

@anususmi
Copy link

anususmi commented Mar 13, 2020

$table->json('column_name')->charset(null)->nullable()->change();
is not working for me...!!!
I want to add nullable to already existing column $table->string('phone_number');
My laravel version 5.7 and doctrine/dbal ^2.10
Please help me..!!
Thanks in advance :-)

@sheixt
Copy link

sheixt commented Apr 29, 2020

I'm in the same situation as @anususmi (Laravel 5.7.22 & doctrine/dbal ^2.10) and the issue is still present even with the ->charset(null) workaround 😢

Guessing the only way to resolve this is to downgrade?

@hbjydev
Copy link

hbjydev commented May 4, 2020

Can I get a bump on this? Using it for work and we really don't wanna be on an outdated version of this library

@willpower232
Copy link

FYI this appears to have been resolved in laravel 6 and 7
laravel/framework#30539 (comment)

@strtz
Copy link

strtz commented Jun 10, 2020

still present in:
"php": "^7.2",
"laravel/framework": "^6.0",
"doctrine/dbal": "^2.10",

error while trying to change column from text to blob

workaround for me was to make a intermediary migration that runs above the alter and drop the column and recreate it for both fwd and reverse migration

@lcobucci
Copy link
Member

@strtz would you please report that to Laravel?

@strtz
Copy link

strtz commented Jun 10, 2020

@strtz would you please report that to Laravel?

laravel/framework#30539 added, just leaving info in both places in case someone is looking for workaround that do not involve raw statements

@sarahman
Copy link

I solved my errors in the migration files adding ->charset(null).

My new line now looks like this: $table->json('column_name')->charset(null)->nullable()->change();. Finally the errors are gone

By using ->charset(null), my error is not solved; but after using ->charset('')->collation(''), the error is gone. For example:
$table->integer('resource_type')->default(0)->charset('')->collation('')->change();

@jpswade
Copy link

jpswade commented Jul 7, 2021

I think dateTime is missing from that list.

@MichaelBelgium
Copy link

This still occurs in v2.13.x in doctrine migrations.

public function up(Schema $schema) : void
    {
        $table = $schema->getTable(self::TABLE);
        $column = $table->getColumn(self::COLUMN);
        $column->setType(Type::getType(Types::JSON));
    }
An exception occurred while executing 'ALTER TABLE tbl_logs CHANGE action_data action_data JSON CHARACTER SET utf8 DEFAULT NULL COLLATE `utf8_general_ci`':

SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'CHARACTER SET utf8 DEFAULT NULL COLLATE `utf8_general_ci`' at line 1

@github-actions
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests