Easyadminbundle: Virtual fields set as "required" since update

[JoeKre]

Hi.

Don't know if this is a bug or intentional but virtual properties of an entity are now by default marked as required in edit view. (Version 1.17.3)

Maybe this is relatd to: 432b1c2f
A good example would be the file properties used with vich_uploader:

form:
    fields:
        - name
        - { property: 'imageFile', type: 'vich_image' }

/**
 * @Vich\UploadableField(mapping="image", fileNameProperty="image")
 */
 protected $imageFile;

/**
 * @ORM\Column(type="string", length=255, nullable=true)
 */
 protected $image;

[javiereguiluz]

@JoeKre thanks for reporting and for the reproduction example.

@alterphp @iluuu1994 could you please help me check if this is indeed related to 432b1c2f400dfa5eb73104ece0ea834dcac57d09? Thanks!

[alterphp]

@JoeKre @javiereguiluz Yes, it could be related. I'm going to investigate...

[COil]

Hi, we also experiencing the same thing, very annoying.

[COil]

At least, we found bugs where we were using virtual field instead of real columns. :)

[alterphp]

@JoeKre @COil Can you test this attempt of fix https://github.com/javiereguiluz/EasyAdminBundle/pull/1847 ?

As I don't use VichUploader nor virtual fields, I can not really test it. Can you write a test for the fix ?

[COil]

You can add a field as 'test_field' and declare it in easy admin with 'test_field' instead of the doctrine property 'testField' that will make the field "virtual". It was our case.

[alterphp]

The test of PropertyConfigPass is not a functional test, it does not use full AppBundle test bench...

Can you dump parameters sent to getFormTypeOptionsOfProperty method at __line 194 of PropertyConfigPass__ file ?

dump($normalizedConfig, $fieldMetadata, $originalFieldConfig);

$normalizedConfig['type_options'] = $this->getFormTypeOptionsOfProperty(
     $normalizedConfig, $fieldMetadata, $originalFieldConfig
 );

[JoeKre]

@alterphp
i hope that's what you mean:

array:21 [▼
  "css_class" => ""
  "format" => null
  "help" => null
  "label" => null
  "type" => "text"
  "fieldType" => "textarea"
  "dataType" => null
  "virtual" => true
  "sortable" => false
  "template" => null
  "type_options" => []
  "form_group" => null
  "columnName" => "field"
  "fieldName" => "field"
  "id" => false
  "length" => null
  "nullable" => false
  "precision" => 0
  "scale" => 0
  "unique" => false
  "property" => "field"
]

array:11 [▼
  "columnName" => "field"
  "fieldName" => "field"
  "id" => false
  "length" => null
  "nullable" => false
  "precision" => 0
  "scale" => 0
  "sortable" => false
  "type" => "text"
  "unique" => false
  "virtual" => true
]

array:1 [▼
  "property" => "field"
]

That's for a simple property added to an entity
protected $field;

with it added to the entity configuration

form:
    fields:
        - field

[alterphp]

@JoeKre Thanks. I can't see any required item in type_options... Can you dump the result of the getFormTypeOptionsOfProperty method call ?

dump($normalizedConfig, $fieldMetadata, $originalFieldConfig);

$normalizedConfig['type_options'] = $this->getFormTypeOptionsOfProperty(
     $normalizedConfig, $fieldMetadata, $originalFieldConfig
 );

dump($normalizedConfig['type_options']);

And if possible, the same dump before the EasyAdminBundle update.

[JoeKre]

@alterphp

Sure:
[]

it's empty.

Looking at the conditions in getFormTypeOptionsOfProperty that's to be expected.

$userDefinedConfig['type'] isn't set and $userDefinedConfig['type_options'] is also undefined. So neither the if nor else if part are matched. And since the initial type_options were empty there's not much happening.

[alterphp]

@javiereguiluz @JoeKre @COil

So I don't think commit #432b1c2f is related to your problem. The former version neither did match your conditions...

I don't know where the required type options is set... Have you updated vich package too ?

