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

[5.8] Custom column collations are ignored when changing a column in a migration. #28487

Closed
jensdenies opened this issue May 11, 2019 · 4 comments
Labels

Comments

@jensdenies
Copy link

jensdenies commented May 11, 2019

  • Laravel Version: 5.8.16
  • PHP Version: 7.3
  • Database Driver & Version: MySQL 5.7

Description:

Setting a custom column collation in conjunction with the ->change() method in a migration doesn't work. The framework completely ignores the ->collation('') method, because there is not such thing as a "setCollation" option in Doctrine DBAL.

if (method_exists($column, $method = 'set'.ucfirst($option))) {

The collation can be set though by using the setCustomSchemaOption on Doctrine's column builder:

https://www.doctrine-project.org/projects/doctrine-dbal/en/2.9/reference/schema-representation.html#vendor-specific-options

Steps To Reproduce:

Create a new migration:

        Schema::create('tests', function (Blueprint $table) {
            $table->bigIncrements('id');
            $table->string('test_column');
        });

And create another one:

        Schema::table('tests', function (Blueprint $table) {
            $table->bigIncrements('id');
            $table->integer('test_column')->collation('')->change();
        });

Proposed change:

Before i create a pull request, i'd like to hear your thoughts on this:

https://github.com/JacksonIV/framework/blob/37c3a6decc9c6dc5f47dff0105b3c533bd7ac786/src/Illuminate/Database/Schema/Grammars/ChangeColumn.php#L83-L88

@driesvints
Copy link
Member

If Doctrine doesn't offers this then isn't this more of a feature request than a bug to support it? It doesn't explicitly say in the docs that we support this (although I can understand that it's a bit confusing).

@jensdenies
Copy link
Author

@driesvints
Copy link
Member

Okay, feel free to send in a PR with a fix if you have something.

@driesvints
Copy link
Member

PR merged

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

No branches or pull requests

2 participants