Add SQL Enum support #179

michnovka · 2022-04-09T13:53:59Z

Backport the feature originally present in 1.x for native ENUM

michnovka · 2022-04-09T14:01:23Z

I have made some changes to .gitignore, to ignore .idea files (I assume I cant be the only one working in PHPStorm) as its standard used by both Symfony and Doctrine.

I have also included phpunit and psalm dependencies as dev, because well we have lot of @psalm annotations, and then because we actually do have tests for this bundle, so it makes no sense not to have phpunit included.

Speaking of tests, I would appreciate some help on writing them, as I am still a bit lost as to how this bundle is structured. We can start by writing down list of tests that need to be implemented and move on from there:

Create schema with ENUM type
Check retrieval and cast of values from SQL DB ENUM type into PHP representation
Check saving of values from PHP into SQL DB
Check configuration option doctrine.type.enum_sql_declaration works as expected (enabled ENUM SQLs by default)
Check configuration option doctrine.type.types.type = enum works as expected

Possible considerations/discussion:

I have made the option to use INT ENUMs when its BackedEnum of int type. I.e. to have ENUM(1,2,3) instead of ENUM('1','2','3'). Yet if its FlagBag, I keep the INT type in DB. Is this desired? For that reason I had to change Elao\Enum\Bridge\Doctrine\DBAL\Types\AbstractEnumType::isIntBackedEnum from private to protected.
Do we want ODM support too? I dont use ODM in my life, so I have literally 0 experience with it.
Are we happy with the type name SINGLE ? Maybe using PRIMITIVE or SIMPLE has more sense?
Before we make the final release of 2.x maybe it would be interesting to consider removing all const values used inside the bundle into their own Enums? After all this is an enum package :) e.g. Elao\Enum\Bridge\Doctrine\DBAL\Types\TypesDumper::TYPE_SINGLE could be made into an Enum type. Checked the code for consts and TypeDumper seems to be the only place, so I could make that change as part of this PR

ogizanagi · 2022-04-09T15:08:34Z

I have made some changes to .gitignore, to ignore .idea files (I assume I cant be the only one working in PHPStorm) as its standard used by both Symfony and Doctrine.

You're not the only one 😉 But the good practice actually is to register these specific files in a global .gitignore file in your user's home directory, not the project one. See https://gist.github.com/subfuzion/db7f57fff2fb6998a16c for instance.

I have also included phpunit and psalm dependencies as dev, because well we have lot of https://github.com/psalm annotations, and then because we actually do have tests for this bundle, so it makes no sense not to have phpunit included.

Please revert these changes, since these are not necessary. We already have PHPUnit installed through the Symfony phpunit-bridge. Use vendor/bin/simple-phpunit to execute the tests.
If you have make and Symfony CLI installed on your machine, you can run make test. See the Makefile

About Psalm, the annotations were added for using psalm/phpstan on userland code, we do not have any CI task enforcing it yet ; it should be added in another PR along with proper config & CI.

About tests, some existing resources:

Create schema with ENUM type

The integration tests are using a Symfony app to configure some enum using the bundle and the ORM config:

https://github.com/Elao/PhpEnums/blob/2.x/tests/Fixtures/Integration/Symfony/config/config.yml#L19-L33

Then, an integration test case class using the Symfony Kernel make some assertions by consuming the app:

https://github.com/Elao/PhpEnums/blob/2.x/tests/Integration/Bridge/Doctrine/DBAL/Type/EnumTypeTest.php

There are also unit tests for this and the following points for:

Check retrieval and cast of values from SQL DB ENUM type into PHP representation

See this integration test and unit test

Check saving of values from PHP into SQL DB

See this integration test and unit test

Check configuration option doctrine.type.enum_sql_declaration works as expected (enabled ENUM SQLs by default)

Such tests are made with unit tests:

Check configuration option doctrine.type.types.type = enum works as expected

this should be tested in both unit tests case mentioned below (config & extension).

I have made the option to use INT ENUMs when its BackedEnum of int type. I.e. to have ENUM(1,2,3) instead of ENUM('1','2','3'). Yet if its FlagBag, I keep the INT type in DB. Is this desired? For that reason I had to change Elao\Enum\Bridge\Doctrine\DBAL\Types\AbstractEnumType::isIntBackedEnum from private to protected.

I think sql enum should preferably be stored as string. So let's always use ENUM('1','2','3').

