qossmic / rich-model-forms-bundle Goto Github PK

Right now when using the factory option together with expected_exception, all caught exceptions are transformed into form errors that are mapped to the form holding the factory option.

A simple approach to better map errors to the real cause could be the following: When an exception is caught, check each child form for the expected_exception option. If one and only one child form declares to handle the caught exception too, map the error to this very form. In all other cases, the behaviour must not change.

Currently, with the handling of value objects as implemented in #22 we do not deal with exceptions being thrown when constructing such objects. We should reuse the logic that we already use in the data mapper to convert exceptions into form errors, but we now also need to be able to let the exception handler decide which subform the error needs to be associated with.

Hello!

I'm trying to use this bundle with Symfony 5.

What do I have:
php -v:

PHP 7.3.14 (cli) (built: Jan 27 2020 21:55:23) ( NTS )
Copyright (c) 1997-2018 The PHP Group
Zend Engine v3.3.14, Copyright (c) 1998-2018 Zend Technologies
    with Zend OPcache v7.3.14, Copyright (c) 1999-2018, by Zend Technologies
    with Xdebug v2.7.2, Copyright (c) 2002-2019, by Derick Rethans

composer.json

"require": {
        "php": "^7.2.5",
        "ext-ctype": "*",
        "ext-iconv": "*",
        "easycorp/easyadmin-bundle": "^2.3",
        "myclabs/php-enum": "^1.7",
        "sensiolabs-de/rich-model-forms-bundle": "^0.4.0",
        "symfony/console": "5.0.*",
        "symfony/dotenv": "5.0.*",
        "symfony/flex": "^1.3.1",
        "symfony/form": "5.0.*",
        "symfony/framework-bundle": "5.0.*",
        "symfony/http-client": "5.0.*",
        "symfony/monolog-bundle": "^3.1",
        "symfony/orm-pack": "*",
        "symfony/security-bundle": "5.0.*",
        "symfony/twig-pack": "*",
        "symfony/validator": "5.0.*",
        "symfony/yaml": "5.0.*"
    }

I've set up a simple config:

easy_admin:
    entities:
        User:
            class: App\Entity\User

            new:
                fields:
                    - { property: name }

                form_options:
                    factory: App\Entity\User

I have an issue entering new view:

Argument 2 passed to SensioLabs\RichModelForms\ExceptionHandling\FormExceptionHandler::__construct() must be an instance of Symfony\Component\Translation\TranslatorInterface or null, instance of Symfony\Component\Translation\DataCollectorTranslator given

Seems like its related to 27d67bb

Ping me, if you would like to get a repository with reproducable example.

Thanks in advance!

Tried to rewrite example with SF5+rich-model-forms-bundle v.0.6.0 https://github.com/sensiolabs-de/rich-model-forms-demo
Trying to submit ProductType with embeddable PriceType throws an error This value should be of type App\Entity\Price..
Everything seems to be correct, Category form works well with similar settings.
Maybe I'm doing smth wrong?

ProductType.php

->add('price', PriceType::class, [
    'read_property_path' => 'getPrice',
    'write_property_path' => 'costs',
    'handle_exception' => PriceException::class,
]);

Entity/Product.php

public function getPrice(): Price
{
    return $this->price;
}
public function costs(Price $price): void
{
    $this->price = $price;
}

PriceType

class PriceType extends AbstractType
{
    public function buildForm(FormBuilderInterface $builder, array $options): void
    {
        $builder
            ->add('amount', MoneyType::class, [
                'divisor' => 100,
            ])
            ->add('tax', PercentType::class, [
                'type' => 'integer',
            ])
            ->add('currency', CurrencyType::class);
    }

    public function configureOptions(OptionsResolver $resolver): void
    {
        $resolver->setDefault('factory', Price::class);
        $resolver->setDefault('immutable', true);
    }
}

When #22 is merged, we will be able to derive required attributes by analysing the configured factories. We can reuse this information in a form type guesser that marks these subforms as required.

see #1 (review)

Hi, thank you for your work on this project.

I intend to use a form to instantiate an object via its constructor. Only one of the constructor parameters is a form field, the other is to be supplied via a form option.

I attempted to use a closure for the 'factory' option, which the documentation states should receive the form data as its argument:

$resolver->setDefaults([
    'data_class' => SomeClass::class,
    'factory' => function($formData) {
        // $formData is null
    },
]);

Unfortunately on submitting the form, $formData is null so I have no way of getting the form field's data to construct the object.

It looks as though SensioLabs\RichModelForms\Instantiator\FormDataInstantiator->getData() is always null at that point - the child form field is submitted and has data but the overall form is not and has none.

