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
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
2b8e543
Add failing test demonstrating `renameColumn` error with custom Doctr…
bakerkretzmar Nov 18, 2021
8e8d45c
Use `Schema::registerCustomDoctrineType()` to register custom types a…
bakerkretzmar Nov 18, 2021
2ecc82d
Test on MySQL
bakerkretzmar Dec 10, 2021
447df71
Add duplicate test that fails the second time when using a custom column
bakerkretzmar Dec 10, 2021
d0fcae9
Always register type mappings even if the type has already been added
bakerkretzmar Dec 10, 2021
d9c75a3
Wip
bakerkretzmar Dec 10, 2021
9d957ef
Add initial failing tests for registering custom types with multiple …
bakerkretzmar Dec 10, 2021
bd3d3cb
Move custom type registration into Connection, and register all custo…
bakerkretzmar Dec 10, 2021
6f97406
Merge branch '8.x' into fix-doctrine-type-registration
bakerkretzmar Dec 10, 2021
67c9612
Changed test names to camelCase
TomHAnderson Dec 11, 2021
d181640
Merge pull request #6 from TomHAnderson/hotfix/doctrine-type-registra…
bakerkretzmar Dec 13, 2021
a3a4d0f
Update test to check column type registration on all database drivers
bakerkretzmar Dec 13, 2021
e20bedc
Wip
bakerkretzmar Dec 13, 2021
2f66eb9
Formatting
bakerkretzmar Dec 13, 2021
990830d
Wip
bakerkretzmar Dec 13, 2021
492c528
Begin work on timestamp tests
TomHAnderson Dec 13, 2021
c0dbfc4
Rename method
bakerkretzmar Dec 17, 2021
e35bfc2
Move loop / config type registration into method
bakerkretzmar Dec 17, 2021
4af54e2
Add assertions for database column type
bakerkretzmar Dec 17, 2021
00d2aaf
Merge pull request #5 from bakerkretzmar/fix-doctrine-type-registration
bakerkretzmar Dec 18, 2021
5c7932d
Merge branch 'feature/custom-column-types' into feature/timestamp-typ…
bakerkretzmar Dec 18, 2021
b45d753
Update src/Illuminate/Database/DBAL/TimestampType.php
bakerkretzmar Dec 19, 2021
7e4e270
Merge pull request #9 from TomHAnderson/feature/timestamp-type-test
bakerkretzmar Dec 19, 2021
741233a
Merge branch '8.x' into feature/custom-column-types
bakerkretzmar Dec 20, 2021
c42df73
formatting
taylorotwell Dec 22, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
30 changes: 30 additions & 0 deletions src/Illuminate/Database/Connection.php
Expand Up @@ -5,6 +5,7 @@
use Closure;
use DateTimeInterface;
use Doctrine\DBAL\Connection as DoctrineConnection;
use Doctrine\DBAL\Types\Type;
use Exception;
use Illuminate\Contracts\Events\Dispatcher;
use Illuminate\Database\Events\QueryExecuted;
Expand All @@ -21,6 +22,7 @@
use LogicException;
use PDO;
use PDOStatement;
use RuntimeException;

class Connection implements ConnectionInterface
{
Expand Down Expand Up @@ -1010,6 +1012,34 @@ public function getDoctrineConnection()
return $this->doctrineConnection;
}

/**
* Register a custom Doctrine mapping type.
*
* @param string $class
* @param string $name
* @param string $type
* @return void
*
* @throws \Doctrine\DBAL\DBALException
* @throws \RuntimeException
*/
public function registerDoctrineType(string $class, string $name, string $type): void
{
if (! $this->isDoctrineAvailable()) {
throw new RuntimeException(
'Registering a custom Doctrine type requires Doctrine DBAL (doctrine/dbal).'
);
}

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

$this->getDoctrineSchemaManager()
->getDatabasePlatform()
->registerDoctrineTypeMapping($type, $name);
}

