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.7] Makes sure changing a database field to JSON does not include a collation #25741

Merged
merged 2 commits into from
Oct 2, 2018

Conversation

poisa
Copy link
Contributor

@poisa poisa commented Sep 22, 2018

Full issue is discussed here: #25735

@sisve
Copy link
Contributor

sisve commented Sep 23, 2018

This fix would affect all databases while the original issue is only about Mysql. Is the collation unsupported in other databases too?

@poisa
Copy link
Contributor Author

poisa commented Sep 23, 2018

@sisve The DBAL docs mention that collation is supported by MySQL, PostgreSQL, Sqlite, SQL Server and Drizzle. I've asked the maintainers if that parameter is silently ignored in other engines or if it renders invalid SQL. The former would pose no problems while the latter would force us to detect the engine and execute the conditional in my PR accordingly (which would suck big time).
I'll post back as soon as they answer.

@GrahamCampbell GrahamCampbell changed the title Makes sure changing a database field to JSON does not include a collation [5.7] Makes sure changing a database field to JSON does not include a collation Sep 23, 2018
@poisa
Copy link
Contributor Author

poisa commented Sep 25, 2018

@sisve, I just got word back from the guys over at DBAL and can confirm that unsupported database engines silently ignores a passed collation so this change on the Laravel side will not have detrimental effects on such engines.

The actual conditional can be found here: https://github.com/doctrine/dbal/blob/master/lib/Doctrine/DBAL/Platforms/AbstractPlatform.php#L2642-L2654

(the whole process starts here though: https://github.com/doctrine/dbal/blob/master/lib/Doctrine/DBAL/Platforms/AbstractPlatform.php#L2278-L2308)

@poisa
Copy link
Contributor Author

poisa commented Sep 25, 2018

As a temporary workaround I found out that by exploiting the fact that you can pass anything to a Fluent command, a migration can explicitly pass a customSchemaOptions array to DBAL like so:

$table->json('my_column')->customSchemaOptions(['collation' => ''])->change();

This would force DBAL to not use COLLATE in the generated SQL.

@jwage
Copy link

jwage commented Sep 25, 2018

My 2 cents is that this is the appropriate solution. The end user should be responsible for specifying all the necessary data required as I think it will be very hard and error prone to handle all the different possible combinations of column types and collations across the different database types.

@taylorotwell taylorotwell merged commit a13e6ca into laravel:5.7 Oct 2, 2018
@Goddard
Copy link

Goddard commented Dec 5, 2019

For whoever is wondering, I couldn't get this working with only setting the collation. I also had to change charset.

For example
$table->json('response_json')->nullable()->customSchemaOptions(['collation' => '', 'charset' => ''])->comment('changed datatype')->change();

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

Successfully merging this pull request may close these issues.

5 participants