Fix phpstan field options

[franmomu]

Follow-up of #7099 (review) to merge to master is not affected.

[jordisala1991]

If we don't remove this one it fails ?

[franmomu]

yes

[VincentLanglet]

Copy of my last response in #7099

Restricting the option in the interface FieldDescriptionInterface, it's like restricting the option in the FormTypeInterface.
The options resolver is specific to a FormType. Each FormType has custom options.
It's the same here: each FieldDescription could have custom option.

I think it's quite different, by general, there is only one or two implementations (one per persistence bundle) of FieldDescriptionInterface, you don't create an implementation of FieldDescriptionInterface for a custom field.

Yes, but someone could use his own persistence or his own FieldDescription with more features (and options).
This should be allowed.

The options, as stated in the code, are dependent of the type of the context.

Some options are specific to only some FieldDescription type ; for instance timezone, which is only used for date values.

SonataAdminBundle/src/FieldDescription/FieldDescriptionInterface.php

Line 52 in 4b1a41d

* timezone?: string|\DateTimeZone,

or currency

SonataAdminBundle/src/Resources/views/CRUD/display_currency.html.twig

Line 16 in 2c07756

{{ field_description.options.currency }} {{ value }}

which only related to the TYPE_CURRENCY and is not in the FieldDescriptionOptions currently.

another example inverse:

SonataAdminBundle/src/Resources/views/CRUD/display_boolean.html.twig

Line 19 in 2c07756

{% if field_description.options.inverse|default(false) ? not value : value %}

format:

SonataAdminBundle/src/Resources/views/CRUD/display_time.html.twig

Line 17 in 2c07756

{{ value|date(field_description.options.format|default('H:i:s'), field_description.options.timezone|default(null)) }}

I'm sure I can find others options used for specific FieldDescription types.
And since we're allowing custom types, we shouldn't restrict the options for them.

And when using a custom template, options are passed to the template, so it's a way to pass special values to the view, so basically any value could be possible.

For passing values to the template there should be an option for that, to use options that way it's just a hack and I think there is no documentation about that.

I just follow the way we're doing for all the existing templates.

[jordisala1991]

How do we move forward? This is blocking the master branch

[VincentLanglet]

How do we move forward? This is blocking the master branch

As I said, for me the FieldDescriptionOptions is wrong and should be removed in favor of array<string, mixed> since we're using a lot more options than the one in the FieldDescriptionOptions and a user could have some specific options for custom types.

What If I wanted to create my own type with a precision options ?

[franmomu]

what kind of type?

We are trying to close API and restrict things, if we need options we can add them, but not leave this open to anything. Even if we need to add an option for adding options I'd rather do this than mix them with all the options.

[VincentLanglet]

what kind of type?

We are trying to close API and restrict things, if we need options I can add them, but not leave this open to anything.

If we look
https://symfony.com/doc/current/bundles/SonataAdminBundle/reference/field_types.html#list-and-show-actions

There is at least format, timezone, as_string, subject, body, catalogue, currency, editable, inline, display, key_translation_domain, value_translation_domain, ajax_hidden, inverse, choices, multiple, delimiter, class, required, hide_protocol, url, attributes, route.name, route.parameters, route.absolute, route.identifier_parameter_name, strip, truncate, truncate.length, truncate.cut, truncate.ellipsis.

I agree we're trying to close API, but the field description is something open by pattern since custom type are allowed:
https://symfony.com/doc/current/bundles/SonataAdminBundle/reference/field_types.html#create-your-own-field-type
And I see no reason to forbid the developer to pass options to his custom type the same way we're doing it.

I could create my own type amount

->add('initialAmount', 'amount', [
    'currency' => true, // A custom option to display or not the currency
    'precision' => 2, // A custom option for the number of digits after the coma.
])

Here, the developper will have two issues:

• currency will be restricted to string in FieldDescriptionsOption because we use it as a string in the FieldDescriptionInterface::TYPE_CURRENCY, but the amount type has nothing in common and works with a boolean I shouldn't have to rename the option just because it's used by another fieldDescription type.
• precision doesn't exist in FieldDescriptionsOption if we don't add &array<string, mixed> but the code written is valid.

The issue here is that we have generic options but also specific (to only some field description types) ones and we pass them the same way. If something should be in the FieldDescriptionsOption, it should only be the generic options for all the possible types. Then we add &array<string, mixed> to allow any custom specific options.

[franmomu]

To me the right approach would be to do as we do with form that has a separated options:

SonataAdminBundle/src/Form/FormMapper.php

Line 80 in a8731dc

public function add($name, $type = null, array $options = [], array $fieldDescriptionOptions = [])

and filters:

SonataAdminBundle/src/Datagrid/DatagridMapper.php

Lines 78 to 85 in a8731dc

	public function add(
	$name,
	$type = null,
	array $filterOptions = [],
	$fieldDescriptionOptionsOrDeprecatedFieldType = [],
	$deprecatedFieldOptions = null,
	$deprecatedFieldDescriptionOptions = []
	) {

[jordisala1991]

To me the right approach would be to do as we do with form that has a separated options:

SonataAdminBundle/src/Form/FormMapper.php

Line 80 in a8731dc

public function add($name, $type = null, array $options = [], array $fieldDescriptionOptions = [])

and filters:

SonataAdminBundle/src/Datagrid/DatagridMapper.php

Lines 78 to 85 in a8731dc

	public function add(
	$name,
	$type = null,
	array $filterOptions = [],
	$fieldDescriptionOptionsOrDeprecatedFieldType = [],
	$deprecatedFieldOptions = null,
	$deprecatedFieldDescriptionOptions = []
	) {

That's something worth considering, but for now, the array should accept any options, like it was before. And an issue could be opened for tackle that problem.

[VincentLanglet]

To me the right approach would be to do as we do with form that has a separated options.

It could something to start with, keeping in mind that

• For the form, the field descriptions options are added to the options https://github.com/sonata-project/SonataAdminBundle/blob/3.x/src/Builder/AbstractFormContractor.php#L70 before being passed to the form builder

  SonataAdminBundle/src/Form/FormMapper.php

  Line 196 in a8731dc

  $this->formBuilder->add($child, $type, $options);

• For the filters, they are merged

  SonataAdminBundle/src/Datagrid/DatagridMapper.php

  Line 137 in a8731dc

  array_merge($filterOptions, $fieldDescriptionOptions)

Personally, when using these methods, I never used the second array of option and put all my options directly in the first array because I never understood which options should be in the first array, and which should be in the second one.

But we should indeed

• either use two options params, which shouldn't be merged like we do actually, one for generic options, one for custom options.
• either moved all the custom options to a new key like template_options.

This seems to need a lot of refacto, BC-break, reflexions and discussions so I agree with jordisala, the quickest way to solve the issue is to remove FieldDescriptionsOptions in favor of array<string, mixed> and open an issue.