Elao / PhpEnums

:nut_and_bolt: Extended PHP 8.1+ enums features & specific integrations with frameworks and libraries
MIT License
324 stars 27 forks source link

Use actual MySQL ENUM type with Doctrine DBAL #178

Closed michnovka closed 2 years ago

michnovka commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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.

michnovka commented 2 years ago

@ogizanagi if you can help answer https://github.com/Elao/PhpEnums/issues/178#issuecomment-1057954105 I can try to make a working PR. Thanks

ogizanagi commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

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

michnovka commented 2 years ago

@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 commented 2 years ago

Sure 👍🏻

michnovka commented 2 years ago

Lets continue discussion on the PR https://github.com/Elao/PhpEnums/pull/179