Any advice would be appreciated. I'm also not sure how I'm going to supply the factory closure with another parameter for the constructor from the form options but I'm not there yet :)

Right now, using callable for factory is like this (I removed options for brevity):

public function buildForm(FormBuilderInterface $builder, array $options): void
{
    $builder->add('campaign', EntityType::class);       
    $builder->add('invoicedToClient', DateType::class);
}

public function configureOptions(OptionsResolver $resolver): void
{
    $resolver->setDefaults([
        'factory' => [$this, 'factory'],
    ]);
}

public function factory(Campaign $campaign, DateTime $invoicedToClient): Invoice
{
    return new Invoice($campaign, $invoicedToClient);
}

The signature of factory() method must match names of form fields which helps in finding errors.

Typehinting parameters will also help static analysis tools; example is if user adds new dependency to Invoice::__construct() method, these tools will find issue in this InvoiceType form.

Example:

class Invoice
{
	public function __construct(Campaign $x, DateTime $y, bool $c)
	{...}
}

Unlike callables, closures will receive form data as array.

So creating instance of Invoice would prevent static analysis:

'factory' => function (array $data) {
    return new Invoice($data['campaign', $data['invoicedToClient'])
}

My suggestion is to change this decision and allow Closures to work the same as callables. If accepted, above example would be:

'factory' => function (Campaign $campaign, DateTime $invoicedToClient) {
	return new Invoice($campaign, $invoicedToClient);
},

Another big advantage is that this way, we could use form normalizers and construct data object in different way, based on some other option.

So if param $c is supposed to be injected from controller, normalizer would be this:

public function configureOptions(OptionsResolver $resolver): void
{
    $resolver->setRequired('some_param');
 	$resolver->setNormalizer('factory', function (Options $options) {
		$someParam = $options['some_param'];

		return function ($campaign, $invoicedToClient) use ($someParam) {
			return new Invoice($campaign, $invoicedToClient, $someParam);
		}

		// or with upcoming arrow functions:
		// return fn($campaign, $invoicedToClient) => new Invoice($campaign, $invoicedToClient, $someParam);
	})
}

With callables, we cannot use normalizers because other options cannot be passed to it.

It is not possible to use forms to create an entity that requires mandatory arguments in the constructor. We can probably reuse some of the code introduced with #22.

In context of https://github.com/sensiolabs-de/rich-model-forms-demo/pull/6 it getting obvious that custom exception handler might be very stupid glue code which could be replaced by config option to reduce implementation overhead.

namespace App\Form;

class ProductType extends AbstractType
{
    public function buildForm(FormBuilderInterface $builder, array $options)
    {
        $builder
            ->add('name', null, [
                'read_property_path' => 'getName',
                'write_property_path' => 'rename',
                'expected_exception' => CategoryException::class,
                // or
                'expected_exception' => [ProductException::class, CategoryException::class],
                'exception_handling_strategy' => ['product_errors', 'type_error'],
            ])
    }

    public function configureOptions(OptionsResolver $resolver)
    {
        ...
        $resolver->setDefault('expected_exception', CategoryException::class);
        // or
        $resolver->setDefault('expected_exception', [CategoryException::class, ProductException::class]);
        ...
    }
}

By default exception message is used as text for form error, but we should consider to introduce config options for message override and/or message translation domain

When using the read_property_path and write_property_path options, developers need to manually adjust these options in their form types after refactoring the underlying model (if the accessors referenced by either option have changed).

Most of the times, people are probably okay with this limitation as it basically applies to the built-in property_path option too. For those who prefer built-in refactoring support, we should think about an alternative option (like property_mapper) that accepts an object of a PropertyMapper interface which could look like this:

interface PropertyMapper
{
    public function readProperty($model);

    public function writeProperty($model, $value);
}

Sometimes, a property can only be accessed under certain circumstance. Take the following example where the parent category can only be retrieved when it is actually set:

public function getParent(): Category
{
    if (!$this->hasParent()) {
        throw CategoryException::hasNoParent($this);
    }

    return $this->parent;
}

This means that getParent could not be used as a value for the read_property_path option for Category instances without a parent category. This could be solved by allowing the option to be an anonymous function which could be used like this ():

$builder->add('parent', EntityType::class, [
    'read_property_path' => function (?Category $category) {
        return null !== $category && $category->hasParent() ? $category->getParent() : null;
    }),
]);

I created a form for the following user entity:

class User
{
    private $username;
    private $plainPassword;

