Use enum values from enumType in DiscriminatorColumn and check DiscriminatorMap values against it#12065
Use enum values from enumType in DiscriminatorColumn and check DiscriminatorMap values against it#12065greg0ire merged 1 commit intodoctrine:3.6.xfrom whataboutpereira:fix-enum-discriminator-column
Conversation
|
What's the correct way to turn off tests for DBAL versions not supporting enumType? |
|
You could mark the test as skipped unless |
|
Let's try this now. |
|
MySQL also required. :) |
|
Sorry, that was depressing. Last attempt for now. |
|
Retargeting to 3.6.x since this is a new feature. |
| } | ||
|
|
||
| if (! $this->getTestEntityManager()->getConnection()->getDatabasePlatform() instanceof AbstractMySQLPlatform) { | ||
| self::markTestSkipped('Test valid for MySQL/MariaDB only.'); |
There was a problem hiding this comment.
Why shouldn't we run this test on other DBMS?
There was a problem hiding this comment.
Tests were failing on SQLite. I'm not sure which other feature detection we should use to skip.
There was a problem hiding this comment.
The more I look at it it the more confused I get. :) Now I'm thinking Types::ENUM doesn't need to be involved at all when setting the values in ClassMetadata, because you should still get the values from the enum class even if not using type enum in the database. But it's 2AM, will test tomorrow.
|
|
|
Things got a bit clearer. It seems I was mislead by the title "Implement an EnumType for MySQL/MariaDB", where in fact it works for all databases, but just maps the value to string. So Types::ENUM should always be used to populate values. |
|
Can you please squash your commits together and give the remaining commit a good message? |
Thanks, done now. Hopefully correctly. :) |
| ); | ||
| } | ||
|
|
||
| $values = $this->discriminatorColumn->options['values'] ?? null; |
There was a problem hiding this comment.
I'm now thinking we should wrap these checks in if ($this->discriminatorColumn?->enumType !== null), otherwise DiscriminatorColumn without enumType, but with options: ['values' => ['...']] will fail. Though values probably shouldn't be used without.
|
Okay, I rebased against v3.5 which is behind the v3.6 that I should have used. Fixed again. |
Check DiscriminatorMap keys match enum cases. Test values are populated from enum cases and mismatched values throw an exception. Fixes #11794
|
Thanks @whataboutpereira ! |
Thank you as well! :) |
Fix for issue #11794.
Currently the values of enumType in DiscriminatorColumn are ignored which results in an error when generating a migration:
Fixed by populating values from the enum and also prevent invalid entries in DiscriminatorMap.