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

Enum support #6347

Open
wants to merge 8 commits into
base: 4.1.x
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
16 changes: 16 additions & 0 deletions src/Schema/Column.php
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ class Column extends AbstractAsset

protected ?int $_length = null;

/** @var string[] */
protected array $_members = [];

protected ?int $_precision = null;

protected int $_scale = 0;
Expand Down Expand Up @@ -219,6 +222,18 @@ public function setAutoincrement(bool $flag): self
return $this;
}

/** @return string[] */
public function getMembers(): array
{
return $this->_members;
}

/** @param string[] $members */
public function setMembers(array $members): void
{
$this->_members = $members;
}

public function setComment(string $comment): self
{
$this->_comment = $comment;
Expand All @@ -240,6 +255,7 @@ public function toArray(): array
'default' => $this->_default,
'notnull' => $this->_notnull,
'length' => $this->_length,
'members' => $this->_members,
'precision' => $this->_precision,
'scale' => $this->_scale,
'fixed' => $this->_fixed,
Expand Down
5 changes: 5 additions & 0 deletions src/Schema/MySQLSchemaManager.php
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
use function implode;
use function is_string;
use function preg_match;
use function preg_match_all;
use function str_contains;
use function strtok;
use function strtolower;
Expand Down Expand Up @@ -211,6 +212,10 @@
'autoincrement' => str_contains($tableColumn['extra'], 'auto_increment'),
];

if ($dbType === 'enum' && preg_match_all("/'([^']+)'/", $tableColumn['type'], $members) !== false) {
Copy link
Member

Choose a reason for hiding this comment

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

preg_match_all() returns false only if an error occurred, for instance if the regular expression is invalid. I'm pretty sure that's not the case you wanted to catch here.

$options['members'] = $members[1];

Check warning on line 216 in src/Schema/MySQLSchemaManager.php

View check run for this annotation

Codecov / codecov/patch

src/Schema/MySQLSchemaManager.php#L215-L216

Added lines #L215 - L216 were not covered by tests
}

if (isset($tableColumn['comment'])) {
$options['comment'] = $tableColumn['comment'];
}
Expand Down
161 changes: 161 additions & 0 deletions src/Types/EnumType.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,161 @@
<?php

declare(strict_types=1);

namespace Doctrine\DBAL\Types;

use BackedEnum;
use Doctrine\DBAL\Platforms\AbstractPlatform;
use Doctrine\DBAL\Platforms\PostgreSQLPlatform;
use Doctrine\DBAL\Platforms\SQLitePlatform;
use Doctrine\DBAL\Platforms\SQLServerPlatform;
use Doctrine\DBAL\Types\Exception\InvalidFormat;
use Doctrine\DBAL\Types\Exception\InvalidType;
use Doctrine\DBAL\Types\Exception\ValueNotConvertible;
use ReflectionClass;
use Stringable;
use Throwable;
use UnitEnum;

use function array_map;
use function class_exists;
use function enum_exists;
use function implode;
use function in_array;
use function is_object;
use function is_string;
use function sprintf;

