-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
base: 4.1.x
Are you sure you want to change the base?
Enum support #6347
Changes from all commits
20f952c
bd1688c
ccdb800
1a7fa0e
420239f
c48fdd9
29f5dcb
4e06b15
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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'; | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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), | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||||||||||||
), | ||||||||||||
); | ||||||||||||
|
||||||||||||
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), | ||||||||||||
default => sprintf('ENUM(%s)', $values), | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
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)) { | ||||||||||||
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]); | ||||||||||||
} | ||||||||||||
|
||||||||||||
if ($value instanceof BackedEnum) { | ||||||||||||
return (string) $value->value; | ||||||||||||
} | ||||||||||||
|
||||||||||||
return $value->name; | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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]); | ||||||||||||
} | ||||||||||||
|
||||||||||||
if (! $value instanceof Stringable) { | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Codecov / codecov/patchsrc/Types/EnumType.php#L105
|
||||||||||||
} | ||||||||||||
} | ||||||||||||
|
||||||||||||
return (string) $value; | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)) { | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Codecov / codecov/patchsrc/Types/EnumType.php#L119
|
||||||||||||
} | ||||||||||||
|
||||||||||||
if ($this->enumClassname === null) { | ||||||||||||
if (! in_array($value, $this->members)) { | ||||||||||||
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()); | ||||||||||||
} | ||||||||||||
|
||||||||||||
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); | ||||||||||||
} | ||||||||||||
} | ||||||||||||
|
||||||||||||
throw new ConversionException(sprintf('Class %s does not exists', $this->enumClassname)); | ||||||||||||
} | ||||||||||||
|
||||||||||||
private function getInternalDocrineType(): string | ||||||||||||
{ | ||||||||||||
if ($this->enumClassname) { | ||||||||||||
Check failure on line 155 in src/Types/EnumType.php GitHub Actions / Static Analysis with PHPStan (8.3)
Check failure on line 155 in src/Types/EnumType.php GitHub Actions / Static Analysis with Psalm (8.3)RiskyTruthyFalsyComparison
|
||||||||||||
return sprintf('%s(%s)', $this->name, $this->enumClassname); | ||||||||||||
} | ||||||||||||
|
||||||||||||
return sprintf('%s(%s)', $this->name, 'string'); | ||||||||||||
} | ||||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 GitHub Actions / Coding Standards / Coding Standards (8.3)
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,7 @@ | |
use Doctrine\DBAL\Platforms\AbstractPlatform; | ||
use Doctrine\DBAL\Platforms\MariaDBPlatform; | ||
use Doctrine\DBAL\Platforms\MySQL80Platform; | ||
use Doctrine\DBAL\Platforms\MySQLPlatform; | ||
use Doctrine\DBAL\Schema\AbstractSchemaManager; | ||
use Doctrine\DBAL\Schema\Column; | ||
use Doctrine\DBAL\Schema\Comparator; | ||
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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} | ||
* | ||
|
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); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
preg_match_all()
returnsfalse
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.