About FlagBag, you should not worry about it in this PR, since it would be a dedicated DBAL type storing the combination of multiple enum cases into 1 INT column. So you cannot store it using an ENUM SQL type.

Do we want ODM support too? I dont use ODM in my life, so I have literally 0 experience with it.

ODM is already supported since #176. There is no change to made in tis PR regarding a "native" enum type in MongoDB.

Are we happy with the type name SINGLE ? Maybe using PRIMITIVE or SIMPLE has more sense?

I'd go with SCALAR

Before we make the final release of 2.x maybe it would be interesting to consider removing all const values used inside the bundle into their own Enums? After all this is an enum package :) e.g. Elao\Enum\Bridge\Doctrine\DBAL\Types\TypesDumper::TYPE_SINGLE could be made into an Enum type. Checked the code for consts and TypeDumper seems to be the only place, so I could make that change as part of this PR

That's a bit out-of-scope, so I'd say please avoid doing this in this PR 😄 Anyway, keep in mind not every const values make sense as enums. Regarding the TypesDumper ones, it doesn't bring much value, mostly because it's internal as well as using enums instead might complexify a bit the Symfony DI config handling which does not support backed enums out-of-the-box.

…g ones

michnovka · 2022-04-09T15:25:46Z

Good to know about that global .gitignore, I learnt something new.

I have removed the newly added dependencies, except for doctrine-odm-bundle. This is vital for phpunit tests of ODM. If I dont add it, I always get errors when running tests. Psalm and phpunit gone.

I have removed int ENUMs, so they are always strings.

Did not yet rename SINGLE to SCALAR, I will do so once we agree on it globally. Or does your ring rule them all @ogizanagi ? :)

Will have a look at the remaining points and mainly tests later.

ogizanagi · 2022-04-09T15:33:01Z

have removed the newly added dependencies, except for doctrine-odm-bundle. This is vital for phpunit tests of ODM

That's unexpected : the test cases should be skipped when the bundle is not installed 🤔 :

PhpEnums/tests/Integration/Bridge/Doctrine/ODM/Type/EnumTypeTest.php

Lines 28 to 30 in 49a73e0

    
           if (!class_exists(DoctrineMongoDBBundle::class)) { 
        
               self::markTestSkipped('Doctrine MongoDB ODM bundle not installed'); 
        
           }

Did not yet rename SINGLE to SCALAR, I will do so once we agree on it globally.

Let's go with SCALAR for DBAL types yes (since we'll also have enum and later on: flagbag, json_collection, csv_collection). For ODM ones, we'll keep SINGLE one as opposed to COLLECTION, as already implemented and since there should not be any other value.

michnovka · 2022-04-09T15:39:57Z

That's unexpected : the test cases should be skipped when the bundle is not installed thinking

Ah, I didnt realize its test skip not an error. So this is desired? I think that if we have tests for that part of bridge, then why not require it as dev? Anyways removing since I am not touching ODM so it should not be part of this PR

ogizanagi · 2022-04-09T15:52:40Z

That's unexpected : the test cases should be skipped when the bundle is not installed thinking

Ah, I didnt realize its test skip not an error. So this is desired? I think that if we have tests for that part of bridge, then why not require it as dev? Anyways removing since I am not touching ODM so it should not be part of this PR

The reasoning here simply is testing ODM requires an advanced setup on your machine (php mongodb extension, a mongodb instance running, …), so we won't impose it be default and simply skip the related test cases when the ODM bundle is not installed (it's installed on CI in the github actions, so any PR is fully tested).

michnovka · 2022-04-10T10:09:11Z

I came across one "issue" or better said property of current TypesDumper.

I tried this config:

elao_enum:
    doctrine:
        types:
            App\Enum\Suit:
                default: !php/const App\Enum\Suit::Spades
            suit_sql_enum:
                class: App\Enum\Suit
                type: enum

So I wanted to have an alias for the same Enum. This resulted in this code by TypesDumper

<?php

namespace ELAO_ENUM_DT_DBAL\App\Enum {

    if (!\class_exists(SuitType::class)) {
        class SuitType extends \Elao\Enum\Bridge\Doctrine\DBAL\Types\AbstractEnumType
        {
            protected function getEnumClass(): string
            {
                return \App\Enum\Suit::class;
            }

            protected function onNullFromDatabase(): ?\BackedEnum
            {
                return \App\Enum\Suit::from('S');
            }

            protected function onNullFromPhp(): int|string|null
            {
                return 'S';
            }
        }
    }

