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] Add the ability to register custom DBAL types in the schema builder. #28214

Merged
merged 9 commits into from Apr 17, 2019

Conversation

Projects
None yet
5 participants
@JacksonIV
Copy link
Contributor

commented Apr 14, 2019

Overview

This pull request aims to resolve issue #8840. In addition to that, it allows you to easily add custom, platform specific DBAL types to the framework in the future.

Quick Summary

When changing an existing column to tinyint, it will produce the following error:

DBALException in DBALException.php line 228:
Unknown column type "tinyinteger" requested. Any Doctrine type that you use has to be registered with \Doctrine\DBAL\Types\Type::addType()

Reproduction

In order to reproduce this error, you'll need to run two migrations. One should create a column and the other should should change the column:

// The first migration.
Schema::create('test_migration', function($table) {
    $table->char('test_column');
});
// The second migration.
Schema::table('test_migration', function($table) {
    $table->tinyInteger('test_column')->change();
});

Proposed Solution

In order to solve this problem, i created a "registerCustomDBALType" method in the "Builder" class. This way the platform specific builder can call this method to register their own custom type. In addition to that, the user can register their own type in their project by calling the "registerCustomDBALType" on the "Schema" facade.

@staudenmeir

This comment has been minimized.

Copy link
Contributor

commented Apr 14, 2019

We should also register the type on other databases, at least on SQL Server (which has a native tinyint type). Maybe even on PostgreSQL and SQLite.

Why are you overriding convertToPHPValue() and convertToDatabaseValue() without changing the implementation?

Please add a test that checks the generated SQL.

@JacksonIV

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

@staudenmeir I removed the unnecessary overrides and added the tests.

Show resolved Hide resolved src/Illuminate/Database/Schema/Builder.php Outdated
Show resolved Hide resolved src/Illuminate/Database/Schema/MySqlBuilder.php Outdated
Show resolved Hide resolved src/Illuminate/Database/Schema/MySqlBuilder.php Outdated
Show resolved Hide resolved src/Illuminate/Database/Schema/Types/TinyInteger.php Outdated
@JacksonIV

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2019

@driesvints I made the requested changes.

@taylorotwell taylorotwell merged commit 1bcad05 into laravel:5.8 Apr 17, 2019

2 checks passed

continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@JacksonIV JacksonIV deleted the JacksonIV:custom-dbal-types branch Apr 17, 2019

@SturmB

This comment has been minimized.

Copy link

commented Apr 25, 2019

Hey, is there a chance someone could explain how to use this to get the dreaded "TINYINT" issue to disappear? I'm still learning and don't know how to use registerCustomDBALType properly (like where to call it, for example).

@driesvints

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

@SturmB this was partially reverted again in #28282 because of the following issue: #28282

@JacksonIV

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2019

@SturmB @driesvints I just want to let you know that i’m still working on a solution to fix the problems with TINYINT, as well as other column types.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.