/**
* Get the current PDO connection.
*
Expand Down
4 changes: 2 additions & 2 deletions src/Illuminate/Database/DBAL/TimestampType.php
Expand Up @@ -2,10 +2,10 @@

namespace Illuminate\Database\DBAL;

use Doctrine\DBAL\DBALException;
use Doctrine\DBAL\Platforms\AbstractPlatform;
use Doctrine\DBAL\Types\PhpDateTimeMappingType;
use Doctrine\DBAL\Types\Type;
use RuntimeException;

class TimestampType extends Type implements PhpDateTimeMappingType
{
Expand Down Expand Up @@ -36,7 +36,7 @@ public function getSQLDeclaration(array $fieldDeclaration, AbstractPlatform $pla
return $this->getSQLitePlatformSQLDeclaration($fieldDeclaration);

default:
throw new DBALException('Invalid platform: '.$name);
throw new RuntimeException('Invalid platform: '.$name);
}
}

Expand Down
15 changes: 15 additions & 0 deletions src/Illuminate/Database/DatabaseManager.php
Expand Up @@ -183,9 +183,24 @@ protected function configure(Connection $connection, $type)
// the connection, which will allow us to reconnect from the connections.
$connection->setReconnector($this->reconnector);

$this->registerConfiguredDoctrineTypes($connection);

return $connection;
}

/**
* Register custom Doctrine types from the config with the connection.
*
* @param \Illuminate\Database\Connection $connection
* @return void
*/
protected function registerConfiguredDoctrineTypes(Connection $connection): void
{
foreach ($this->app['config']->get('database.dbal.types', []) as $name => $class) {
$connection->registerDoctrineType($class, $name, $name);
}
}

/**
* Prepare the read / write mode for database connection instance.
*
Expand Down
22 changes: 0 additions & 22 deletions src/Illuminate/Database/DatabaseServiceProvider.php
Expand Up @@ -2,7 +2,6 @@

namespace Illuminate\Database;

use Doctrine\DBAL\Types\Type;
use Faker\Factory as FakerFactory;
use Faker\Generator as FakerGenerator;
use Illuminate\Contracts\Queue\EntityResolver;
Expand Down Expand Up @@ -44,7 +43,6 @@ public function register()
$this->registerConnectionServices();
$this->registerEloquentFactory();
$this->registerQueueableEntityResolver();
$this->registerDoctrineTypes();
}

/**
Expand Down Expand Up @@ -108,24 +106,4 @@ protected function registerQueueableEntityResolver()
return new QueueEntityResolver;
});
}

/**
* Register custom types with the Doctrine DBAL library.
*
* @return void
*/
protected function registerDoctrineTypes()
{
if (! class_exists(Type::class)) {
return;
}

$types = $this->app['config']->get('database.dbal.types', []);

foreach ($types as $name => $class) {
if (! Type::hasType($name)) {
Type::addType($name, $class);
}
}
}
}
20 changes: 1 addition & 19 deletions src/Illuminate/Database/Schema/Builder.php
Expand Up @@ -3,11 +3,9 @@
namespace Illuminate\Database\Schema;

use Closure;
use Doctrine\DBAL\Types\Type;
use Illuminate\Database\Connection;
use InvalidArgumentException;
use LogicException;
use RuntimeException;

class Builder
{
Expand Down Expand Up @@ -392,26 +390,10 @@ protected function createBlueprint($table, Closure $callback = null)
* @param string $name
* @param string $type
* @return void
*
* @throws \Doctrine\DBAL\DBALException
* @throws \RuntimeException
*/
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);
Comment on lines 394 to +396
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.

}

