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

Use actual MySQL ENUM type with Doctrine DBAL #178

Closed
michnovka opened this issue Mar 3, 2022 · 12 comments · Fixed by #182
Closed

Use actual MySQL ENUM type with Doctrine DBAL #178

michnovka opened this issue Mar 3, 2022 · 12 comments · Fixed by #182
Labels
Projects
Milestone

Comments

@michnovka
Copy link
Contributor

michnovka commented Mar 3, 2022

Hi, thanks so much for your package. I do not have time to actually make a PR at the moment, but I wanted to share what I did to support native MySQL ENUM types.

I have my own AbstractEnumType class:

<?php

namespace App\DBAL;

use BackedEnum;
use Doctrine\DBAL\Platforms\AbstractPlatform;
use Doctrine\DBAL\Types\Type;

abstract class AbstractEnumType extends Type
{
    abstract public static function getEnumsClass(): string;

    public function getName(): string // the name of the type.
    {
        return static::getEnumsClass();
    }

    public function getSQLDeclaration(array $fieldDeclaration, AbstractPlatform $platform): mixed
    {
        /** @var BackedEnum $class */
        $class = static::getEnumsClass();

        $values = array_map(
        /** @var BackedEnum $val */
            function ($val) {
                return "'" . $val->value . "'";
            },
            $class::cases()
        );

        return "ENUM(" . implode(", ", $values) . ")";
    }

    public function convertToDatabaseValue($value, AbstractPlatform $platform): mixed
    {
        if ($value instanceof \BackedEnum) {
            return $value->value;
        }
        return null;
    }

    public function convertToPHPValue($value, AbstractPlatform $platform): mixed
    {
        if (false === enum_exists($this->getEnumsClass(), true)) {
            throw new \LogicException("This class should be an enum");
        }

        /** @var BackedEnum $class */
        $class = static::getEnumsClass();

        return $class::tryFrom($value);
    }


    /**
     * @codeCoverageIgnore
     */
    public function requiresSQLCommentHint(AbstractPlatform $platform): bool
    {
        return true;
    }
}

When I make new enum like

<?php

namespace App\Entity\Enum;

use Elao\Enum\Attribute\EnumCase;
use Elao\Enum\ReadableEnumInterface;
use Elao\Enum\ReadableEnumTrait;

enum AdminRole: string implements ReadableEnumInterface
{
    use ReadableEnumTrait;

    #[EnumCase('Admin')]
    case ADMIN = 'ROLE_ADMIN';

    #[EnumCase('Super Admin')]
    case SUPER_ADMIN = 'ROLE_SUPER_ADMIN';

    #[EnumCase('CSR')]
    case CSR = 'ROLE_CSR';

    #[EnumCase('Manager')]
    case MANAGER = 'ROLE_MANAGER';

    #[EnumCase('Accounting')]
    case ACCOUNTING = 'ROLE_ACCOUNTING';

    #[EnumCase('SEO')]
    case SEO = 'ROLE_SEO';
}

I then make also a "wrapper" in DBAL folder:

<?php

namespace App\DBAL;

use App\Entity\Enum\AdminRole;

class AdminRoleType extends AbstractEnumType
{
    public static function getEnumsClass(): string // the enums class to convert
    {
        return AdminRole::class;
    }
}

This wrapper just returns the PHP enum which it links.

In config/services.yaml I put:

services:
   _instanceof:
        App\DBAL\AbstractEnumType:
          tags: ['app.doctrine_enum_type']

and then in Kernel.php, I register all "wrapper" classes as valid types:

<?php

namespace App;

use App\DBAL\AbstractEnumType;
use Symfony\Bundle\FrameworkBundle\Kernel\MicroKernelTrait;
use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\HttpKernel\Kernel as BaseKernel;

class Kernel extends BaseKernel implements CompilerPassInterface
{
    use MicroKernelTrait;

    public function process(ContainerBuilder $container): void
    {
        $typesDefinition = [];
        if ($container->hasParameter('doctrine.dbal.connection_factory.types')) {
            $typesDefinition = $container->getParameter('doctrine.dbal.connection_factory.types');
        }
        $taggedEnums = $container->findTaggedServiceIds('app.doctrine_enum_type');

        foreach ($taggedEnums as $enumType => $definition) {
            /** @var $enumType AbstractEnumType */
            $typesDefinition[$enumType::getEnumsClass()] = ['class' => $enumType];
        }

        $container->setParameter('doctrine.dbal.connection_factory.types', $typesDefinition);
    }

}

And thats it!. In Entity I use:

    #[ORM\Column(type: AdminRole::class)]
    private AdminRole $admin_role = AdminRole::CSR;

