🇫🇷France @just_like_good_vibes

for the issue, i recopy my small comment from this morning:

there is a little mistake when you fill in meta:enum
in component plugin manager line 182.

the meta:enum expect the format :
variant_key: title1
variant_key2: variant_title2

where currently, what is provided is a copy of the content of the variant part in the component which is :
variant_key:
title: title1
variant_key2:
title: variant_title2

ready for review

hello, ok i check now

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 :)

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?

thanks christian, to parallelize last alpha3 efforts, i re-take it :)

An we exchanged during the weekly, let's remark the following thing in the current 2.0.x codebase.
when we treated the issue 3444716 ✨ [2.0.0-alpha3] Add block source (for slots) Fixed ,
we declared a new consumer "ui_patterns" for block discovery and filtering.
As seen in Drupal\ui_patterns\Plugin\UiPatterns\Source::listBlockDefinitions,
we are calling plugin.manager.block:getFilteredDefinitions with ui_patterns as the first argument.
(see Drupal\Core\Plugin\FilteredPluginManagerTrait::getFilteredDefinitions).
this implies, some alter hooks are called during plugin discovery "plugin_filter_{$type}" and "plugin_filter_{$type}__{$consumer}".

We proposed a first implementation of those hooks in ui_patterns.module, and this may not be perfect right now,
and has to refined maybe in this issue.

see functions "ui_patterns_plugin_filter_block__ui_patterns_alter" and "ui_patterns_plugin_filter_block_alter" in ui_patterns.module

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 :)

hello,
i am just getting the code, and i get the following error: please review.

Uncaught PHP Exception TypeError: "Drupal\ui_patterns\Element\ComponentPropForm::getSources(): Argument #2 ($definition) must be of type array, null given, called in /var/www/html/ui_patterns/src/Element/ComponentPropForm.php on line 75" at /var/www/html/ui_patterns/src/Element/ComponentPropForm.php line 167

hello, please rebase before my review :)

code pushed, ready for review.

just_like_good_vibes → created an issue.

done, ready for review

just_like_good_vibes → created an issue.

hello,
updated code, ready :)

hello, freshly rebased MR

Finally : lot of classes renamed, some removed, cleaner code, better code.
some mechanisms removed, like the "getDiscoveryContexts" which is not really required anymore thanks to a few context_definitions annotations and the use of the new mechanism "RequirementsContext".
many thanks for Christian with his super remarks in DM.

100 lines of codes removed :)

the implementation has been prepared also for the merge of issue 3414293 📌 [2.0.0-alpha3] Implement PropTypeInterface::convertFrom() Needs review .

please let's review and merge if everything is ok, a much needed refactoring and cleaning.

to review

just_like_good_vibes → created an issue.

just_like_good_vibes → created an issue.

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

i take this one :)

thanks to issue 3454615 📌 [2.0.0-alpha3] Improved source plugins organization Fixed ,
we introduced tags to some basic ui_patterns sources.
The tags are used to filter sources fetched with method "SourcePluginManager::getDefinitionsForPropType" in the case of fetching sources with convertible to a specific prop type.
This allows to propose a better list of convertible sources for props and slots, especially string and slots.
The sources discarded from the convertible list, are the ones tagged with "widget:dismissible".
The sources tagged with "widget:dismissible" are widgets (in the sense the value is not automatically taken extracted from another data using drupal api), but the ones we think can be discarded when their prop type can be converted to the a target prop type.
For example, with string prop type, we have many simple prop types convertible to string : integer, urls .etc
To set value for a string prop using a source plugin, we don't want basic widgets for integer and urls to be available when we already have the textfield available which is natively compatible with string. "Path", "Url" & "Number" sources are discarded while textfield is kept in the list.
but the mechanism is generic :)

please review... i think now it may be ready for merge, addressing all previous comments except number #20 from Christian.

thanks guys :)

ok, i addressed your remark,
created specific classed for context definition and context. It is quite nice actually...
I also kept the method in SourcePluginBase to keep the management of requirements simple from consumer point of view.
please review :)

Thank you Christian for reviewing.
about the method you mentioned, i just created it to factorize a piece of code that would:
1: create the context when missing and adds the values provided
2: overwrite the context if it already exists and adds the values provided to the already existing values.