final class EnumType extends Type
{
public string $name = 'enum';
Copy link
Member

Choose a reason for hiding this comment

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

What's the purpose of this property? Types are usually unaware of the name(s) under which they've been registered.


public ?string $enumClassname = null;
Copy link
Member

Choose a reason for hiding this comment

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

Should this be a constructor parameter? Is the class even usable without a class name? Why is this public?


/** @var array<int, string> */
public array $members = [];

/**
* {@inheritDoc}
*/
public function getMappedDatabaseTypes(AbstractPlatform $platform): array
{
return [$this->name];
}
Comment on lines +41 to +44
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't the database type that we map to depend on the platform?


/**
* {@inheritDoc}
*/
public function getSQLDeclaration(array $column, AbstractPlatform $platform): string
{
$values = implode(
', ',
array_map(
static fn (string $value) => sprintf('\'%s\'', $value),
Copy link
Member

Choose a reason for hiding this comment

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

The platform should know better how to properly quote and escape a string.

$column['members'] ?: $column['type']->members,

Check failure on line 55 in src/Types/EnumType.php

View workflow job for this annotation

GitHub Actions / Static Analysis with PHPStan (8.3)

Short ternary operator is not allowed. Use null coalesce operator if applicable or consider using long ternary.
),
);

return match (true) {
$platform instanceof SQLitePlatform => sprintf('TEXT CHECK(%s IN (%s))', $column['name'], $values),
$platform instanceof PostgreSQLPlatform, $platform instanceof SQLServerPlatform => sprintf('VARCHAR(255) CHECK(%s IN (%s))', $column['name'], $values),

Check warning on line 61 in src/Types/EnumType.php

View workflow job for this annotation

GitHub Actions / Coding Standards / Coding Standards (8.3)

Line exceeds 120 characters; contains 163 characters
default => sprintf('ENUM(%s)', $values),
Copy link
Member

Choose a reason for hiding this comment

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

Isn't ENUM() a MySQL-specific SQL extension? In that case, it does not look like a reasonable default.

That being said, the platform classes exist so we don't have to do this SQL dialect limbo anywhere but in those platform classes. Move this logic into the platforms, please.

};
}

/**
* {@inheritDoc}
*/
public function convertToDatabaseValue($value, AbstractPlatform $platform): ?string
Comment on lines +66 to +69
Copy link
Member

@derrabus derrabus Mar 27, 2024

Choose a reason for hiding this comment

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

Suggested change
/**
* {@inheritDoc}
*/
public function convertToDatabaseValue($value, AbstractPlatform $platform): ?string
public function convertToDatabaseValue(mixed $value, AbstractPlatform $platform): ?string

Why did you remove the parameter type?

{
if ($value === null) {
return null;
}

if ($this->enumClassname === null) {
if (! is_string($value)) {
throw InvalidType::new($value, $this->name, ['null', 'string']);
}

if (! in_array($value, $this->members)) {

Check failure on line 80 in src/Types/EnumType.php

View workflow job for this annotation

GitHub Actions / Static Analysis with PHPStan (8.3)

Call to function in_array() requires parameter #3 to be set.
throw InvalidType::new($value, $this->name, ['null', 'string']);
}

return $value;
}

if (enum_exists($this->enumClassname)) {
if (! $value instanceof UnitEnum) {
throw InvalidType::new($value, $this->name, ['null', $this->enumClassname]);

Check warning on line 89 in src/Types/EnumType.php

View check run for this annotation

Codecov / codecov/patch

src/Types/EnumType.php#L89

Added line #L89 was not covered by tests
}

if ($value instanceof BackedEnum) {
return (string) $value->value;
}

return $value->name;
Copy link
Member

Choose a reason for hiding this comment

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

If an enum is not backed, we don't know its scalar representation. No guesswork please. That means, we can only support backed enums.

}

if (is_object($value)) {
if ($value::class !== $this->enumClassname) {
throw InvalidType::new($value, $this->name, ['null', $this->enumClassname]);

Check warning on line 101 in src/Types/EnumType.php

View check run for this annotation

Codecov / codecov/patch

src/Types/EnumType.php#L101

Added line #L101 was not covered by tests
}

if (! $value instanceof Stringable) {
Copy link
Member

Choose a reason for hiding this comment

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

Why are we dealing with stringables here? I thought this is an enum type?

throw new ConversionException(sprintf('Class %s must implement Stringable interface', $this->enumClassname));

Check warning on line 105 in src/Types/EnumType.php

View check run for this annotation

Codecov / codecov/patch

src/Types/EnumType.php#L105

Added line #L105 was not covered by tests

Check warning on line 105 in src/Types/EnumType.php

View workflow job for this annotation

GitHub Actions / Coding Standards / Coding Standards (8.3)

Line exceeds 120 characters; contains 125 characters
}
}

return (string) $value;
Copy link
Member

Choose a reason for hiding this comment

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

Again, no guesswork please. This type is responsible for writing enums into the database. If the passed value is not an enum, this class shouldn't guess what to do with it.

}

public function convertToPHPValue(mixed $value, AbstractPlatform $platform): mixed
{
if ($value === null) {
return null;
}

if (! is_string($value)) {
Copy link
Member

Choose a reason for hiding this comment

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

So, we don't support integer-backed enums?

throw InvalidFormat::new($value, $this->name, 'string');

Check warning on line 119 in src/Types/EnumType.php

View check run for this annotation

Codecov / codecov/patch

src/Types/EnumType.php#L119

Added line #L119 was not covered by tests

Check failure on line 119 in src/Types/EnumType.php

View workflow job for this annotation

GitHub Actions / Static Analysis with PHPStan (8.3)

Parameter #1 $value of static method Doctrine\DBAL\Types\Exception\InvalidFormat::new() expects string, mixed given.
}

if ($this->enumClassname === null) {
if (! in_array($value, $this->members)) {

Check failure on line 123 in src/Types/EnumType.php

View workflow job for this annotation

GitHub Actions / Static Analysis with PHPStan (8.3)

Call to function in_array() requires parameter #3 to be set.
throw ValueNotConvertible::new($value, $this->name);
}

return $value;
}

if (enum_exists($this->enumClassname)) {
foreach ($this->enumClassname::cases() as $case) {
if (($case instanceof BackedEnum && $value === $case->value) || $value === $case->name) {
return $case;
}
}

throw ValueNotConvertible::new($value, $this->getInternalDocrineType());

Check warning on line 137 in src/Types/EnumType.php

View check run for this annotation

Codecov / codecov/patch

src/Types/EnumType.php#L137

Added line #L137 was not covered by tests
}

if (class_exists($this->enumClassname)) {
$refl = new ReflectionClass($this->enumClassname);

try {
return $refl->newInstance($value);
} catch (Throwable $e) {
throw ValueNotConvertible::new($value, $this->getInternalDocrineType(), $e->getMessage(), $e);

Check warning on line 146 in src/Types/EnumType.php

View check run for this annotation

Codecov / codecov/patch

src/Types/EnumType.php#L145-L146

Added lines #L145 - L146 were not covered by tests
}
}

throw new ConversionException(sprintf('Class %s does not exists', $this->enumClassname));

Check warning on line 150 in src/Types/EnumType.php

View check run for this annotation

Codecov / codecov/patch

src/Types/EnumType.php#L150

Added line #L150 was not covered by tests
}

private function getInternalDocrineType(): string

Check warning on line 153 in src/Types/EnumType.php

View check run for this annotation

Codecov / codecov/patch

src/Types/EnumType.php#L153

Added line #L153 was not covered by tests
{
if ($this->enumClassname) {

Check failure on line 155 in src/Types/EnumType.php

View workflow job for this annotation

GitHub Actions / Static Analysis with PHPStan (8.3)

Only booleans are allowed in an if condition, string|null given.

Check failure on line 155 in src/Types/EnumType.php

View workflow job for this annotation

GitHub Actions / Static Analysis with Psalm (8.3)

RiskyTruthyFalsyComparison

src/Types/EnumType.php:155:13: RiskyTruthyFalsyComparison: Operand of type null|string contains type string, which can be falsy and truthy. This can cause possibly unexpected behavior. Use strict comparison instead. (see https://psalm.dev/356)
return sprintf('%s(%s)', $this->name, $this->enumClassname);

Check warning on line 156 in src/Types/EnumType.php

View check run for this annotation

Codecov / codecov/patch

src/Types/EnumType.php#L155-L156

Added lines #L155 - L156 were not covered by tests
}

return sprintf('%s(%s)', $this->name, 'string');

Check warning on line 159 in src/Types/EnumType.php

View check run for this annotation

Codecov / codecov/patch

src/Types/EnumType.php#L159

Added line #L159 was not covered by tests
}
}
1 change: 1 addition & 0 deletions src/Types/Type.php
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ abstract class Type
Types::DATETIMETZ_MUTABLE => DateTimeTzType::class,
Types::DATETIMETZ_IMMUTABLE => DateTimeTzImmutableType::class,
Types::DECIMAL => DecimalType::class,
Types::ENUM => EnumType::class,
Types::FLOAT => FloatType::class,
Types::GUID => GuidType::class,
Types::INTEGER => IntegerType::class,
Expand Down
1 change: 1 addition & 0 deletions src/Types/Types.php
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ final class Types
public const DATETIMETZ_MUTABLE = 'datetimetz';
public const DATETIMETZ_IMMUTABLE = 'datetimetz_immutable';
public const DECIMAL = 'decimal';
public const ENUM = 'enum';
public const FLOAT = 'float';
public const GUID = 'guid';
public const INTEGER = 'integer';
Expand Down
10 changes: 10 additions & 0 deletions tests/Functional/Schema/ComparatorTestUtils.php
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,16 @@
);
}