/**
Expand Down
112 changes: 112 additions & 0 deletions tests/Integration/Database/ConfigureCustomDoctrineTypeTest.php
@@ -0,0 +1,112 @@
<?php

namespace Illuminate\Tests\Integration\Database\SchemaTest;

use Doctrine\DBAL\Platforms\AbstractPlatform;
use Doctrine\DBAL\Types\Type;
use Illuminate\Database\Grammar;
use Illuminate\Database\Schema\Blueprint;
use Illuminate\Support\Facades\DB;
use Illuminate\Support\Facades\Schema;
use Illuminate\Tests\Integration\Database\DatabaseTestCase;

class ConfigureCustomDoctrineTypeTest extends DatabaseTestCase
{
protected function defineEnvironment($app)
{
$app['config']['database.connections.sqlite.database'] = ':memory:';
$app['config']['database.dbal.types'] = [
'bit' => MySQLBitType::class,
'xml' => PostgresXmlType::class,
];
}

public function testRegisterCustomDoctrineTypesWithNonDefaultDatabaseConnections()
{
$this->assertTrue(
DB::connection()
->getDoctrineSchemaManager()
->getDatabasePlatform()
->hasDoctrineTypeMappingFor('xml')
);

// Custom type mappings are registered for a connection when it's created,
// this is not the default connection but it has the custom type mappings
$this->assertTrue(
DB::connection('sqlite')
->getDoctrineSchemaManager()
->getDatabasePlatform()
->hasDoctrineTypeMappingFor('xml')
);
}

public function testRenameConfiguredCustomDoctrineColumnTypeWithPostgres()
{
if ($this->driver !== 'pgsql') {
$this->markTestSkipped('Test requires a Postgres connection.');
}

Grammar::macro('typeXml', function () {
return 'xml';
});

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

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

$this->assertFalse(Schema::hasColumn('test', 'test_column'));
$this->assertTrue(Schema::hasColumn('test', 'renamed_column'));
}

public function testRenameConfiguredCustomDoctrineColumnTypeWithMysql()
{
if ($this->driver !== 'mysql') {
$this->markTestSkipped('Test requires a MySQL connection.');
}

Grammar::macro('typeBit', function () {
return 'bit';
});

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

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

$this->assertFalse(Schema::hasColumn('test', 'test_column'));
$this->assertTrue(Schema::hasColumn('test', 'renamed_column'));
}
}

class PostgresXmlType extends Type
{
public function getSQLDeclaration(array $fieldDeclaration, AbstractPlatform $platform)
{
return 'xml';
}

public function getName()
{
return 'xml';
}
}

class MySQLBitType extends Type
{
public function getSQLDeclaration(array $fieldDeclaration, AbstractPlatform $platform)
{
return 'bit';
}

public function getName()
{
return 'bit';
}
}
62 changes: 62 additions & 0 deletions tests/Integration/Database/DBAL/TimestampTypeTest.php
@@ -0,0 +1,62 @@
<?php

namespace Illuminate\Tests\Integration\Database\DBAL;

use Illuminate\Database\DBAL\TimestampType;
use Illuminate\Database\Schema\Blueprint;
use Illuminate\Support\Facades\Schema;
use Illuminate\Tests\Integration\Database\DatabaseTestCase;

class TimestampTypeTest extends DatabaseTestCase
{
protected function defineEnvironment($app)
{
$app['config']['database.dbal.types'] = [
'timestamp' => TimestampType::class,
];
}

public function testRegisterTimestampTypeOnConnection()
{
$this->assertTrue(
$this->app['db']->connection()
->getDoctrineSchemaManager()
->getDatabasePlatform()
->hasDoctrineTypeMappingFor('timestamp')
);
}

public function testChangeDatetimeColumnToTimestampColumn()
{
Schema::create('test', function (Blueprint $table) {
$table->addColumn('datetime', 'datetime_to_timestamp');
});

Schema::table('test', function (Blueprint $table) {
$table->timestamp('datetime_to_timestamp')->nullable(true)->change();
});

$this->assertTrue(Schema::hasColumn('test', 'datetime_to_timestamp'));
// Only Postgres and MySQL actually have a timestamp type
in_array($this->driver, ['pgsql', 'mysql'])
? $this->assertSame('timestamp', Schema::getColumnType('test', 'datetime_to_timestamp'))
: $this->assertSame('datetime', Schema::getColumnType('test', 'datetime_to_timestamp'));
}

public function testChangeTimestampColumnToDatetimeColumn()
{
Schema::create('test', function (Blueprint $table) {
$table->addColumn('timestamp', 'timestamp_to_datetime');
});

Schema::table('test', function (Blueprint $table) {
$table->dateTime('timestamp_to_datetime')->nullable(true)->change();
});

$this->assertTrue(Schema::hasColumn('test', 'timestamp_to_datetime'));
// Postgres only has a timestamp type
$this->driver === 'pgsql'
? $this->assertSame('timestamp', Schema::getColumnType('test', 'timestamp_to_datetime'))
: $this->assertSame('datetime', Schema::getColumnType('test', 'timestamp_to_datetime'));
}
}
22 changes: 22 additions & 0 deletions tests/Integration/Database/SchemaBuilderTest.php
Expand Up @@ -65,4 +65,26 @@ public function testRegisterCustomDoctrineType()
$this->assertArrayHasKey(TinyInteger::NAME, Type::getTypesMap());
$this->assertSame('tinyinteger', Schema::getColumnType('test', 'test_column'));
}

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'));
}
Comment on lines +68 to +89
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).

}