that's why i need the whole contexts array,
from the caller point of view, this makes adding values very easy, one line of code. you provide the context array and the values you want to add to the requirements, and it does the job (the method may also allow to update implementation later maybe by hiding the key in the context)

i put it as a static method in that class, but we could have put it elsewhere if we wanted.
i am not so sure to introduce a new context class as you suggested,
probably more elegant in terms of code, but unfortunately it adds even more code (& complexity ?) without really adding a new functionality.

i wanted this MR to removes code at the end, i think this is achieved if you do no count comment lines.

so i would rather keep it like this for now. what do you think?

ready for review

just_like_good_vibes → created an issue.

let's do this in alpha3

just_like_good_vibes → made their first commit to this issue’s fork.

hello,
should be fixed in Issue 3440278 🐛 [2.0.0-alpha2] Field Formatters: views show all source plugins Needs work

by the way, i just did a rebase of the current MR with branch 2.0.x, ready for review :)

should we tackle the following case :

a source X has declared prop type "integer".
in a string prop, we can select source X because conversion is possible between integer and string.

conversion from string to slot is also possible, but it is not declared from integer to slot.

in that latter case, do we explicitly try to find a conversions from integer to slot,
using a "conversion path" integer -> string -> slot
or we only cover direct conversions and require each conversion pairs to be declared and coded?

Hello,
as we discussed during the camp, i made the changes to allow better field formatter naming and working.
i think this MR is one of the toughest i have made during the last weeks, but it brings so much goodness :

+ better source plugins management for slots and props, through a better handling of contexts. Especially custom contexts which are
specified by ui patterns sources and have constraints (views source plugins, field prop source plugins...etc). The magic really happens now
The modification allowed also to clean the ".module" of ui_patterns_views and put the right code at the right place inside each ui patterns views ' source plugins ;)

+ some bugfixes on ui_patterns_field_formatters, which were appearing during nesting. the nested field formatter was not handling correction the field definition..

+ some renaming of the two field formatters which are "Single component" and "Component per item", to allow better understanding of what's happening when you render a field and its items with ui patterns.

+ renaming of field formatter PHP classes for better homogeneity

please review

G4MBINI → credited just_like_good_vibes → .

i take this one

done, please review :)

let's review please :)

hello,
i take this one

Hello,
i have worked on this issue, focusing on how to nest patterns.
the new code also removes the key "component_form" from the form objects...
please review, thank you :)

just_like_good_vibes → made their first commit to this issue’s fork.

just_like_good_vibes → created an issue.

The issue seems resolved with code coming from other recent issues.

Hello,
i have rebased your MR and ported the work to another branch, plus finished it according to the state of the code.
i have opened another MR, i think it is ready for merge.
would you please review?
thank you

just_like_good_vibes → made their first commit to this issue’s fork.

fixed in issue #3449560 ✨ Support Views configured with Ajax Fixed

just_like_good_vibes → created an issue.

big +1
thanks for being part of the community
and what you do

Hello, bugfix is attached the MR

just_like_good_vibes → created an issue.

just_like_good_vibes → created an issue.

just_like_good_vibes → created an issue.

just_like_good_vibes → made their first commit to this issue’s fork.

hello,
based on the previous discussions, i proposed a small rework of the source plugins to better handle default values while at the same time
providing a little clarity about the different sources available.
i felt like i had to introduce a special base class for the simple source plugins that shares some behavior about their value stored in the settings and compatible with the default from prop schema.
I also finally removed the two functions as mentioned earlier in the comments, to simplify the already too large source plugin classes.
(and sorry for the mess above this comment in the commit history of the MR, rebase did not go as expected).
let me know if you find it better.

Hello, i have rebased the MR and checked it.
it is effectively the port of the code from issue https://www.drupal.org/project/ui_patterns_settings/issues/3437139 ✨ LinksSettingType: add attributes of the link generator Needs review
ok to merge in this scope.

just_like_good_vibes → made their first commit to this issue’s fork.

hello,
i added some code to handle the conversion and source selection.

just_like_good_vibes → made their first commit to this issue’s fork.

hello,
i proposed a new code to handle this issue.
please let's review it.

---

