🇫🇷France @just_like_good_vibes

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.

just_like_good_vibes → created an issue.

just_like_good_vibes → created an issue.

hello Pierre, you are right, i will push it now.
thank you for checking carefully :)

Thank you Pierre, for writing down this issue.
I agree with the technical proposition & refactor, and i would like to add my opinion as you suggested.
First, i agree the method would be rather static, unless we found a specific case of Prop type where it wouldn't be statically callable (i don't have such example in mind right now).
Second, i would rather vote to call this method "sanitize" or "normalize".
The role of the method is to consolidate the value, clean it and conform it to the right "expectations" of the plugin.
The two names "sanitize" or "normalize" are quite close to the final processing. I have a little personal preference to sanitize, but normalize express better the idea to conform to the right format.

MR !57 ready for review :)

hello,
the issue also occurs in the footer pattern

G4MBINI → credited just_like_good_vibes → .

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

just_like_good_vibes → created an issue.

just_like_good_vibes → created an issue.

Hello,
experiencing the same issue here.

the problem, seems to be a bit deeper from my understanding.
Indeed, let's take the example of a settings of type "token".
i configure a block in layout builder, which is mapped to a pattern, and i have ui_pattern_settings module enabled.
my pattern has a settings of type token -> "close_title".
i can configure my block in layout builder without error, including the settings close_title.

but when i save the layout, i have an error :

The configuration property third_party_settings.layout_builder.sections.0.components.3967b6f6-0208-4868-ba2f-8a3a6fefb983.configuration.settings.close_title.input doesn't exist." at /var/www/html/web/core/lib/Drupal/Core/Config/Schema/ArrayElement.php line 77

the "input" is added inside \Drupal\ui_patterns_settings\Plugin\TokenSettingTypeBase::settingsForm.

this function is called inside \Drupal\component_blocks\Plugin\Block\ComponentBlock::formatterSettingsProcessCallback

from my understanding, the component_block module, retrieves some form elements for the settings,
and in case of tokens, an. additional form key "input" is added, which further creates a configuration with that "input" as a parent key for the value, when it shouldn't.

something would need to be changed inside \Drupal\component_blocks\Plugin\Block\ComponentBlock::blockSubmit ?

G4MBINI → credited just_like_good_vibes → .

thank you Pierre, all the best

hello,
i have contributed this module which help in solving the issue.

Varnish Purge Tags Override →

it implements the approach (c). It rewrites the generated "Cache-Tags" header value. It allows to replace content entity cache tags (like node:XX) by list cache tag (like node_list).

just_like_good_vibes → created an issue.

just_like_good_vibes → created an issue.