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

[8.x] Improve support for custom Doctrine column types #40119

Merged
merged 25 commits into from Dec 22, 2021
Merged

[8.x] Improve support for custom Doctrine column types #40119

merged 25 commits into from Dec 22, 2021

Conversation

bakerkretzmar
Copy link
Contributor

Context

This PR is my and @TomHAnderson's follow-up to several other issues and PRs that address registering custom Doctrine columns in Laravel.

Using custom Doctrine column types in a Laravel app is useful because it makes renaming and changing columns in migrations really easy. While Laravel handles creating columns itself internally, it relies on Doctrine to rename columns or make changes to them. Doctrine doesn't have built-in support for all of Laravel's column types, so in a fresh Laravel app there are actually several column types that can be created fluently in migrations but require custom SQL statements to change or rename later.

The TimestampType added by @TomHAnderson in #35591 is a great example of this—it adds platform-specific SQL statements to support handling timestamp columns on several different database drivers, which allows developers to keep calling ->timestamp() in migrations and always have it behave as expected regardless of which driver they're using. Without it, renaming or changing a timestamp column in a migration might also silently convert it to a datetime column, because that's the closest type Doctrine has built in (see #39959).

The Xml and Bit types used in this PR's tests are similar, platform-specific examples, which would make it possible to do things like $table->renameColumn('old_xml_name', 'new_xml_name') or $table->bit('bit_column')->nullable()->change().

See: #35498, #39959, #35591, #39683.

Motivation

There is existing functionality in the framework that handles most of the required setup for registering custom Doctrine types already, and this PR fills in the gaps to make this functionality more consistent.

Specifically, the Laravel documentation includes an example of registering custom column types automatically by listing them in config/database.php, but this currently doesn't work in all contexts (multiple connections, tests, etc.):

use Illuminate\Database\DBAL\TimestampType;

'dbal' => [
    'types' => [
        'timestamp' => TimestampType::class,
    ],
],

Solution

This PR ensures that all custom Doctrine database column types listed in config('database.dbal.types') are registered consistently with the framework, so that columns using those types can be created, changed, renamed, and deleted in migrations and in tests.

Changes

  • Moves the automatic registration of custom types in config files from the framework's core DatabaseServiceProvider into the DatabaseManager class at the point where a particular database connection is actually created.
  • Moves the code responsible for performing the registration of a specific custom type from the Schema Builder class into the database Connection class, because Doctrine's type "mappings" are connection-specific.

Usage

Using the database.dbal.types config array as shown above, or calling DB::connection()->registerDoctrineType() directly, should now work as expected in all contexts, and in tests.

Tests

We added tests for the existing TimestampType verifying that timestamp columns can be modified and/or changed to datetime, and vice versa, on supported database drivers.

We also added tests for completely new custom types, xml in Postgres and bit in MySQL, verifying that:

  • custom types can be registered automatically simply by adding them to the database.dbal.types config array
  • custom types can be registered on multiple database connections

Finally, we added a test to verify that custom type registration works in tests.

Note: since this feature relies on data types that are specific to certain database drivers, some of our tests are also slightly different for different databases.

bakerkretzmar and others added 24 commits November 17, 2021 23:53
…m types in Manager when connection is created
…tion-test-names

