- Issue created by @pdureau
- Assigned to Christian.wiedemann
- 🇫🇷France pdureau Paris
Christian, as the author of the initial code, you are the most suited for this task :)
- 🇫🇷France pdureau Paris
Do we also implement
allow_variant_expose
here?If yes, that would imply changes:
- "variants" become a "magic" prop type plugin like "slot" (so not like the regular JSON schema based props)
- the form hardcoded in
ComponentForm::buildComponentVariantSelectorForm()
become teh default source plugin of this prop type - this source plugin target also "variants" prop type. A source selector become available for variants in forms
- 🇩🇪Germany Christian.wiedemann
This feature does two things:
1. Provide values from a component as options into a field. This is done by updating the field storage with an allowed_values function which points to the component. So one field storage maps to a component setting
2. Proprocess the value of this field into the component if the field exists inside the entity.The problem with the current implemention is that:
1. There is a hard link between a field storage and a component setting
2. Every viewmode takes this mappingSo actually this are two features.
1. Provide values to a field
2. A SourcePlugin where you can easily set field value to a property. Something like EntityValueSourcePlugin.For part 1 I have no idea. For part 2. we should discuss if we need it and if we can reuse code from field formatter to handle every property of every value.
- Status changed to Needs review
6 months ago 11:37am 16 May 2024 - 🇫🇷France pdureau Paris
For part 1 I have no idea.
Maybe we don't need part 1. Let's focus on part 2.
if we can reuse code from field formatter to handle every property of every value.
In ui_patterns_field_formatter, we have 3 source plugins, all based on the entity + field context (we cannot select something outside of the field we are currently working on):
- FieldLabel source: just the field label, nothing fancy.
- FieldProperty source: with a deriver on the field property, and no specific configuration, we only "take" the value of the specific field property. There will be a mapping with field property types (as typed data types) inside each component prop types annotation:
* @PropType( * id = "string", ... * typed_data = { * "datetime_iso8601", * "email", * "string" * } * )
- FieldFormatter source: the context is the (entity type, field) tuple, without and the configuration is the selected formatter configuration form
The second one is the interesting one to study.
Raw ideas
For this source, we are not stuck into a specific field context, we have access to all the fields from the current entity.
So, it would be great to consider this source as a generalization of
FieldProperty
source, where the selection is not limited to the current field any more. However, let's avoid the use of a deriver here, because it will make the source selector busy.We can work on the field property level, and reuse the "typed_data" mapping from annotations, or we also can stay at the field level and we will need another mapping.
The source plugin form will be one or two select lists to pick the field and/or the field property, according to the prop type.
If the prop has an enumeration, and the selected source field storage has an
allowed_values
mechanism, it may be nice to alter the list and cover the "thing" 1. . But let's be careful about altering the data model from the display. Let's skip this idea if there is any doubt. - Status changed to Active
6 months ago 5:04pm 16 May 2024 - 🇫🇷France pdureau Paris
See also ✨ [2.0.0-alpha3] Blocks: Add sources with an entity context Needs review
- Status changed to Needs work
6 months ago 8:50am 6 June 2024 - Merge request !112Resolve #3423185 "2.0.0 alpha3 add allowexpose" → (Open) created by Christian.wiedemann
- Status changed to Needs review
6 months ago 10:37pm 10 June 2024 - 🇩🇪Germany Christian.wiedemann
Rebased against the current dev. Filter mechanism works great. I am using tags also to filter for groups. Flor that I add the mech to compoent form element.
I think we should move the Field Sources to ui_patterns not to ui_patterns_field_formatter. Field formatter is only the formatter. - 🇫🇷France just_like_good_vibes PARIS
Hello,
here are my comments on this work in the MR.
lot of things :
approx 1000 lines of codes added, and 500 lines removed.First, i think we definitely need to separate this work into two separate issues :
+ part 1 of the work : complete rewriting of how forms work and are organised. this is a bit touchy and need to be addressed alone first.
+ part 2 of the work : a new source, with an advanced data "picker".
about the part 1 of the work :
maybe still a few bugs? for example in "ComponentForm::buildPropsForm", missing #tag_filter ?
now we have 5 "FormElement", encapsulated.
i wonder how the current and how your proposed implement can give room for updates and UI improvements ?
just a random idea for example, could we imagine a component with a twig file describing its form organisation, and easily applying this twig with the current implementation?i would feel more confortable, addressing it alone without part 2 (and with a diverse set of test scenarios)
about the part 2 of the work :
i like the work done by the new source, but i don't like how it has changed the existing sources from ui_patterns_field_formatters.
i would have prefered a complete new set of sources and keep the other sources without any change.
for example, i did not understood why you had to put this code in various sources... for me the context_requirements are really used to make it work in the right context, to make them properly available as soon as the context is matched.
While tags are more for organization, allowing to pick only a selection of sources depending on the caller intent.'context_requirements' => [ 'group:field_property' ],
another remark about "alterContexts", which adds another layer of complexity imho.
how could we proceed now to go further ?
can we make part 2 and part 1 independent?
thanks
- Status changed to Needs work
5 months ago 1:15pm 19 June 2024 - Status changed to Needs review
5 months ago 10:57pm 20 June 2024 - 🇩🇪Germany Christian.wiedemann
So now the MR is smaller.
I renamed the group stuff to Picker.
Each picker has two mehtod to decide which sub source are displayed.
I also removed the field property picker. - Status changed to Needs work
5 months ago 8:25pm 21 June 2024 - 🇫🇷France just_like_good_vibes PARIS
here are my comments :)
- class EntityFieldSourceDeriver : why not subclassing FieldPropertiesSourceDeriverBase like the class FieldFormatterSourceDeriver which does similar derivation? you may save lines of code. no constuctor needed, no create function. you would have to recopy 3 lines of getPropTypesDeriver from FieldFormatterSourceDeriver, and write a few lines inside getDerivativeDefinitionsForField to derive the plugins.
- i think this may be also the right time to do a little renaming and very small refacfor of the derivers and the sources., to have nice family : a base class, and three sublcasses : 1 for FieldPropertySourceProp (entity field props), 1 for field formatters (FieldFormatterSource) and 1 for EntityFieldPicker. FieldPropertySourceProp should be "FieldPropertySource". EntityFieldPicker should be "FieldPickersource". the derivers may be renamed too : FieldPropertiesSourceDeriverBase -> FieldDeriverBase, FieldPropertiesSourcePropDeriver ->FieldPropertySourceDeriver, EntityFieldSourceDeriver -> FieldPickersourceDeriver. Also the implementation of getBaseDerivativeDefinitionForField should be abstract in the base class and the actual default one moved to FieldPropertySourceDeriver::getBaseDerivativeDefinitionForField
- in EntityFieldPicker, getPickedSourcesContextRequirements returns "field_granularity:item" which is generally wrong. indeed, this context requirement is when field items are considered one by one, and it allows access to prop. this context is automaticllay injected when we have a single value field and use field formatter, or when we have a multi-value field and use the perItem formatter. Otherwise "field_granularity:item" may be out of scope or not applicable.
- i think the name "Entity Fields Picker" is not really easy to understand. i would rather prefer like "Use entity field". Because it is what it does : injecting a context (entity,field) into a prop or slot form, to be able to get the list of associated sources and use them
- the EntityFieldPicker plugin labels are not good, this
t("Field : @field", ["@field" => $field_name]) is not ok on the UI and should rather show the right field label, according to the label configured for each bundle. - also "Picker" may not be the best name now that i am faced to it... why not "Selector". SourcePickerBase -> SourceSelectorBase.
EntityFieldsPicker -> FieldSelectorSource. EntityFieldPicker -> FieldSourcesSelectorSource. yes that's right, EntityFieldPicker is not really letting you pick a field. but rather, it allows you to select a source compatible with a field. Also, you may have noticed, we are naming source plugins with "Source" at the end or "Widget" at the end, let's keep that :)
Globally, the idea of this MR is quite nice, and the way it will work is also quite smart (re-using prop_form and slot_form).
it will be a real game changer, because it will allow to select compatible sources with the context of a field.
the more sources exist, the more will automatically appear without any line of code...
But unfortunately, it still needs some work, i can help if you want.i am not sure i listed all my comments, but let's address those right now and see how it goes at next round :)
- 🇫🇷France just_like_good_vibes PARIS
Hello, as we discussed off-line, i tested a new idea of a source to implement the ability to pick sources from fields of an entity.
it seems to work...
someone wants to review? - Merge request !128Resolve #3423185 "using derivable contexts" → (Merged) created by just_like_good_vibes
- Status changed to Needs review
5 months ago 2:21pm 24 June 2024 - 🇫🇷France just_like_good_vibes PARIS
hello,
following our internal meeting, I did another try using "DerivableContext" solution.
Very nice one. Seems to work like a charm.
promising framework, especially for entity reference or other derivable contexts...
please review :) - Status changed to Needs work
5 months ago 9:32am 26 June 2024 - 🇫🇷France just_like_good_vibes PARIS
i put here the comments received from Christian, by PM.
- architecture seems good.
- the generic DerivableContextSource Class raises question and confusion. Christian suggests it should be a base class because it loads all derivable contexts and all the sources. Following the logic that a source itself should have a specific use case (Like loading fields), Nobody will understand what the source is really doing. This is confusing. - Status changed to Needs review
5 months ago 10:05am 26 June 2024 - 🇫🇷France just_like_good_vibes PARIS
Ok, this is better now !
please review, thank you so much Christian for your comments.
this is more understandable now.. - 🇩🇪Germany Christian.wiedemann
Yes we are nearly done now.
I update the MR with comments.
Following ideas - but we can do that later:
* Should we introduce a "field" tag? I think we should only display field related sources.
protected function getSourcesTagFilter(): array { return [ "field" => TRUE, "widget:dismissible" => FALSE, "widget" => FALSE, ]; }
* Should we also introduce tags also for the DerivableContextPluginBase annotation to filter contexts?
- 🇫🇷France just_like_good_vibes PARIS
ok i addressed the comments,
let's go...
better to merge issue 3444768 ✨ [2.0.0-alpha3] Blocks: Add sources with an entity context Needs review
before and then this one if everything is ok. - 🇫🇷France just_like_good_vibes PARIS
MR rebased after recent merge. let's go :)
- 🇫🇷France just_like_good_vibes PARIS
Hello,
just added
- better labels for DerivableContext
- added "metadata" key to DerivableContext annotations
- improved DerivableContextDeriver to add metadata in DerivableContext annotations (to allow future UI to sort and filter fields)
- the improved deriver here will be used in another issue for deriver refactoring and simplificationthanks
- Assigned to pdureau
- Assigned to Christian.wiedemann
- 🇫🇷France just_like_good_vibes PARIS
another improvement, as suggested by Christian and it also cover some Pierre remarks :
- added some tags to plugin annotation of some sources from ui_patterns and ui_patterns_field_formatters. In particular sources linked to fields
- we restict EntityFieldSource sources to have the field tag when we get the sources for a derivable Contextwhat do you think, better right?
-
Christian.wiedemann →
committed 7d35fe4b on 2.0.x authored by
just_like_good_vibes →
Issue #3423185 by just_like_good_vibes, Christian.wiedemann, pdureau: [2...
-
Christian.wiedemann →
committed 7d35fe4b on 2.0.x authored by
just_like_good_vibes →
- Status changed to Fixed
5 months ago 12:47pm 28 June 2024 - Issue was unassigned.
- Status changed to Fixed
5 months ago 1:43pm 3 July 2024