    public function __construct(string $username, string $plainPassword)
    {
        $this->username = $username;
        $this->plainPassword = $plainPassword;
    }

    // getters...
}

My form type was configured like this:

public function configureOptions(OptionsResolver $resolver)
{
    $resolver->setDefaults([
        'data_class' => User::class,
        'factory' => User::class,
        'immutable' => true,
    ]);
}

When I submitting the form with all required data I got the following error:

Argument 1 passed to App\Entity\User::__construct() must be of the type string, null given, called in /path/to/symfony-app/vendor/sensiolabs-de/rich-model-forms-bundle/src/Instantiator/ObjectInstantiator.php on line 63

Omitting the data_class solved the issue.

When dealing with collections, using write_property_path can be problematic when dealing with non-direct relations.

Simple example; please note I am using new arrow function for readibility, recent github changes for code blocks are problematic:

Lets say we have Category<->Product many2many but with association class. But our form don't need to display assoc class, it can be much simpler:

// CategoryType
public function buildForm(FormBuilderInterface $builder, array $options): void
{
    $builder->add('products', EntityType::class, ...options...];
}

And Category entity:

public function getProducts(): array
{
    return $this->association
        ->map(fn(CategoryProductAssociation $assoc) => $assoc->getProduct())
        ->toArray();
}

public function addProduct(Product $product): void
{
    $this->assoc->add(
        new ProductCategoryAssociation($this, $product,... some extra param...)
    );
}

public function removeProduct(Product $product): void
{
    if ($assoc = $this->findAssociationToProduct($product)) {
        $this->association->removeElement($assoc);
    }
}

But when there is only setProducts(array $products)

'write_property_path' => fn($category, array $products) => $category->setProducts($products)

code would be much more complex. User would need to manually compare existing vs new values, something that Symfony already does perfectly.

Usage

New options would be:

$builder->add('products', EntityType::class, [
    'read_property_path' => fn(Category $category) => $category->getProducts(),
    // removed typehints for readibility
    'adder' => fn($category, $product) => $category->addProduct($product),
    'remover' => fn($category, $product) => $category->removeProduct($product),
]);

If adder and remover are set, then write_property_path is not required.

This has more usages when there is extra param needed, something I really need in my current app and the reason why I have to use DTO. If interested, I will put the problem here.

But in short; I have m2m relation between Client and CustomValue. CustomValue has discriminator string column type.
Entire ClientType form is made of 11 pages (flow bundle) and most of fields are working with that m2m relation but depend on value type column.

So for each value of type, I would need to write getter, adder and remover methods and make my entity totally unreadable.

With this RFC, I could change that to simple:

// ClientType.php
'read_property_path' => fn($client) => $client->getValuesOfType('xxx'),
'adder' => fn($client, $value) => $client->addValueOfType('xxx', $value),
'remover' => fn($client, $value) => $client->removeValueOfType('xxx', $value),

This way, I would need only 3 methods.

Given a model that has a setter-method with two or more arguments

class Order
{
    public function ship(Address $address, string $trackingNumber): void
    {
        $this->shippingAddress = $address;
        $this->trackingNumber = $trackingNumber;
    }
}

what is the best solution to this problem without changing the model to use a value object or something? is there already a good solution or do we need to come up with an idea?

Imagine an entity like the following:

class Subscription
{
    private $cancelledBy;

    public function cancelFrom(\DateTimeInterface $cancellationDate): void
    {
        $this->cancelledBy = (new \DateTimeImmutable())->setTimestamp($cancellationDate->getTimestamp());
    }

    public function cancelledFrom(): ?\DateTimeImmutable
    {
        return $this->cancelledBy;
    }
}

Using the default Form component options does not support this use case. The property path for the write method would be cancelFrom while the read operation requires cancelledFrom. We need two new options read_property_path and write_property_path handled by a custom data mapper to support this.

Some entities do not have generic setter methods that support passing arbitrary (even if restricted to a certain set of) values. Instead their interface requires to call different methods depending on the state transformation that is required:

class Subscription
{
    private $suspended;

    public function suspend(): void
    {
        $this->suspended = true;
    }

    public function reactivate(): void
    {
        $this->suspended = true;
    }

    public function isSuspended(): bool
    {
        return $this->suspended;
    }
}

Based on the solution for #3 we need to make it possible to configure the actual write_property_path value based on the submitted data.

@zmitic created a nice bundle which comes with some interesting features. We could join forces and see if there are features we could implement here too.

This idea comes from symfony/symfony#36022 (comment).