public static function assertNoDiffDetected(Connection $connection, Comparator $comparator, Table $table): void
{
$schemaManager = $connection->createSchemaManager();

$diff = self::diffFromActualToDesiredTable($schemaManager, $comparator, $table);
var_dump($diff->getModifiedColumns()[0]);

Check failure on line 46 in tests/Functional/Schema/ComparatorTestUtils.php

View workflow job for this annotation

GitHub Actions / Coding Standards / Coding Standards (8.3)

Function var_dump() should not be referenced via a fallback global name, but via a use statement.

Check failure on line 46 in tests/Functional/Schema/ComparatorTestUtils.php

View workflow job for this annotation

GitHub Actions / Static Analysis with Psalm (8.3)

ForbiddenCode

tests/Functional/Schema/ComparatorTestUtils.php:46:9: ForbiddenCode: Unsafe var_dump (see https://psalm.dev/002)
Copy link
Member

Choose a reason for hiding this comment

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

Forgot your debug code?


TestCase::assertTrue($diff->isEmpty());
}

public static function assertDiffNotEmpty(Connection $connection, Comparator $comparator, Table $table): void
{
$schemaManager = $connection->createSchemaManager();
Expand Down
17 changes: 17 additions & 0 deletions tests/Functional/Schema/MySQL/ComparatorTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
use Doctrine\DBAL\Platforms\AbstractPlatform;
use Doctrine\DBAL\Platforms\MariaDBPlatform;
use Doctrine\DBAL\Platforms\MySQL80Platform;
use Doctrine\DBAL\Platforms\MySQLPlatform;

Check failure on line 12 in tests/Functional/Schema/MySQL/ComparatorTest.php

View workflow job for this annotation

GitHub Actions / Coding Standards / Coding Standards (8.3)

Type Doctrine\DBAL\Platforms\MySQLPlatform is not used in this file.
use Doctrine\DBAL\Schema\AbstractSchemaManager;
use Doctrine\DBAL\Schema\Column;
use Doctrine\DBAL\Schema\Comparator;
Expand Down Expand Up @@ -214,6 +215,22 @@
ComparatorTestUtils::assertDiffNotEmpty($this->connection, $this->comparator, $table);
}

public function testEnumDiffDetected(): void
{
$table = new Table('enum_test_table');

$table->addColumn('enum_col', Types::ENUM, ['members' => ['a', 'b']]);
$this->dropAndCreateTable($table);

ComparatorTestUtils::assertNoDiffDetected($this->connection, $this->comparator, $table);

// Alter column to previous state and check diff
$sql = 'ALTER TABLE enum_test_table CHANGE enum_col enum_col ENUM(\'NOT_A_MEMBER_ANYMORE\') NOT NULL';
$this->connection->executeStatement($sql);

ComparatorTestUtils::assertDiffNotEmpty($this->connection, $this->comparator, $table);
Copy link
Member

Choose a reason for hiding this comment

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

It's good that the diff is not empty, but we should actually have an opinion on what is in that diff, shouldn't we.

}

/**
* @return array{Table,Column}
*
Expand Down
16 changes: 16 additions & 0 deletions tests/Functional/Schema/Oracle/ComparatorTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -60,4 +60,20 @@ public function testChangeBinaryColumnFixed(): void
$table,
)->isEmpty());
}

public function testEnumDiffDetected(): void
{
$table = new Table('enum_test_table');

$table->addColumn('enum_col', Types::ENUM, ['members' => ['a', 'b']]);
$this->dropAndCreateTable($table);

ComparatorTestUtils::assertNoDiffDetected($this->connection, $this->comparator, $table);

// Alter column to previous state and check diff
$sql = 'ALTER TABLE enum_test_table CHANGE enum_col enum_col ENUM(\'NOT_A_MEMBER_ANYMORE\') NOT NULL';
$this->connection->executeStatement($sql);

ComparatorTestUtils::assertDiffNotEmpty($this->connection, $this->comparator, $table);
}
}
16 changes: 16 additions & 0 deletions tests/Functional/Schema/PostgreSQL/ComparatorTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -92,4 +92,20 @@ private function testColumnModification(callable $addColumn, callable $modifyCol
$table,
)->isEmpty());
}

public function testEnumDiffDetected(): void
{
$table = new Table('enum_test_table');

$table->addColumn('enum_col', Types::ENUM, ['members' => ['a', 'b']]);
$this->dropAndCreateTable($table);

ComparatorTestUtils::assertNoDiffDetected($this->connection, $this->comparator, $table);

// Alter column to previous state and check diff
$sql = 'ALTER TABLE enum_test_table CHANGE enum_col enum_col ENUM(\'NOT_A_MEMBER_ANYMORE\') NOT NULL';
$this->connection->executeStatement($sql);

ComparatorTestUtils::assertDiffNotEmpty($this->connection, $this->comparator, $table);
}
}
10 changes: 10 additions & 0 deletions tests/Functional/Schema/SQLite/ComparatorTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -42,4 +42,14 @@ public function testChangeTableCollation(): void
$column->setPlatformOption('collation', 'NOCASE');
ComparatorTestUtils::assertDiffNotEmpty($this->connection, $this->comparator, $table);
}

public function testEnumColumnIsCreated(): void
{
$table = new Table('enum_test_table');

$table->addColumn('enum_col', Types::ENUM, ['members' => ['a', 'b']]);
$this->dropAndCreateTable($table);

ComparatorTestUtils::assertNoDiffDetected($this->connection, $this->comparator, $table);
}
}
32 changes: 32 additions & 0 deletions tests/Functional/Types/EnumTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
<?php

declare(strict_types=1);

namespace Doctrine\DBAL\Tests\Functional\Types;

use Doctrine\DBAL\Schema\Table;
use Doctrine\DBAL\Tests\FunctionalTestCase;
use Doctrine\DBAL\Types\Types;

class EnumTest extends FunctionalTestCase
{
protected function setUp(): void
{
$table = new Table('enum_table');
$table->addColumn('val', Types::ENUM, ['members' => ['a', 'b']]);

$this->dropAndCreateTable($table);
}

public function testInsertAndSelect(): void
{
$val = 'b';

$result = $this->connection->insert('enum_table', ['val' => $val]);
self::assertSame(1, $result);

$value = $this->connection->fetchOne('SELECT val FROM enum_table');

self::assertEquals($val, $value);
}
}
14 changes: 14 additions & 0 deletions tests/Schema/ColumnTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ public function testToArray(): void
'default' => 'baz',
'notnull' => false,
'length' => 200,
'members' => [],
'precision' => 5,
'scale' => 2,
'fixed' => true,
Expand Down Expand Up @@ -148,4 +149,17 @@ public function testColumnComment(): void
self::assertArrayHasKey('comment', $columnArray);
self::assertEquals('foo', $columnArray['comment']);
}

public function testEnumMembers(): void
{
$column = new Column('bar', Type::getType(Types::ENUM));
self::assertSame([], $column->getMembers());

$column->setMembers(['a', 'b']);
self::assertEquals(['a', 'b'], $column->getMembers());

$columnArray = $column->toArray();
self::assertArrayHasKey('members', $columnArray);
self::assertEquals(['a', 'b'], $columnArray['members']);
}
}