and the SQL generated by Doctrine migrations is:

CREATE TABLE admin (id INT AUTO_INCREMENT NOT NULL, email VARCHAR(180) NOT NULL, admin_role ENUM('ROLE_ADMIN', 'ROLE_SUPER_ADMIN', 'ROLE_CSR', 'ROLE_MANAGER', 'ROLE_ACCOUNTING', 'ROLE_SEO') NOT NULL COMMENT '(DC2Type:App\\Entity\\Enum\\AdminRole)', password VARCHAR(255) NOT NULL, token VARCHAR(255) DEFAULT NULL, UNIQUE INDEX UNIQ_880E0D76E7927C74 (email), PRIMARY KEY(id)) DEFAULT CHARACTER SET utf8mb4 COLLATE `utf8mb4_unicode_ci` ENGINE = InnoDB

Adding new native ENUM type consists of only creating the php enum, then making the wrapper class.

So I was thinking this might be interesting to extend in your own AbstractEnumType doctrine bundle, which now uses either INT or VARCHAR. And if nothing else, it might help others who want native MySQL ENUMs

Thanks for your work!

@michnovka
Copy link
Contributor Author

My idea for a PR was to make a new derived class AbstractSQLEnumType extends AbstractEnumType. The main thing I dont know how to do is how to actually tag all the instances without having to insert stuff into services.yaml:

services:
   _instanceof:
        App\DBAL\AbstractEnumType:
          tags: ['app.doctrine_enum_type']

I assume the actual part from Kernel.php could be moved into Bundle build() function.

@ogizanagi
Copy link
Member

ogizanagi commented Mar 3, 2022

Hi @michnovka. Thank you for opening this feature request and sharing your code.

This was supported in the V1 of this package, but I didn't reintroduced this feature since the sql ENUM type is controversial, lacks support for proper diffs with the Doctrine schema diff tool when adding new cases and was never truely useful to us.
See http://komlenic.com/244/8-reasons-why-mysqls-enum-data-type-is-evil

This is also the reason why the Doctrine team didn't used this type for their native support of PHP 8.1 enums.

@michnovka
Copy link
Contributor Author

Hi @ogizanagi , the 8 reasons article is from 2011. And its mostly incorrect for current versions of MySQL and MariaDB. But this is a preference choice, I like ENUMs in DB because they are easily readable and actually improve performance when used properly. There are cases where it makes really a lot of sense, such as ticket status (Open, In progress, Answered,...) or Continents. I know that this wont be implemented in Doctrine anytime soon because prejudice against them and also incompatibility with other DB engines, but I think we could extend this package to support it if anybody wants to actually use it.

@ogizanagi ogizanagi added this to the 2.x milestone Mar 3, 2022
@michnovka
Copy link
Contributor Author

@ogizanagi if you can help answer #178 (comment) I can try to make a working PR. Thanks

@ogizanagi
Copy link
Member

ogizanagi commented Mar 7, 2022

@michnovka : There is no need for derived classes nor tags for each of your enums types ; the bundle exposes a config allowing to declare the enums you'd like to store in a database and their format, then it generates and registers DBAL types for you.
The V1 had an SQL enum type and base class used by the dumper. The V2 system is similar, but does not include the SQL enum type base class yet.

@michnovka
Copy link
Contributor Author

@ogizanagi I see, my solution allowed automatic type registration with Doctrine without the need of adding every single one into config file. I prefer this, because if I extend AbstractEnumType, then I definitely want it registered anyways.

@ogizanagi
Copy link
Member

ogizanagi commented Mar 7, 2022

@michnovka : Sure, but you need to extend a class, whereas the bundle config is generating these for you 😄
But your solution is fine in userland if you prefer to extend the base class rather than letting the config generate it 👌🏻 (but unnecessary in this bundle)

@michnovka
Copy link
Contributor Author

@ogizanagi so does the v2 bundle support type: enum or not? It seems that the removal of this is a set-back as it forces v2 to use varchar type always.

@ogizanagi
Copy link
Member

Not yet, but a PR is welcomed if you really wish this in the bundle config rather than using your solution in userland.

@michnovka
Copy link
Contributor Author

@ogizanagi ok, I will have a look. Id like to have this for the 2.0 final version. Will play around and let you know by Monday if I am able to do this, sounds good?

@ogizanagi
Copy link
Member

Sure 👍🏻

@michnovka
Copy link
Contributor Author

Lets continue discussion on the PR #179

This was linked to pull requests Apr 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
v2.0
  
Awaiting triage
Development

Successfully merging a pull request may close this issue.

2 participants