framework icon indicating copy to clipboard operation
framework copied to clipboard

[9.x] Enhance column modifying

Open hafezdivandari opened this issue 2 years ago • 2 comments

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:

  • MySQL: https://github.com/doctrine/dbal/blob/de42378d0e521fd45d375020472caf37a003d289/src/Platforms/AbstractMySQLPlatform.php#L1041-L1072
  • PostreSQL: https://github.com/doctrine/dbal/blob/de42378d0e521fd45d375020472caf37a003d289/src/Platforms/PostgreSQLPlatform.php#L1083-L1125
  • SQLServer: https://github.com/doctrine/dbal/blob/de42378d0e521fd45d375020472caf37a003d289/src/Platforms/SQLServerPlatform.php#L1392-L1423
  • SQLite : https://github.com/doctrine/dbal/blob/de42378d0e521fd45d375020472caf37a003d289/src/Platforms/SqlitePlatform.php#L663-L696

hafezdivandari avatar Aug 04 '22 02:08 hafezdivandari

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

driesvints avatar Aug 04 '22 07:08 driesvints

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

hafezdivandari avatar Aug 04 '22 12:08 hafezdivandari

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

taylorotwell avatar Aug 16 '22 15:08 taylorotwell

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

hafezdivandari avatar Aug 16 '22 17:08 hafezdivandari

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.

taylorotwell avatar Sep 08 '22 14:09 taylorotwell

@taylorotwell I can add some tests if it makes it easier to review this PR, as it took about a month to be closed.

As you know, many columns can't be modified on Laravel right now (leads to an error) but somehow supported by doctrine/dbal. This PR tries to fix this in minimum amount of code. I agree it may be not ideal but it's the way DBAL does this. Please let me know how you want this to be done, unless you have something else in your mind like dropping requirement of DBAL and implementing column modifying on Laravel core itself. That would be great.

hafezdivandari avatar Sep 08 '22 20:09 hafezdivandari