[2.0.0-alpha3] Add allow_expose source plugin

Created on 22 February 2024, 9 months ago
Updated 3 July 2024, 5 months ago

Problem/Motivation

This mechanism from UI Patterns Settings 2.x is expected in UI Patterns 2.x:

allow_expose: true (V2)
With allow_expose enabled you can expose this setting directly to a field storage.
 The same works allow_variant_expose for variants.
 After enabling, a drop-down with allowed settings appears during field generation if the field type matches to the settings type.

Proposed resolution

Add it as a source plugin

Remaining tasks

Don't forger to update the specs first.

Feature request
Status

Fixed

Version

2.0

Component

Code

Created by

🇫🇷France pdureau Paris

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • 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 mapping

    So 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
  • 🇫🇷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
  • Status changed to Needs work 6 months ago
  • Status changed to Needs review 6 months ago
  • 🇩🇪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
  • Status changed to Needs review 5 months ago
  • 🇩🇪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.

  • 🇫🇷France just_like_good_vibes PARIS

    hello, please rebase before my review :)

  • Status changed to Needs work 5 months ago
  • 🇫🇷France just_like_good_vibes PARIS

    here are my comments :)

    1. 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.
    2. 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
    3. 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.
    4. 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
    5. 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.
    6. 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 :)

  • Merge request !127New source → (Closed) created by just_like_good_vibes
  • 🇫🇷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?

  • Status changed to Needs review 5 months ago
  • 🇫🇷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
  • 🇫🇷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
  • 🇫🇷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 simplification

    thanks

  • Assigned to pdureau
  • 🇫🇷France pdureau Paris

    I will have a look

  • 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 Context

    what do you think, better right?

  • 🇩🇪Germany Christian.wiedemann

    YEAH it is merged. Thanks, thanks, thanks!!!

  • Status changed to Fixed 5 months ago
  • 🇫🇷France just_like_good_vibes PARIS

    youhouuuu !

  • Issue was unassigned.
  • Status changed to Fixed 5 months ago
Production build 0.71.5 2024