    if (!\class_exists(SuitType::class)) {
        class SuitType extends \Elao\Enum\Bridge\Doctrine\DBAL\Types\AbstractEnumSQLDeclarationType
        {
            protected function getEnumClass(): string
            {
                return \App\Enum\Suit::class;
            }

            public function getName(): string
            {
                return 'suit_sql_enum';
            }
        }
    }

}

So the issue is clear. The name for the types is based on the Enum class name. I think it should be based instead on the type name as specified in config and slugged. Opinions?

ogizanagi · 2022-04-10T10:20:40Z

You're right indeed, using the PascalCased type name might be better for such edge-cases.
Just duplicate the enum class for your test-case if you need it for now, in case you prefer not handling with this in this PR right now. We'll fix it in a dedicated one later.

michnovka · 2022-04-10T16:11:51Z

@ogizanagi Running into issues with tests with SQLite... It has no ENUM type. What do you suggest? I have checked the original test in 1.x and there seems to be no testing of actual SQLEnum type functionality

michnovka · 2022-04-10T16:47:41Z

There are further issues with this. Because the test requires use of SchemaManager to create the DB, I am getting the following errors when making the tests:

Doctrine\DBAL\Exception : Unknown database type enum requested, Doctrine\DBAL\Platforms\MariaDb1027Platform may not support it.
 /home/superuser/scripts/PhpEnums/vendor/doctrine/dbal/src/Platforms/AbstractPlatform.php:396
 /home/superuser/scripts/PhpEnums/vendor/doctrine/dbal/src/Schema/MySQLSchemaManager.php:137
 /home/superuser/scripts/PhpEnums/vendor/doctrine/dbal/src/Schema/AbstractSchemaManager.php:981
 /home/superuser/scripts/PhpEnums/vendor/doctrine/dbal/src/Schema/AbstractSchemaManager.php:208
 /home/superuser/scripts/PhpEnums/vendor/doctrine/dbal/src/Schema/AbstractSchemaManager.php:321
 /home/superuser/scripts/PhpEnums/vendor/doctrine/dbal/src/Schema/MySQLSchemaManager.php:336
 /home/superuser/scripts/PhpEnums/vendor/doctrine/dbal/src/Schema/AbstractSchemaManager.php:306
 /home/superuser/scripts/PhpEnums/vendor/doctrine/dbal/src/Schema/AbstractSchemaManager.php:1229
 /home/superuser/scripts/PhpEnums/vendor/doctrine/orm/lib/Doctrine/ORM/Tools/SchemaTool.php:846
 /home/superuser/scripts/PhpEnums/vendor/doctrine/orm/lib/Doctrine/ORM/Tools/SchemaTool.php:831
 /home/superuser/scripts/PhpEnums/tests/Integration/Bridge/Doctrine/DBAL/Type/EnumTypeTest.php:33

I think this is the reason why we didnt have tests in 1.x. I am not sure what we can do fix this, any ideas?

…dded tests

michnovka · 2022-04-10T17:12:30Z

I have pushed the changes so you can have a look too. But I dont think I can resolve this. The original version also did not have tests and I believe the main issue is due to SQLite lack of support. However even if we use locally MySQL, I have the problems with SchemaManager trying to map enum type, which is not supported by the platform officially. I do not have this issue when using migrations / working with ORM only, so I assume it has to do with the SchemaManager itself. But please, a review is needed.

In the meantime I have made the SINGLE -> SCALAR change. I have also changed how the TypesDumper creates names - if no custom name is specified then nothing changes. But if custom name is used in config, then its pascal-cased and used instead of the base name of the enum

The tests I included in this PR are marked as skipped for the above mentioned issues

michnovka · 2022-04-13T09:24:28Z

@ogizanagi Can we merge this?

michnovka · 2022-04-13T09:57:56Z

@ogizanagi fixed lint errors, please re-run tests

src/Bridge/Symfony/Bundle/DependencyInjection/Configuration.php

src/Bridge/Doctrine/DBAL/Types/AbstractEnumSQLDeclarationType.php

src/Bridge/Doctrine/DBAL/Types/TypesDumper.php

src/Bridge/Doctrine/Common/AbstractTypesDumper.php