Changed test names to camelCase
Comment on lines 394 to +396
public function registerCustomDoctrineType($class, $name, $type)
{
if (! $this->connection->isDoctrineAvailable()) {
throw new RuntimeException(
'Registering a custom Doctrine type requires Doctrine DBAL (doctrine/dbal).'
);
}

if (! Type::hasType($name)) {
Type::addType($name, $class);

$this->connection
->getDoctrineSchemaManager()
->getDatabasePlatform()
->registerDoctrineTypeMapping($type, $name);
}
$this->connection->registerDoctrineType($class, $name, $type);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll submit a separate follow-up PR to remove this method in Laravel 9 since it's now redundant and users should just call DB::registerDoctrineType directly.

Comment on lines +68 to +89

public function testRegisterCustomDoctrineTypeASecondTime()
{
if ($this->driver !== 'sqlite') {
$this->markTestSkipped('Test requires a SQLite connection.');
}

Schema::registerCustomDoctrineType(TinyInteger::class, TinyInteger::NAME, 'TINYINT');

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

$blueprint = new Blueprint('test', function (Blueprint $table) {
$table->tinyInteger('test_column')->change();
});

$blueprint->build($this->getConnection(), new SQLiteGrammar);

$this->assertArrayHasKey(TinyInteger::NAME, Type::getTypesMap());
$this->assertSame('tinyinteger', Schema::getColumnType('test', 'test_column'));
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is identical to the one above it, but prior to this PR it failed, because registering custom Doctrine types only worked once during test runs (the very first time).

@tpetry
Copy link
Contributor

tpetry commented Dec 21, 2021

I am not against your pull request but wouldn't it make more sense just providing the missing doctrine types than rewriting parts of Laravel, so the user can add new doctrine types? I mean it's already possible to add custom doctrine types for migrations. I am doing this for PostgreSQL where I am adding numerous missing types [1] [2].

@bakerkretzmar
Copy link
Contributor Author

@tpetry yeah providing more of the missing types out of the box would be great! You're right it's already possible to do this, this PR just aims to make it a bit easier.

@taylorotwell taylorotwell merged commit 2bcf6e8 into laravel:8.x Dec 22, 2021
@skollro
Copy link
Contributor

skollro commented Jan 5, 2022

Using custom DBAL types via database.dbal.types leads to the following error with Laravel's parallel testing feature: Doctrine\DBAL\Driver\PDO\Exception: SQLSTATE[08006] [7] FATAL: sorry, too many clients already. Tests work as expected when I register the custom DBAL type using DB::registerDoctrineType() in the AppServiceProvider's registermethod.

@bakerkretzmar
Copy link
Contributor Author

@skollro Interesting, do you get that error immediately, like on one of the first tests, or only after a lot of tests have run? I wonder if the code we're calling inside Doctrine is creating new connections every time and if there's a way around that.... I'll look into this asap.

@skollro
Copy link
Contributor

skollro commented Jan 5, 2022

@bakerkretzmar Thanks a lot! Yes, it happens after a few tests. I also had the same thoughts and I've tracked it down to this line https://github.com/laravel/framework/blob/8.x/src/Illuminate/Database/Connection.php#L1038

@bakerkretzmar
Copy link
Contributor Author

Tests work as expected when I register the custom DBAL type using DB::registerDoctrineType() in the AppServiceProvider's register method.

@skollro can you show me exactly how you're doing this? It fails both ways for me.

I think the line you referenced is causing the tests to create two database connections when they only need one, but I'm not sure why that matters. Laravel is already creating connections and that doesn't cause any issues, so maybe they're getting cleaned up somehow and Doctrine's aren't?

@canvural
Copy link
Contributor

canvural commented Jan 6, 2022

This change broke our tests when we upgraded to 8.78.1 We were getting errors like Packets out of order. Expected 0 received 1. Packet size=23 or MySQL server has gone away.

We had dbal array in our config/database.php Removing that and adding a DB::registerDoctrineType() in AppServiceProvider instead fixed it.

So this seems like a breaking change.

@TomHAnderson
Copy link
Contributor

@canvural Before rearranging your Laravel project did you increase your packet size?

max_allowed_packet=200M

@canvural
Copy link
Contributor

canvural commented Jan 6, 2022

@canvural Before rearranging your Laravel project did you increase your packet size?

max_allowed_packet=200M

Why would I? It's working fine with 8.77

@TomHAnderson
Copy link
Contributor

TomHAnderson commented Jan 6, 2022

Why would I? It's working fine with 8.77

I ask because the error was a MySQL error. However, I don't want to discount your report to just be the fault of another service. Can you provide a unit test that replicates the issue you've reported? I'm afraid that without a test to duplicate it, I personally am going to be at a loss.

@bakerkretzmar
Copy link
Contributor Author

@canvural are you running tests in parallel? If so, do you still see those errors when you run them 'normally' (not in parallel)?

@canvural
Copy link
Contributor

canvural commented Jan 6, 2022

@canvural are you running tests in parallel? If so, do you still see those errors when you run them 'normally' (not in parallel)?

No, I don't run parallel.

@skollro
Copy link
Contributor

skollro commented Jan 7, 2022

@bakerkretzmar You're right, I've recreated the issue in a fresh Laravel project, using a PostgreSQL database. Calling DB::registerDoctrineType() in the AppServiceProvider only works for sequential test runners. Not even the test databases are created when parallel testing is used. Maybe @nunomaduro could have a look to help us here?

@canvural
Copy link
Contributor

canvural commented Jan 7, 2022

Also adding DB::registerDoctrineType call to a service provider causes Laravel to make db connections at boot. I'm trying to avoid doing any db calls in any service provider.

An application should be bootable even if there is no db connection or it has wrong configuration.

@canvural
Copy link
Contributor

canvural commented Jan 7, 2022

I created an example repo: https://github.com/canvural/doctrine-type-bug

In the master branch there is just the dbal array in config/database.php And when running tests it fails.

In fixed branch I removed the dbal array and instead added DB::registerDoctrineType to AppServiceProvider and it works.

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.

None yet

6 participants