Skip to content

[PoC] Use options resolver in BaseFieldDescription#6078

Closed
phansys wants to merge 2 commits intosonata-project:masterfrom
phansys:field_description_options
Closed

[PoC] Use options resolver in BaseFieldDescription#6078
phansys wants to merge 2 commits intosonata-project:masterfrom
phansys:field_description_options

Conversation

@phansys
Copy link
Member

@phansys phansys commented May 4, 2020

Subject

I am targeting this branch, because this change breaks BC.

Changelog

### Added
- Leverage options resolver at `BaseFieldDescription`.

To do

  • Fix tests;
  • Decide how to deal with the invalid values (array) generated by BaseFieldDescription::mergeOptions().

Since BaseFieldDescription::mergeOptions() calls to array_merge_recursive(), the previously set options are being overridden as array:

If the input arrays have the same string keys, then the values for these keys are merged together into an array, and this is done recursively, so that if one of the values is an array itself, the function will merge it with a corresponding entry in another array too.

This, by instance, is causing "virtual_field" option to hold an array instead of a boolean at ListMapper::add().

@phansys phansys added the feature label May 4, 2020
@SonataCI
Copy link
Collaborator

Could you please rebase your PR and fix merge conflicts?

@github-actions
Copy link

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale label Nov 19, 2020
@github-actions github-actions bot closed this Nov 27, 2020
@franmomu
Copy link
Member

franmomu commented Mar 7, 2021

@phansys I think this could be great, just in case you want to|can continue working.

@phansys
Copy link
Member Author

phansys commented Mar 11, 2021

I agree @franmomu, I need to find enough time for this task.

@franmomu
Copy link
Member

I'll leave it open, so it doesn't get lost.

@franmomu franmomu reopened this Mar 12, 2021
@franmomu franmomu added keep and removed stale labels Mar 12, 2021
@franmomu franmomu mentioned this pull request Apr 21, 2021
@VincentLanglet
Copy link
Member

There was some discussions about this:

Some options are global: type, template, role, ...
Some options are specific: inverse for boolean type, currency, route for url type, ...
Custom option could be possible for custom FieldDescription. I could indeed create a type Amount with an option show_currency: bool to display or not the currency.

So I'm not sure we can use an option resolver, since we should allow every possible options.
Or we have to say that non-generic options should be in a key templateOptions and this require a lot of BC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants