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
Conversation
Thank you for your contribution. However, we don't merge any changes that aren't covered by tests. |
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.
Your feature is not tested against any database. Please have a look at our existing functional tests and add some that cover your feature.
UPDATE: It maps to string by default. Provided functional test in |
|
What are unhappy cases? |
Sorry but it don't help me. I know what is Happy path. Also where I can found similar Schema introspection and comparison tests so I can see and write the missing cases |
All tests in |
bb32bc9
to
63b812a
Compare
@derrabus Wrote Functional tests. You can review. |
It seems that CHANCH/ALTER COLUMN functionality is not supported on SQLite. |
That is correct, see https://www.sqlite.org/lang_altertable.html |
Also there is not better way to inspect CHECK constraints on SQLite Should we drop support for SQLite platform or fallback to TEXT type without CHECK constraint? |
If this cannot be implemented on all platforms, then I don't think we should support enums at all, sorry. |
@@ -211,6 +212,10 @@ protected function _getPortableTableColumnDefinition(array $tableColumn): Column | |||
'autoincrement' => str_contains($tableColumn['extra'], 'auto_increment'), | |||
]; | |||
|
|||
if ($dbType === 'enum' && preg_match_all("/'([^']+)'/", $tableColumn['type'], $members) !== false) { |
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()
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.
{ | ||
public string $name = 'enum'; | ||
|
||
public ?string $enumClassname = null; |
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.
Should this be a constructor parameter? Is the class even usable without a class name? Why is this public?
|
||
final class EnumType extends Type | ||
{ | ||
public string $name = 'enum'; |
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.
What's the purpose of this property? Types are usually unaware of the name(s) under which they've been registered.
public function getMappedDatabaseTypes(AbstractPlatform $platform): array | ||
{ | ||
return [$this->name]; | ||
} |
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.
Doesn't the database type that we map to depend on the platform?
$values = implode( | ||
', ', | ||
array_map( | ||
static fn (string $value) => sprintf('\'%s\'', $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.
The platform should know better how to properly quote and escape a string.
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 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?
} | ||
} | ||
|
||
return (string) $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.
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.
return null; | ||
} | ||
|
||
if (! is_string($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.
So, we don't support integer-backed enums?
$schemaManager = $connection->createSchemaManager(); | ||
|
||
$diff = self::diffFromActualToDesiredTable($schemaManager, $comparator, $table); | ||
var_dump($diff->getModifiedColumns()[0]); |
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.
Forgot your debug code?
$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 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.
Dropping support for SQLite is not an option. We must make sure, our type is usable on all platforms in some shape or form. |
Please have a look at the errors reported by PHPCS and PHPStan. You can run those tools locally. |
I'm migrating a project to DBAL 4.
This is initial support for native ENUM type.
A custom mapping type is used in the ORM side.
Summary
This change allows to track ENUM members in order to be able to generate right migration.
If this is welcome and you like it, I can continue to work on this feature adding some tests