[JoeKre]

It looks like the required appears when the form is generated in editAction.

$editForm = $this->executeDynamicMethod('create<EntityName>EditForm', array($entity, $fields));

with this configuration for a virtual field:

"field" => array:21 [▼
    "css_class" => ""
    "format" => null
    "help" => null
    "label" => null
    "type" => "text"
    "fieldType" => "textarea"
    "dataType" => null
    "virtual" => true
    "sortable" => false
    "template" => null
    "type_options" => []
    "form_group" => null
    "columnName" => "field"
    "fieldName" => "field"
    "id" => false
    "length" => null
    "nullable" => false
    "precision" => 0
    "scale" => 0
    "unique" => false
    "property" => "field"
  ]

Looks like the FormBuilder defaults it to required. But i have not the slightest why the behavior changed.
@javiereguiluz Do any other changes between 1.16.13 and 1.17.3 come to mind that may have affected this? Just downgraded again to confirm the difference once again to make sure i'm not going crazy.

@alterphp I didn't check the vich bundle directly that was just an example where in my case the virtual fields were used. But it also accurs when you just add a property to your doctrine entity without making it a doctrine managed field and add the property into the edit form through easyadmin.yml

[JoeKre]

Ok i identified the culprit.
The reason is this change https://github.com/javiereguiluz/EasyAdminBundle/commit/56809b2dce92fcee69399cf56b7f933c85e40521 in PropertyConfigPass

$requiredGuess = $this->getFormRequiredGuessOfProperty($entityConfig['class'], $propertyName);
is only executed in processMetaDataConfig.
But the virtual Fields are not part of the metaData from doctrine and are not handled at all.

As a short proof i added the following in the processFieldConfig method (line 194):

if(array_key_exists('type_options', $normalizedConfig) 
    && !array_key_exists('required', $normalizedConfig['type_options'])) {
    $normalizedConfig['type_options']['required'] = $this->getFormRequiredGuessOfProperty($entityConfig['class'], $fieldName)->getValue();
}

right before the call of

$normalizedConfig['type_options'] = $this->getFormTypeOptionsOfProperty(
    $normalizedConfig, $fieldMetadata, $originalFieldConfig
);

Edit: probably adding

if(!array_key_exists('required', $normalizedConfig['type_options'])) {
    $normalizedConfig['type_options']['required'] = $this->getFormRequiredGuessOfProperty($entityConfig['class'], $fieldName)->getValue();
}

after the getFormTypeOptionsOfProperty-Call would be a good solution?

[iluuu1994]

I investigated this.

Turns out this is because form fields are required by default and the RequiredOptionConfigurator was removed.

This could be solved by adding 'required' => false to the default config.

[iluuu1994]

I'm running the tests and I'll create a pull request. Afterwards I'll think about a way to test this to avoid the same bug in the future.

[iluuu1994]

@JoeKre @COil Can you check if this patch solves your issue?

[JoeKre]

@iluuu1994

Confirmed. This solves the problem with virtual fields.
Thx.

[alterphp]

@iluuu1994 Does ValidatorTypeGuesser (from Symfony) still apply ? If you have a NotBlank constraint on a field, is required value set to __true__ ?

[iluuu1994]

@alterphp Yes. The default value from the $defaultEntityFieldConfig is overwritten by the guessed property form type here unless the property doesn't exist (as in virtual properties).

Maybe it would be better to add it to the $defaultVirtualFieldMetadata array? That would probably cause less confusion.

[alterphp]

@iluuu1994 Ok, seems good ! I think adding the default value to $defaultVirtualFieldMetadata is definitely the best approach (as it is specific to virtual fields for now).

[COil]

Hi, thanks for the correction, any chance to have a new tag?

[iluuu1994]

@COil I don't have merge privileges.

@javiereguiluz Would you prefer the required => false default value to be in $defaultVirtualFieldMetadata? I don't have time until later tonight to make this change.

[iluuu1994]

@javiereguiluz Voilà. Can you merge and tag?