No form data when using a closure as factory

[hjkl]

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 :)

[xabbuh]

Thank you for the report. Could you please create a small application that allows us to reproduce the issue? I am not sure yet if this is a bug or some misuse here (which maybe could be prevented by improving the documentation).

[hjkl]

Thanks for your response, Christian.

I've made a small application here: https://github.com/hjkl/rich-model-closure-factory.git

[xabbuh]

Thanks, this helped a lot. 👍 Your case can be solved by setting the immutable option to true, but I am still going to investigate if we don't need to fix something here.

[zmitic]

I noticed the same problem but didn't have time to set reproducer. I solved it different way (this is really big form, I removed some fields for brevity):

public function buildForm(FormBuilderInterface $builder, array $options): void
{
	//.... 
    $builder->add('invoicedToClient', DateType::class, [
        'label' => 'Date the Supplier issued invoice to customer',
        'constraints' => [
            new NotNull(),
        ],
    ]);
	//.... 
}

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

/** @noinspection PhpTooManyParametersInspection */
public function factory(Campaign $campaign, DateTime $invoicedToClient, DateTime $sentAt): Invoice
{
    return new Invoice($campaign, $invoicedToClient, $sentAt);
}
    

This trick is to name parameters in factory() as form fields. Hope this helps.

---

I didn't want to use 'factory' => Invoice::class because variable names in constructor would need to match field names.

[xabbuh]

I didn't want to use 'factory' => Invoice::class because variable names in constructor would need to match field names.

You are right. Currently, that's indeed a limitation (see also #50 (comment)).

[zmitic]

@xabbuh To be honest, I don't think of this as limitation. I prefer creating instances manually because it is easy to find usages of __construct(); just ctrl+click.

More important is that static analysis tools could also easily detect problems. Example: I add new requirement to constructor, phpstan will see the problem in InvoiceType form. But if I used 'factory' => Invoice::class, it wouldn't.

Btw, I really love this bundle. No more wasting time on DTO classes, thank you guys!

[hjkl]

@zmitic That's really helpful. Your code works as the callback isn't a closure.

SensioLabs\RichModelForms\Instantiator\ObjectInstantiator's instantiateObject method explicitly checks if the factory is a \Closure:

if ($this->factory instanceof \Closure) {
    return ($this->factory)($this->getData());
}

As in the docs, for a closure it's supposed to give the form data as a single argument (but doesn't work).

Whereas for your [$this, 'factory'] callback, it unpacks into separate (...$arguments)

[zmitic]

@hjkl Exactly! I had to take a look at the code to figure how to use it and I am really impressed with argument unpacking solution; you just can't make a mistake.

I think the Closure version should work the same way as callable. User can typehint every parameter, bundle will catch TypeErrors, static analysis will always work...

Also, argument unpacking is much cleaner.

Smaller example, with future arrow functions:

'factory' => fn(Campaign $campaign) => new Invoice($campaign),

It is clean, readable and foolproof.

@xabbuh What do you think about that? Given that bundle is not 1.0.0 tagged, BC is not a problem, right?

[xabbuh]

@zmitic I am not sure I completely got what you would like to see supported. Could you create a new issue further described your feature request so we can keep things separated and not lose your idea when this bug is fixed?

[zmitic]

@xabbuh Done: #61

Sorry if too much text, documentation for factory is missing. So I decided to describe how it works, might help others.

[xabbuh]

@hjkl Can you confirm that #63 fixes your issue?

[hjkl]

Thanks @xabbuh, I can confirm that's fixed it.

[xabbuh]

@hjkl Thank you for the confirmation.