Is it really necessary that both options read_property_path and write_property_path need to be configured at the same time (see #1 (comment))? We should check that and refactor accordingly if we think that there are valid use cases for using only one or the other.

In context of https://github.com/sensiolabs-de/rich-model-forms-demo/pull/6 it looks redundant to use both factory and data_class option.

is it possible to reuse data_class option for our factories or is it possible to drop data_class in case factory is used?

Right now, the way exceptions/errors are handled is a hard-coded two step process:

1. If a TypeError is thrown because of an argument type mismatch, it will be handled by the ArgumentTypeMismatchExceptionHandler.
2. All remaining exceptions will be caught by the FallbackExceptionHandler and transformed into a generic error message.

This approach is not very flexible and we should strive to make it customizable:

1. Allow to add new exception handlers in userspace by creating a class implementing the ExceptionHandler interface and registering it as a service that is tagged with some special tag (the name is to be defined).
2. On a per-form basis allow to define the exception handling strategies to apply (and the order in which they are processed).

Seems like the TypeError validation is not caught.

Given the following form type:

final class UserClickType extends AbstractType
{
    /**
     * @param FormBuilderInterface $builder
     * @param array $options
     */
    public function buildForm(FormBuilderInterface $builder, array $options): void
    {
        $builder
            ->add('type', null, [
                'empty_data' => '',
            ])
            ->add('typeId', IntegerType::class, [
                'empty_data' => '0',
                'constraints' => [
                    new NotNull(),
                ],
            ]);
    }

    /**
     * @inheritDoc
     */
    public function configureOptions(OptionsResolver $resolver): void
    {
        $resolver->setDefaults([
            'csrf_protection' => false,
            'factory' => UserClickModel::class,
            'immutable' => true,
        ]);
    }
}

When using a string for typeId field I get the following stacktrace:

  "type": "http://localhost/api/error",
  "title": "Symfony\\Component\\Debug\\Exception\\FatalThrowableError",
  "status": 500,
  "detail": "Argument 2 passed to App\\Form\\Model\\UserClickModel::__construct() must be of the type int, null given, called in /var/www/html/vendor/sensiolabs-de/rich-model-forms-bundle/src/Instantiator/ObjectInstantiator.php on line 63",
  "trace": [
    {
      "file": "/var/www/html/vendor/sensiolabs-de/rich-model-forms-bundle/src/Instantiator/ObjectInstantiator.php",
      "line": 63,
      "function": "__construct",
      "class": "App\\Form\\Model\\UserClickModel",
      "type": "->",
      "args": [
        "store",
        null
      ]
    },
    {
      "file": "/var/www/html/vendor/sensiolabs-de/rich-model-forms-bundle/src/DataTransformer/ValueObjectTransformer.php",
      "line": 63,
      "function": "instantiateObject",
      "class": "SensioLabs\\RichModelForms\\Instantiator\\ObjectInstantiator",
      "type": "->",
      "args": []
    },
    {
      "file": "/var/www/html/vendor/symfony/form/Form.php",
      "line": 1137,
      "function": "reverseTransform",
      "class": "SensioLabs\\RichModelForms\\DataTransformer\\ValueObjectTransformer",
      "type": "->",
      "args": [
        {
          "type": "store"
        }
      ]
    },
    {
      "file": "/var/www/html/vendor/symfony/form/Form.php",
      "line": 638,
      "function": "viewToNorm",
      "class": "Symfony\\Component\\Form\\Form",
      "type": "->",
      "args": [
        {
          "type": "store"
        }
      ]
    },
    {
      "file": "/var/www/html/src/Controller/Api/User/UserClickController.php",
      "line": 130,
      "function": "submit",
      "class": "Symfony\\Component\\Form\\Form",
      "type": "->",
      "args": [
        []
      ]
    },
    {
      "file": "/var/www/html/vendor/symfony/http-kernel/HttpKernel.php",
      "line": 151,
      "function": "postClick",
      "class": "App\\Controller\\Api\\User\\UserClickController",
      "type": "->",
      "args": [
        {},
        {
          "attributes": {},
          "request": {},
          "query": {},
          "server": {},
          "files": {},
          "cookies": {},
          "headers": {}
        },
        {}
      ]
    },

This happens when in controller I submit the form without data:

        $form = $this->createForm(\App\Form\Type\UserClickType::class);
        $form->submit($decoder->decode($rawRequest->getContent(), 'json'));

The current name of the expected_exception option is misleading. It can easily be misinterpreted as the developer expecting the model to always throw these exceptions given the behaviour one observes in PHPUnit when using @expectedException and the like.

Talking to other people on the conferences we talked about this bundle we got some good suggestions for a better name. Amongst these are the following (I probably forgot some):

• intercept_exception
• catch_exception
• validation_exception
• handle_exception
• throws
• catch

When adding a SubmitType to a FormType the bundle attempts to map it to the model.

Reproducer example:

https://github.com/dbrumann/rich-form-bug1/pull/1

How to reproduce

1. Build a form mapping a model with a SubmitType

2. Build a controller for processing the form and a template using {{ form(form) }} for rendering the form

3. Open the page and submit the form

=> The form throws an error because the submit cannot be mapped to the object

Maybe this is already supported implicitly. In this case, we should add some tests to make sure not to break it in the future.

Hi Guys,
thanks for that awesome Bundle!
I am having some fancy mapping problem, i am a bit clueless about

consider this form:

<?php


namespace App\UI\Web\Form\Api\User\Type;


use App\Application\Core\User\Command\EditUserDetailsCommand;
use App\Domain\Enum\EmailNotificationFrequency;
use Symfony\Component\Form\AbstractType;
use Symfony\Component\Form\Extension\Core\Type\ChoiceType;
use Symfony\Component\Form\Extension\Core\Type\TextType;
use Symfony\Component\Form\FormBuilderInterface;
use Symfony\Component\OptionsResolver\OptionsResolver;

class ApiEditDetailsType extends AbstractType
{
    private ?int $userId;
    public function getBlockPrefix()
    {
        return '';
    }
    public function buildForm(FormBuilderInterface $builder, array $options)
    {
        $this->userId = $options['userId'] ?? null;
        $builder
            ->add('emailFrequency', ChoiceType::class, [
                'required' => true,
                'label' => 'Notification frequency',
                'choices' => [
                    'Never send me any emails!' => (string)EmailNotificationFrequency::NEVER(),
                    'Send me any news immediatly!' => (string)EmailNotificationFrequency::IMMEDIATLY(),
                    'Send a summary once a day' => (string)EmailNotificationFrequency::DAILY(),
                    'Send a summary once a week' => (string)EmailNotificationFrequency::WEEKLY(),
                ]
            ])
            ->add('claim', TextType::class, [
                'label' => 'Profile sentence',
                'required' => false,
            ])
        ;
    }
    public function configureOptions(OptionsResolver $resolver)
    {
        $resolver->setDefaults([
            'factory' => function($emailFrequency, $claim) {
                 return EditUserDetailsCommand::fromFormViewData(
		     $this->userId,
	             $emailFrequency,
	             $claim ?? '',
		 );
            },
            'csrf_protection' => false,
            'immutable' => true,
        ]);
        $resolver->setDefined([
            'userId'
        ]);
    }
}

and this simple command
<?php


namespace App\Application\Core\User\Command;


use Symfony\Component\Validator\Constraints as Assert;

class EditUserDetailsCommand
{
    protected int $userId;
    protected string $emailFrequency;
    /**
     * @Assert\Length(max="108")
     */
    protected ?string $claim;

    public function __construct(
        int $userId,
        string $emailFrequency,
        ?string $claim
    ) {
        $this->userId = $userId;
        $this->emailFrequency = $emailFrequency;
        $this->claim = $claim;
    }

    public static function fromFormViewData(int $userId, string $emailFrequency, string $claim)
    {
        return new self(
            $userId,
            $emailFrequency,
            $claim
        );
    }

    public function getUserId(): int
    {
        return $this->userId;
    }
    public function getEmailFrequency(): string
    {
        return $this->emailFrequency;
    }
    public function getClaim(): ?string
    {
        return $this->claim;
    }
}

but if the claim is more than 108 letters, i do not get the error as an error of the claim field but the form itself,

BUT if I add the following to the form defaults:

            'error_mapping' => [
                'emailFrequency' => 'emailFrequency',
                'claim' => 'claim'
            ],

everything works as expected ...

What am i doing wrong ? :)
since i dont think that info mus be there duplicated

While working on #63 I realised that a configured factory to initialise empty data is not called if the form is not compound.

For example, with the following form type if the submitted data is '500' the getData() method of the form would return 500 as an integer instead of a Price object containing the value 500:

class PriceType extends AbstractType
{
    public function configureOptions(OptionsResolver $resolver): void
    {
        $resolver->setDefault('factory', Price::class);
    }

    public function getParent(): string
    {
        return IntegerType::class;
    }
}

The Price class can be something like this:

final class Price
{
    private $amount;

    public function __construct(int $amount)
    {
        $this->amount = $amount;
    }

    public function amount(): int
    {
        return $this->amount;
    }
}

Do you plan to add Symfony 5 support in the near future? Thanks!