tests/Unit/Bridge/Doctrine/Common/Types/BaseTypesDumperTest.php

tests/Unit/Bridge/Symfony/Bundle/DependencyInjection/ConfigurationTest.php

tests/Unit/Bridge/Doctrine/DBAL/Types/TypesDumperTest.php

tests/Unit/Bridge/Symfony/Bundle/DependencyInjection/ElaoEnumExtensionTest.php

…ng XML and YAML declaration

michnovka · 2022-04-15T18:29:33Z

@ogizanagi I went over your comments and resolved most things. I also added some more tests which led me to find some bugs. All fixed.

I have created a MySQL test class. It can be enabled by changing phpunit.xml.dist to use MySQL instead of SQLite.

I still dont get your feedback on FlagBag inside ElaoEnumExtension::resolveDbalType(). The reason that check is there is to prevent using ENUM at any cost if FlagBag is used.

Please review once again and let me know if the way I enable MySQL tests is ok, or you wish to use some different approach. Right now I am checking $_ENV['DOCTRINE_DBAL_URL'] for pdo-mysql://

michnovka · 2022-04-15T19:58:22Z

I have a failing test on Windows due to DBAL URL I moved into phpunit.xml.dist

    <env name="DOCTRINE_DBAL_URL" value="sqlite:////%kernel.cache_dir%/db.sqlite" />

Anybody has windows who can test and tell me what it should be to work there? Thanks!

michnovka · 2022-04-15T20:00:31Z

@ogizanagi I tried to fix it, can you re-run tests please?

michnovka · 2022-04-15T20:10:29Z

That didnt work well. lets see if this version works, seems I had an extra / in the first revision.

ogizanagi · 2022-04-15T20:19:52Z

All green now ✅
Thank you @michnovka for your work and investigating the issues. I'll probably make some more minor tweaks to this PR in the next days, so I'm keeping it open for now as a reminder.

michnovka · 2022-04-15T20:21:13Z

I think it would be a good idea to set up MySQL (or MariaDB) workflow test at least for Symfony 5.4 with PHP 8.1 on Linux

ogizanagi · 2022-04-15T20:23:47Z

That's part of the plan indeed :)

michnovka · 2022-04-15T20:53:45Z

I finally understand the FlagBag issue. I can work to make FlagBag type as part of this PR, what do you think?

I also thought it might be nice to have an actual Symfony Form type of FlagBag, which would be a select with multi=true

ogizanagi · 2022-04-15T21:07:07Z

Both of these features are listed in #124 indeed.
If you'd like to work on it, that's great, but please open different PRs :)

This PR was merged into the 2.x-dev branch. Discussion ---------- Add SQL Enum support Had some mess with branches (was working on main one, not on fork branch), so after renaming it got deleted in the old PR #179 This is the same content, just different branch name is used so that I can work on other stuff before this is merged. Sorry for the mix-up, didnt know renaming branch would close existing PR, I expected itd just be renamed there too Commits ------- 8b58253 [Dev] Rework Makefile a11eda8 [Tests] Fix skipped tests in PHPUnit beforeClass hooks f35bebc [CI] Add MySQL for integration tests 371db9f Add SQL Enum support

michnovka · 2022-10-11T07:18:41Z

Well given that I need lot of code from this PR and you plan to modify it yet, I think Ill wait for the final merge

ogizanagi · 2022-10-11T07:22:28Z

@michnovka : What do you mean? #182 was merged

michnovka · 2022-10-11T07:59:43Z

strange, I sent that email on April 15th :)

michnovka added 3 commits April 9, 2022 14:19

Added .idea into gitignore

82e7cae

Added required dependencies for running PHPUnit

2c6c3f0

Added support for ENUM classes

ce6679d

michnovka mentioned this pull request Apr 9, 2022

Use actual MySQL ENUM type with Doctrine DBAL #178

Closed

Removed dev dependencies. Do not use ENUMs with int types, only strin…

0d8a402

…g ones

Revert some changes from initial commits in the PR

0512c55

Removed doctrine/mongodb-odm-bundle

1a7dc7f

michnovka force-pushed the 2.x branch from 4a952b3 to 1a7dc7f Compare April 9, 2022 15:41

michnovka added 2 commits April 10, 2022 19:00

Renamed SINGLE to SCALAR. Changed TypesDumper class name deduction. A…

