agence-adeliom / easy-media-bundle

A Symfony Media manager bundle for EasyAdmin
MIT License
27 stars 14 forks source link

fix(form): fix reverseTransform returning mediaId instead of media ob… #12

Closed j0r1s closed 1 year ago

j0r1s commented 1 year ago

…ject

sonarcloud[bot] commented 1 year ago

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

j0r1s commented 1 year ago

I was getting this error while using the EasyMediaField :

Expected argument of type "App\Entity\EasyMedia\Media", "string" given at property path "..."

I dug into the type and found what looks like a typo, here is a PR that fixes it.

j0r1s commented 1 year ago

This fixes the EasyMediaField when used in a relation but actually breaks EasyMediaType that are encoded in JSON...

@arnaud-ritti any idea ?

j0r1s commented 1 year ago

As a workaround I've created a Form Extension that adds an option to reset and readd the correct DataTransformer depending on the case :

<?php

namespace App\Form\Extension;

use Adeliom\EasyMediaBundle\Form\EasyMediaType;
use Adeliom\EasyMediaBundle\Service\EasyMediaManager;
use App\Entity\EasyMedia\Media;
use Symfony\Component\Form\AbstractTypeExtension;
use Symfony\Component\Form\CallbackTransformer;
use Symfony\Component\Form\Exception\TransformationFailedException;
use Symfony\Component\Form\FormBuilderInterface;
use Symfony\Component\OptionsResolver\OptionsResolver;

class EasyMediaTypeExtension extends AbstractTypeExtension
{
    public function __construct(
        private readonly EasyMediaManager $manager,
    ) {

    }

    public function buildForm(FormBuilderInterface $builder, array $options)
    {
        if (true === $options['transform_entity']) {
            $builder->resetModelTransformers();
            $builder->addModelTransformer(new CallbackTransformer(
                function ($media) {
                    if (empty($media)) {
                        return '';
                    }

                    $class = $this->manager->getHelper()->getMediaClassName();
                    if (!($media instanceof $class)) {
                        $media = $this->manager->getMedia($media);
                    }

                    if (!$media instanceof Media) {
                        return '';
                    }

                    return $media->getId();
                },
                function ($mediaId) {
                    if (!$mediaId) {
                        return null;
                    }

                    $media = $this->manager->getMedia($mediaId);

                    if (!$media instanceof Media) {
                        throw new TransformationFailedException(sprintf('An media with id "%s" does not exist!', $mediaId));
                    }

                    return $media;
                }
            ));
        }
    }

    public function configureOptions(OptionsResolver $resolver)
    {
        $resolver
            ->setDefault('transform_entity', false)
        ;
    }

    public static function getExtendedTypes(): iterable
    {
        return [EasyMediaType::class];
    }
}

Then in CrudController I can :

yield EasyMediaField::new('image')->setFormTypeOption('transform_entity', true);
jdadeliom commented 1 year ago

@j0r1s thanks, I think you are right, we should add this option. We should rename it removeTransformer and set it to false by default to respect the way easy admin is doing it.