symfony / ux

Symfony UX initiative: a JavaScript ecosystem for Symfony
https://ux.symfony.com/
MIT License
820 stars 297 forks source link

[LiveComponent] Incorrect handling of nullable enum types #414

Closed maartendekeizer closed 2 years ago

maartendekeizer commented 2 years ago

I created a component with 2 writeable live props, both nullable and with a BackedEnumType. When I changed a value a sync is happen as expected, but this will result in an error when one of the fields are empty

With request payload {"data":1,"newPriority":null,"newStatus":"new","_checksum":"***"} the following error occured The data is neither an integer nor a string, you should pass an integer or a string that can be parsed as an enumeration case of type App\Enum\Priority.

And with request payload {"data":1,"newPriority":"high","newStatus":"","_checksum":"***"} the following error occured "" is not a valid backing value for enum "App\Enum\Priority"

It think this is caused by the fact that the hydrator always try to denormalize and that empty strings are not converted to null values. https://github.com/symfony/ux/blob/ebe425c03d03643ab76080ee2860e019443b4d6e/src/LiveComponent/src/LiveComponentHydrator.php#L170-L175

A suggestion for the fix

            if ($method = $liveProp->hydrateMethod()) {
                // TODO: Error checking
                $value = $component->$method($dehydratedValue);
            } elseif ($property->getType() instanceof \ReflectionNamedType && !$property->getType()->isBuiltin()) {
                if ($value === '' && $property->getType()->allowsNull() === true) {
                    // value is given as empty string, the property type is not a string (isBuiltin-method)
                    // so we can assume that value is meant to be empty
                    $value = null;
                }
                if ($value !== null || $property->getType()->allowsNull() === false) {
                    // if value is null but the property does not allow null run denormalization too
                    $value = $this->normalizer->denormalize($value, $property->getType()->getName(), 'json', [self::LIVE_CONTEXT => true]);
                }
            }

Component

<?php

declare(strict_types=1);

namespace App\Twig\Component;

use App\Enum\Priority;
use App\Enum\Status;
use Symfony\Bundle\FrameworkBundle\Controller\AbstractController;
use Symfony\UX\LiveComponent\Attribute\AsLiveComponent;
use Symfony\UX\LiveComponent\Attribute\LiveAction;
use Symfony\UX\LiveComponent\Attribute\LiveProp;

#[AsLiveComponent('prio', template: 'component/status.html.twig')]
class StatusComponent extends AbstractController
{
    #[LiveProp(writable: true)]
    public ?Priority $newPriority = null;

    #[LiveProp(writable: true)]
    public ?Status $newStatus = null;

    public function __invoke(): void
    {
    }
}

Template

    <div {{ attributes }}>
        Status: <input type="text" data-model="newStatus" value="{{ this.newStatus.value|default('') }}">
        Prio: <input type="text" data-model="newPriority" value="{{ this.newPriority.value|default('') }}">
        <br>
        {{ this.newStatus.value|default('') }} / {{ this.newPriority.value|default('') }}
    </div>
kbond commented 2 years ago

Thanks for the report @maartendekeizer, #416 should fix this.

maartendekeizer commented 2 years ago

@kbond Tried #416, it works for the situation where the payload value is null, but when a empty string is submitted I still got an error Expected argument of type "?App\Enum\MyEnum", "string" given at property path "myField". I think an empty string is submitted when the user touched the input/select but did cleared the value afterwards.

kbond commented 2 years ago

Got it, I'll adjust my PR to account for this.

kbond commented 2 years ago

it works for the situation where the payload value is null, but when a empty string is submitted I still got an error

416 should now account for this. @maartendekeizer, can you confirm?

maartendekeizer commented 2 years ago

@kbond yes, thanks a lot!