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

Native SQL enum improvements #212

Open
michnovka opened this issue Dec 17, 2022 · 5 comments
Open

Native SQL enum improvements #212

michnovka opened this issue Dec 17, 2022 · 5 comments

Comments

@michnovka
Copy link
Contributor

michnovka commented Dec 17, 2022

I have been using this library in my project, but for native SQL enums I decided to use my own implementation, as I was missing some features:

  1. autoconfiguration of enum types without having to type them all in doctrine yaml config
  2. allow nullable field in DB, but non-nullable PHP field (so that if ENUM field in DB is NULL, it will fallback to some specific PHP enum value)
  3. allow excluding some specific enum cases from the DB ENUM declaration

My question is - do you want me to port these features into this package?

See my implementation:

AbstractEnumType.php

<?php

declare(strict_types=1);

namespace App\DBAL\Types;

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

abstract class AbstractEnumType extends Type
{
    /** @return class-string<DBALSQLEnumTypeInterface> */
    abstract public static function getEnumClass(): string;

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

    /**
     * {@inheritdoc}
     */
    public function getSQLDeclaration(array $column, AbstractPlatform $platform): string
    {
        $class = static::getEnumClass();

        if (!is_a($class, DBALSQLEnumTypeInterface::class, true)) {
            throw new LogicException(sprintf('%s must be of %s type', $class, DBALSQLEnumTypeInterface::class));
        }

        $values = [];

        foreach ($class::cases() as $val) {
            if (!$val->getExcludeFromSQLEnumDeclaration()) {
                $values[] = "'" . $val->value . "'";
            }
        }

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

    /**
     * {@inheritdoc}
     */
    public function convertToDatabaseValue($value, AbstractPlatform $platform): mixed
    {
        if ($value instanceof DBALSQLEnumTypeDefaultForNullInterface && $value === $value::getDefaultForNull()) {
            return null;
        }

        if ($value instanceof DBALSQLEnumTypeInterface) {
            if ($value->getExcludeFromSQLEnumDeclaration()) {
                throw new LogicException(sprintf('%s is not a valid value for %s in database', $value->value, get_debug_type($value)));
            }

            return $value->value;
        }

        return null;
    }

    /**
     * {@inheritdoc}
     */
    public function convertToPHPValue($value, AbstractPlatform $platform): ?BackedEnum
    {
        if (false === enum_exists(static::getEnumClass(), true)) {
            throw new LogicException('This class should be an enum');
        }

        /** @var DBALSQLEnumTypeInterface $class */
        $class = static::getEnumClass();

        if (is_a($class, DBALSQLEnumTypeDefaultForNullInterface::class, true) && $value == null) {
            /** @var DBALSQLEnumTypeDefaultForNullInterface $class */
            return $class::getDefaultForNull();
        }

        if ((!is_int($value)) && !is_string($value)) {
            return null;
        }

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

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

DBALSQLEnumTypeInterface.php

<?php

declare(strict_types=1);

namespace App\DBAL\Types;

use BackedEnum;
use Elao\Enum\ReadableEnumInterface;

interface DBALSQLEnumTypeInterface extends ReadableEnumInterface, BackedEnum
{
    public function getExcludeFromSQLEnumDeclaration(): bool;
}

DBALSQLEnumTypeDefaultForNullInterface.php

<?php

declare(strict_types=1);

namespace App\DBAL\Types;

interface DBALSQLEnumTypeDefaultForNullInterface extends DBALSQLEnumTypeInterface
{
    public static function getDefaultForNull(): self;
}

DBALSQLEnumTypeTrait.php

<?php

declare(strict_types=1);

namespace App\DBAL\Types;

use Elao\Enum\ExtrasTrait;
use Elao\Enum\ReadableEnumTrait;

trait DBALSQLEnumTypeTrait
{
    use ReadableEnumTrait;
    use ExtrasTrait;

    public function getExcludeFromSQLEnumDeclaration(): bool
    {
        return !!$this->getExtra('excludeFromSQLEnumDeclaration', false);
    }
}

an example enum showcasing all the features:

<?php

declare(strict_types=1);

namespace App\Entity\Enum;

use App\DBAL\Types\DBALSQLEnumTypeDefaultForNullInterface;
use App\DBAL\Types\DBALSQLEnumTypeTrait;
use Elao\Enum\Attribute\EnumCase;

enum ClientMembershipType: string implements DBALSQLEnumTypeDefaultForNullInterface
{
    use DBALSQLEnumTypeTrait;

    #[EnumCase('Trial')]
    case TRIAL = 'TRIAL';

    #[EnumCase('Paid')]
    case PAID = 'PAID';

    #[EnumCase('None', extras: ['excludeFromSQLEnumDeclaration' => true])]
    case NONE = 'NONE';

    public static function getDefaultForNull(): self
    {
        return self::NONE;
    }
}

the entity field then looks like this (notice how the DB value is nullable, but PHP is not, thanks to the fallback. The SQL declaration will be ENUM('PAID','TRIAL') NULL, the 'NONE' option is missing, if you assign it to your PHP instance, it will be converted to DB NULL on insert/update):

#[ORM\Column(type: ClientMembershipType::class, nullable: true)]
private ClientMembershipType $oldMembershipType;

My services.yaml tags all AbstractEnumType instances:

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

and my Kernel takes care of auto-registration:

<?php

declare(strict_types=1);

namespace App;

use App\DBAL\Types\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')) {
            /** @var array $typesDefinition */
            $typesDefinition = $container->getParameter('doctrine.dbal.connection_factory.types');
        }

        $taggedEnums = $container->findTaggedServiceIds('app.doctrine_enum_type');

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

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

at this point I still need to create this boilerplate for every enum

<?php

declare(strict_types=1);

namespace App\DBAL\Types;

use App\Entity\Enum\ClientMembershipType;

class ClientMembershipTypeType extends AbstractEnumType
{
    public static function getEnumClass(): string // the enums class to convert
    {
        return ClientMembershipType::class;
    }
}

but at least I dont have to register them then in doctrine.yaml. I am thinking about how to remove this requirement, maybe auto-generate the classes inside Kernel together with the auto-registration in TypeRegister


So are you interested in this feature, or should I leave this for everybody to implement on their own?

EDIT: credits to https://debest.fr/en/blog/php-8-1-enums-doctrine-and-symfony-enumtype as I took heavy inspiration from there with the tagging

@ogizanagi
Copy link
Member

ogizanagi commented Dec 17, 2022

.1 autoconfiguration of enum types without having to type them all in doctrine yaml config
[…]
at this point I still need to create this boilerplate for every enum
but at least I dont have to register them then in doctrine.yaml

Currently, you don't have to write anything in doctrine.yaml when using the elao_enum.doctrine.types config, which generates the DBAL types and registers them for you using prependExtensionsConfig.
So I would keep this mechanism. But we could perhaps add a mechanism to autodiscover the enums in a NS at compilation time and configure them using an optional PHP 8 attribute, rather than declaring explicitly each of the enum classes in the elao_enum config.

  1. allow nullable field in DB, but non-nullable PHP field (so that if ENUM field in DB is NULL, it will fallback to some specific PHP enum value)

Isn't it covered by the current ORM implementation when using elao_enum.doctrine.types config? (see the RequestStatus example with default)

  1. allow excluding some specific enum cases from the DB ENUM declaration

This use-case looks way too much specific to me to be implemented as-is in this package. But we can think of solutions to open extending the behavior of the generated DBAL types, through config, interfaces and/or annotations.

@michnovka
Copy link
Contributor Author

But we could perhaps add a mechanism to autodiscover the enums in a NS at compilation time and configure them using an optional PHP 8 attribute, rather than declaring explicitly each of the enum classes in the elao_enum config.

Yes, thats what I want

Isn't it covered by the current ORM implementation when using elao_enum.doctrine.types config? (see the RequestStatus example with default)

But again it requires a config entry. I would very much like to be able to define this on the enum level either as an attribute, or some function.

This use-case looks way too much specific to me to be implemented as-is in this package. But we can think of solutions to open extending the behavior of the generated DBAL types, through config, interfaces and/or annotations.

The use-case is actually I think very common, as if you have a Form in Symfony, and you want a to populate a field, and have some value to represent the null DB value, then currently you cannot achieve this. So it will only be empty. See my example with membership type. I want null to represent no membership. But currently, the only way to have this nicely displayed inside Symfony form is to create a custom type and override the options.

@michnovka
Copy link
Contributor Author

michnovka commented Dec 17, 2022

so how about we make an attribute

#[Attribute(Attribute::TARGET_CLASS)]
class DoctrineConfig
{
    public function __construct(
        /** @var 'single'|'flagbag' $type */
        private string $type = 'single',
        private ?BackedEnum $default = null,
        private ?string $DBALTypeName = null,
        /** @var class-string<AbstractEnumType> $DBALTypeParentClass */
        private ?string $DBALTypeParentClass = null,
        private bool $DBALTypeAutoConfigure = true,
    ) {}
}

and in elao config we add this:

elao_enum:
    doctrine:
        scan_namespaces_for_config:
            - App\Namespace1
            - App\SomeOtherNamespace\Enums

WDYT?

Edit: Ive added the $DBALTypeParentClass which if null will default to checking elao.doctrine.enum_sql_declaration. But it allows us to further extend TypesDumper to support custom implementations of AbstractEnumType

@ogizanagi
Copy link
Member

ogizanagi commented Dec 17, 2022

The use-case is actually I think very common, as if you have a Form in Symfony, and you want a to populate a field, and have some value to represent the null DB value, then currently you cannot achieve this. So it will only be empty. See my example with membership type. I want null to represent no membership. But currently, the only way to have this nicely displayed inside Symfony form is to create a custom type and override the options.

I don't get this. Why would you need an enum case to represent the null value? Use null to represent null.
There is no issue with exposing a null value in your form type as part of the choices with its own label or using the placeholder option.

Given the point 3 was about:

allow excluding some specific enum cases from the DB ENUM declaration

this looks even more specific to me, just to handle this 😅

@michnovka
Copy link
Contributor Author

this looks even more specific to me, just to handle this sweat_smile

yea, so I think we should implement sth like #212 (comment)

this will allow everybody to make their own custom logic (with custom $DBALTypeParentClass) and use auto-configuration from namespaces instead of explicit doctrine type listing in yaml config.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants