[9.x] Enhance column modifying by hafezdivandari · Pull Request #43541 · laravel/framework

hafezdivandari · 2022-08-04T02:48:08Z

Before this PR, we were manually map Laravel column types to its Doctrine equivalent and many types were missing, this PR uses each database platform own map instead. So we can support modifying many more columns, e.g. double, float, jsonb, set, timeTz, timestamp (no need to register a custom Doctrine type class), timestampTz, tinyText, unsignedDecimal, unsignedDouble, unsignedFloat, year, etc.

It also fix a bug when modifying a binary column that were being changed to LONGBLOB instead of BLOB.

Here is the Doctrine type mapping of:

driesvints · 2022-08-04T07:27:13Z

There are several breaking changes to method signatures in this PR.

hafezdivandari · 2022-08-04T12:21:25Z

@driesvints I fixed that in a new commit and reverted all changes on method signatures.

src/Illuminate/Database/Schema/Grammars/ChangeColumn.php

taylorotwell · 2022-08-16T15:57:09Z

src/Illuminate/Database/Schema/Grammars/ChangeColumn.php

        }

+        if ($fluent['type'] === 'binary') {
+            $options['length'] = AbstractMySQLPlatform::LENGTH_LIMIT_BLOB;


Why use MySQL platform? What if user is using SQL Server?

Laravel binary type maps to Doctrine blob type and only MySQL uses length option:

MySQL: https://github.com/doctrine/dbal/blob/36b9ffc15c31598baa2a37b1abdd5b860d5c164f/src/Platforms/AbstractMySQLPlatform.php#L1270-L1289

SQLServer: https://github.com/doctrine/dbal/blob/36b9ffc15c31598baa2a37b1abdd5b860d5c164f/src/Platforms/SQLServerPlatform.php#L1614-L1617

PostreSQL: https://github.com/doctrine/dbal/blob/36b9ffc15c31598baa2a37b1abdd5b860d5c164f/src/Platforms/PostgreSQLPlatform.php#L1273-L1276

SQLite: https://github.com/doctrine/dbal/blob/36b9ffc15c31598baa2a37b1abdd5b860d5c164f/src/Platforms/SqlitePlatform.php#L857-L860

taylorotwell · 2022-08-16T15:57:16Z

src/Illuminate/Database/Schema/Grammars/ChangeColumn.php

-        if (static::doesntNeedCharacterOptions($fluent['type'])) {
+        if ($fluent['type'] === 'timestamp') {
+            $options['platformOptions'] = [
+                'version' => true,


What does this do?

Laravel timestamp type maps to Doctrine datetime type:

MySQL supports timestamp if $options['platformOptions']['version'] is set:

Mapping: https://github.com/doctrine/dbal/blob/36b9ffc15c31598baa2a37b1abdd5b860d5c164f/src/Platforms/AbstractMySQLPlatform.php#L1176

Declaration: https://github.com/doctrine/dbal/blob/36b9ffc15c31598baa2a37b1abdd5b860d5c164f/src/Platforms/AbstractMySQLPlatform.php#L290-L297

Declares the version option here: https://github.com/doctrine/dbal/blob/36b9ffc15c31598baa2a37b1abdd5b860d5c164f/src/Platforms/AbstractPlatform.php#L4451

PostreSQL supports timestamp and timestampTz:

Mapping: https://github.com/doctrine/dbal/blob/36b9ffc15c31598baa2a37b1abdd5b860d5c164f/src/Platforms/PostgreSQLPlatform.php#L1179-L1180

Declaration: https://github.com/doctrine/dbal/blob/36b9ffc15c31598baa2a37b1abdd5b860d5c164f/src/Platforms/PostgreSQLPlatform.php#L1025-L1036

SQLite platform maps timestamp to datetime:

Mapping: https://github.com/doctrine/dbal/blob/36b9ffc15c31598baa2a37b1abdd5b860d5c164f/src/Platforms/SqlitePlatform.php#L778

Declaration: https://github.com/doctrine/dbal/blob/36b9ffc15c31598baa2a37b1abdd5b860d5c164f/src/Platforms/SqlitePlatform.php#L319-L322

SQLServer has no mapping for timestamp (doesn't support)

What does "version" = true even mean though?

They use this option on MySQL and DB2 only on datetime type. That's the only way to convert datetime to timestamp. but I don't know why they call it "version" if that's the question.

taylorotwell · 2022-08-16T15:57:25Z

src/Illuminate/Database/Schema/Grammars/ChangeColumn.php

+
+        if ($fluent['type'] === 'jsonb') {
+            $options['platformOptions'] = [
+                'jsonb' => true,


What does this do? Why is it needed?

Only PostgreSQL supports jsonb:

Maps jsonb to json: https://github.com/doctrine/dbal/blob/36b9ffc15c31598baa2a37b1abdd5b860d5c164f/src/Platforms/PostgreSQLPlatform.php#L1169

Uses jsonb option: https://github.com/doctrine/dbal/blob/36b9ffc15c31598baa2a37b1abdd5b860d5c164f/src/Platforms/PostgreSQLPlatform.php#L1315-L1322

taylorotwell · 2022-08-16T15:57:36Z

src/Illuminate/Database/Schema/Grammars/ChangeColumn.php

-            default => 255 + 1,
+            'tinyText' => AbstractMySQLPlatform::LENGTH_LIMIT_TINYTEXT,
+            'text' => AbstractMySQLPlatform::LENGTH_LIMIT_TEXT,
+            'mediumText' => AbstractMySQLPlatform::LENGTH_LIMIT_MEDIUMTEXT,


Why MySQL specific?

Only MySQL uses length option on text type:

MySQL: https://github.com/doctrine/dbal/blob/36b9ffc15c31598baa2a37b1abdd5b860d5c164f/src/Platforms/AbstractMySQLPlatform.php#L266-L285

PostgreSQL: https://github.com/doctrine/dbal/blob/3.4.x/src/Platforms/PostgreSQLPlatform.php#L1082-L1085

SQLServer: https://github.com/doctrine/dbal/blob/36b9ffc15c31598baa2a37b1abdd5b860d5c164f/src/Platforms/SQLServerPlatform.php#L1313-L1316

SQLite: https://github.com/doctrine/dbal/blob/36b9ffc15c31598baa2a37b1abdd5b860d5c164f/src/Platforms/SqlitePlatform.php#L514-L517

taylorotwell · 2022-08-16T15:57:54Z

Please mark as ready for review when all questions are answered. Thanks.

hafezdivandari · 2022-08-16T17:37:37Z

@taylorotwell Thank you for reviewing this PR. I answered all.

taylorotwell · 2022-09-08T14:30:33Z

Thanks for your pull request to Laravel!

Unfortunately, I'm going to delay merging this code for now. To preserve our ability to adequately maintain the framework, we need to be very careful regarding the amount of code we include.

If possible, please consider releasing your code as a package so that the community can still take advantage of your contributions!

If you feel absolutely certain that this code corrects a bug in the framework, please "@" mention me in a follow-up comment with further explanation so that GitHub will send me a notification of your response.

hafezdivandari added 4 commits August 4, 2022 06:36

map column types using database platform

1ff0e5e

support jsonb column modifying

656241c

fix modifying binary column type

cefc1c1

fix code style

24fff94

define database platform as a static property

1b038e0

add 2 more types

0293816

antonkomarev reviewed Aug 6, 2022

View reviewed changes

src/Illuminate/Database/Schema/Grammars/ChangeColumn.php Outdated Show resolved Hide resolved

better var name

9520ae1

taylorotwell reviewed Aug 16, 2022

View reviewed changes

taylorotwell marked this pull request as draft August 16, 2022 15:57

hafezdivandari marked this pull request as ready for review August 16, 2022 17:36

taylorotwell closed this Sep 8, 2022

hafezdivandari mentioned this pull request Dec 10, 2022

Unexpected type change when renaming a column #45254

Closed

Conversation

hafezdivandari commented Aug 4, 2022

Uh oh!

driesvints commented Aug 4, 2022

Uh oh!

hafezdivandari commented Aug 4, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hafezdivandari Aug 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hafezdivandari Aug 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

taylorotwell commented Aug 16, 2022

Uh oh!

hafezdivandari commented Aug 16, 2022

Uh oh!

taylorotwell commented Sep 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hafezdivandari Aug 16, 2022 •

edited

Loading

hafezdivandari Aug 16, 2022 •

edited

Loading