071c057

…dded tests

Add comments on TypesDumper and pascalcase only when custom name is used

46c14b5

michnovka added 2 commits April 10, 2022 19:14

Fix unnecessary import

18a025d

Fix FlagBag type always SCALAR

177b019

michnovka mentioned this pull request Apr 10, 2022

Add enumFallbackValue optional parameter for PHP enum types doctrine/orm#9647

Closed

Fix lint

c7f3f55

benji07 approved these changes Apr 15, 2022

View reviewed changes

ogizanagi reviewed Apr 15, 2022

View reviewed changes

Multiple changes based on @ogizanagi suggestions

b190e91

michnovka added 5 commits April 15, 2022 18:08

Add TypesDumperTest enum case

c5ae720

Update XML schema for symfony type configuration. Updated tests

9e0dce7

Fix doctrine_types_sql_enum configuration option and add tests coveri…

3740241

…ng XML and YAML declaration

MySQL test suite

ecfd35a

Fix Doctrine deprecations fetch() and executeUpdate in tests

bd9ef4a

Fix DBAL URL for Windows

2af69b7

Fix DBAL URL path

e23e293

Fix resolveDbalType to not check for FlagBag class

2dbc796

ogizanagi linked an issue Apr 16, 2022 that may be closed by this pull request

Use actual MySQL ENUM type with Doctrine DBAL #178

Closed

michnovka deleted the branch Elao:2.x April 16, 2022 18:12

michnovka closed this Apr 16, 2022

michnovka deleted the 2.x branch April 16, 2022 18:12

michnovka restored the 2.x branch April 16, 2022 18:26

michnovka mentioned this pull request Apr 16, 2022

Add SQL Enum support #182

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SQL Enum support #179

Add SQL Enum support #179

michnovka commented Apr 9, 2022

michnovka commented Apr 9, 2022 •

edited

ogizanagi commented Apr 9, 2022 •

edited

michnovka commented Apr 9, 2022

ogizanagi commented Apr 9, 2022

michnovka commented Apr 9, 2022

ogizanagi commented Apr 9, 2022 •

edited

michnovka commented Apr 10, 2022

ogizanagi commented Apr 10, 2022

michnovka commented Apr 10, 2022 •

edited

michnovka commented Apr 10, 2022

michnovka commented Apr 10, 2022

michnovka commented Apr 13, 2022

michnovka commented Apr 13, 2022

michnovka commented Apr 15, 2022

michnovka commented Apr 15, 2022

michnovka commented Apr 15, 2022

michnovka commented Apr 15, 2022

ogizanagi commented Apr 15, 2022

michnovka commented Apr 15, 2022

ogizanagi commented Apr 15, 2022

michnovka commented Apr 15, 2022

ogizanagi commented Apr 15, 2022

michnovka commented Oct 11, 2022 via email

ogizanagi commented Oct 11, 2022

michnovka commented Oct 11, 2022

Add SQL Enum support #179

Add SQL Enum support #179

Conversation

michnovka commented Apr 9, 2022

michnovka commented Apr 9, 2022 • edited

ogizanagi commented Apr 9, 2022 • edited

michnovka commented Apr 9, 2022

ogizanagi commented Apr 9, 2022

michnovka commented Apr 9, 2022

ogizanagi commented Apr 9, 2022 • edited

michnovka commented Apr 10, 2022

ogizanagi commented Apr 10, 2022

michnovka commented Apr 10, 2022 • edited

michnovka commented Apr 10, 2022

michnovka commented Apr 10, 2022

michnovka commented Apr 13, 2022

michnovka commented Apr 13, 2022

michnovka commented Apr 15, 2022

michnovka commented Apr 15, 2022

michnovka commented Apr 15, 2022

michnovka commented Apr 15, 2022

ogizanagi commented Apr 15, 2022

michnovka commented Apr 15, 2022

ogizanagi commented Apr 15, 2022

michnovka commented Apr 15, 2022

ogizanagi commented Apr 15, 2022

michnovka commented Oct 11, 2022 via email

ogizanagi commented Oct 11, 2022

michnovka commented Oct 11, 2022

michnovka commented Apr 9, 2022 •

edited

ogizanagi commented Apr 9, 2022 •

edited

ogizanagi commented Apr 9, 2022 •

edited

michnovka commented Apr 10, 2022 •

edited