By the way, when I checked the code from Drupal\ui_patterns\SourcePluginBase and SourceInterface,
i remarked some functions are unused now, and i would like to open the way to remove them.
I have the impression they are declared just to have a complete set of get/set helpers for Setting and Settings, but why keep them ?
here are those methods : "getSettings()" and "setSetting(string $key, mixed $value)"
since we have like some code dealing with defaultSettings happening in getSettings and setSettings, it would be better to remove the two others.
what do you think? should we? if yes, we remove them in this issue, in another issue?

thank you

Pierre, just to be sure, would you please elaborate what you exactly meant when you wrote this :
"Some sources plugins (MenuSource & ) are defining default values implementing PluginSettingsInterface::defaultSettings() method, but this method is already used in PropTypePluginBase and it is not working."

thank you :)

G4MBINI → credited just_like_good_vibes → .

About for each and for All,
why not just use the terms "Components"
(for each), and "Component"?

Hello,
I have a comment about the Javascript code added.
why a vanilla JS code instead of a proper drupal JS behavior?
The JS file is added to the global theme library, therefore in my understanding it should follow best practices → .
what do you think?
i can help in writing the code if needed.
thank you

Hello,
thank you for pointing out the MR to do.
But i was trying to open the MR on github, but i just saw you done it 1h ago :)
I guess for now i have nothing to do?
thank you

Hello again, just to summarize what happened in the last hours.
Thomas → added some refactoring today, and i just added a few more commit on top of that during the two last hours in order to simplify a little and correct a few remaining bugs.

The current state is not totally perfect but worth it compared to the past.
This MR should at least cover all the comments referenced by this issue and the three other issues mentioned in the related issues sections. Many thanks Pierre → for your precious reviews and comments.

For each: ❌ the form doesn't load

-> this is true when the field formatter "for each" is not applicable, when the cardinality of the storage is 1 at most.
if you select a field with cardinality superior or equal to 2, the for each will work in views.

i have checked the code, i am not 100% sure, but the list of formatters which is proposed in the views field option form, do not reflect the result of the method "isApplicable" inside the field formatters.

according to what i have seen the line in class "Drupal\views\Plugin\views\field\EntityField" from views module.
$formatters = $this->formatterPluginManager->getOptions($field->getType());

so what do we do?

PHPMD Naming annotations finally removed :)
(still done in issue #3440319 🐛 [2.0.0-alpha2] Formatters: functional feedbacks from alpha1 Needs review )

More code was added to entirely cope with questions from issue #13440310 📌 [2.0.0-alpha2] Formatters: technical feedbacks from alpha1 Needs review

Hello,
All modified code is done in related Issue #3440319 🐛 [2.0.0-alpha2] Formatters: functional feedbacks from alpha1 Needs review .

All answers to questions here :

1. Comment(s) added in hook_field_formatter_info_alter in .module
2. All cache invalidation removed in .module, also because source plugins no longer depend on bundle. cache invalidation in the event subscriber is needed to respond to modeling modifications without needing to clear the cache in order to see the corresponding source plugins appearing or disappearing.
3. the Mapping you are challenging had bad comments. it is in fact a mapping between ui patterns prop types and data types (e.g. types for props). So it should be ok, right? should we provide a mechanism to let eventual new prop types from contrib be handled ?
4. PHPMD Naming annotations removed.
5. The number of plugins has been reduced to reflect the triplets (entity type, field_name, property name). bundles are managed in a more subtle way.

Hello,
here is a quite deep modification of the sub-module ui_patterns_field_formatter,
in order to tackle several issues, and stabilize.
A lot of code has been removed, according to the current or related issues, and also some simplifications has been introduced.

There are less generated ui patterns source plugins (at least we get rid of the "by bundle"),
context has been greatly improved to allow field formatters to work inside view fields and more.

Some new helper functions in field formatter classes have helped the retrieval of settings and context values.

just_like_good_vibes → made their first commit to this issue’s fork.

just_like_good_vibes → made their first commit to this issue’s fork.

just_like_good_vibes → made their first commit to this issue’s fork.

just_like_good_vibes → made their first commit to this issue’s fork.

Merge request updated, please have a look.
sorry but during phpstan and phpcs phase, i had to update some small things inside the code to minimally improve it.

just_like_good_vibes → made their first commit to this issue’s fork.

just_like_good_vibes → created an issue.