Add a common method for suggestions in Field Widget Actions

This is great. I wonder if it could even be more optimized such that the returnSuggestions method only returns a key/value array and the whole ajax implementation is done by the base class. That way, the implementing developers have even less work to do, and we only maintain the ajax part once.

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

I would like to hear some feedback for the initial work that was done. What can be done more? Please check MR and review. It is still in Draft as I am not sure that the issue is done in full. Waiting for suggestions ;)

The code looks great and it feels like implementing this in a plugin is simple. Thanks for that.

I haven't tried it yet, but have 2 questions, though:

• If there is only 1 suggestion, the plugin can return just a string, right?
• If it's more than 1, how should the suggestion array look like? Is it just a list of strings?

The method `returnSuggestions` accepts suggestions as string or array. If it is string - then it is just shown in the ui dialog, if it is array of strings, then they are rendered as list where one can select the item and it will be automatically set as value in the corresponding field (that triggered the action).

So the array should be just array of strings, either associated or normal list, it doesn't matter, as in the twig template it will just loop through all values.

I am still not sure how this would work in case the button is one for the multivalue field (of course if it is a realistic use case).

Sounds great, thank you. I'll give it a try and enhance the ECA implementation of the FWA to support this. Then I can also give a better feedback on if and how it works.

Looking forward to your feedback! Thanks!

I got it working, and the computing is working as expected. The only thing is that the modal doesn't show up. The ajax request receives the expected response that contains all the content as it should be. Just the dialogue is not showing. I guess that's something with the ajax and dialog libraries from core that need some special treatment. I can look into that tomorrow in more detail.

Please check as an example the plugin from ai_content_suggestions module.

I've no luck, even the PromptContentSuggestion doesn't show any popup to me when I can verify that the execution of aiContentSuggestionsAjax returns an array of strings. Can you please help we with that one @a.dmitriiev?

aiContentSuggestionsAjax has return $this->returnSuggestions($suggestions, $selector); as a last line https://git.drupalcode.org/project/ai/-/merge_requests/763/diffs?file=60... . It is not returning the array of suggestions, it passes the array of suggestions to the returnSuggestions method. And returnSuggestions calls open dialog command. Are you doing the same in your Plugin? Maybe you can send a link to your MR?

Yeah, my issue is referenced above, see ✨ Extends field widget action support for a list of suggestions Active

I have even tried the PromptContentSuggestion in the MR above, and it doesn't show any dialog or suggestions either. It sends the ajax request, receives the commands, but nothing happens.

I've tried on Drupal 11.2 with Gin and Claro, and I've also tried with Drupal 10.5 with Gin and Claro. Nothing visible is happening at all.

Ok, with clean Drupal installation I found the problem. The dialog library was missing. For some reason I thought that ajax command adds the library on its own. Please check the updated MR.

Yeah, that fixed it, thank you. It doesn't look nice in Claro, but that's a different issue. I've seen other reports that the dialogue styling in Claro has a problem. With Gin it works and looks nice.

It also works with my ECA implementation, amazing :-)

Now, I have a suggestion, though. The following code in the ajax callback of every implementations should probably contain this:

    // Get the triggering element, as it contains the settings.
    $triggering_element = $form_state->getTriggeringElement();
    $array_parents = $triggering_element['#array_parents'];
    array_pop($array_parents);
    $array_parents[] = static::FORM_ELEMENT_PROPERTY;
    $target_element = NestedArray::getValue($form, $array_parents);
    $selector = $target_element ? $target_element['#attributes']['data-drupal-selector'] : '';

This tends to become a lot of redundant code and I wonder if this couldn't be prepared in the base class already, and the likely to be required values being passed on to the implementation.

Also, there are plenty of warnings like the following in the watchdog, this may require some tweaking in the MR:

Warning: Undefined array key "display_on_focus" in Drupal\ai_content_suggestions\Plugin\FieldWidgetAction\PromptContentSuggestion->buildConfigurationForm() (line 186 of /var/www/html/web/modules/contrib/ai/modules/ai_content_suggestions/src/Plugin/FieldWidgetAction/PromptContentSuggestion.php) 

Last but not least, I wonder about error handling. The PromptContentSuggestion returns a string with the error message if no suggestion could be retrieved. That means that this single string will be populated to the selector, which may be unnoticed by the user. I think, messages should be handled differently.

The same goes for messages that Drupal outputs during that ajax request. An example is if the FWA is attached to the title field of the node. If you click the suggestion button with that field being empty, the form validation emits an error message that the field is required. But that message isn't displayed, only after another page reload. I think, there are 2 things that should follow from this:

1. Form validation should be disabled for the ajax request. I think there is an optional argument for ajax requests to achieve this.
2. The ajax response should collect the messages from the messenger service and add message commands the response to display them right in time to the user as well.

However, the general approach in the MR is great and I really like it a lot.

I was thinking about the triggering element as well. It would be nice of course to delegate this to the base class. Need to find out the best approach though.

Regarding required fields, there is a core issue 🐛 Suppress validation of required fields on ajax calls Needs work . The problem with limit validation errors parameter that is now available, it will disable any kind of validation that is not listed, and this of course is not know to Field Widget Actions. So I guess we will have to wait for the core issue to be fixed.

I will check the warning regarding display_on_focus, this was recently added and there is a default value in the configuration of the plugin, strange that it was not picked up.

Switching to the correct tag

I have fixed the problem with display_on_focus setting. Added the collection of messages that should be displayed to the user. And finally added the helper method for getting the suggestion target selector.

Also I have updated the documentation. Now it includes information on how to create your own field widget action plugin.

@a.dmitriiev this is moving forward really great. I just have 1 further suggestion and 1 bug:

The bug is that if we use returnSuggestions and provide a string as the $suggestions then that string is not selectable in the dialogue. When we provide an array, though, then the items can be selected. I think this can be resolved by turning the single string into an array in returnSuggestions.

And my suggestion for one more improvement is this, that the following code is contained in all ajax callback of all the implementing classes:

    // Get the triggering element, as it contains the settings.
    $triggering_element = $form_state->getTriggeringElement();
    $array_parents = $triggering_element['#array_parents'];
    array_pop($array_parents);
    $array_parents[] = $this->formElementProperty;
    $key = $array_parents[2] ?? 0;
    $form_key = $array_parents[0];

This is only to get $key and $orm_key. Maybe we could provide methods like getFieldName() and getFieldKey()? That could avoid a lot of already existing redundancy and make it another step easier for developers.

I have added more methods to base class and also described them in documentation. Also addressed other comments. Please review again.

This is amazing now, great work @a.dmitriiev

Setting it to RTBC.

To show people how much this can simplify the implementation, here is the diff for the ECA implementation of the FWA: https://git.drupalcode.org/project/eca/-/merge_requests/552/diffs - and to see the full file for the implementation, please have a look at https://git.drupalcode.org/project/eca/-/blob/c2fb81e2684dc58ee1cf5fbf1c...

@a.dmitriiev I saw that you're going to present about FWA at DrupalCamp Ruhr. Do you want to also show the ECA implementation for this? I could easily create a recipe that contains a couple of examples for you.

Thanks Artem! The code looks great and I have tested so the Content Suggestions module works as well.

I have added four comments - the missing key we should fix before merging, since its quite visible.

This looks good to me, but I haven't tested the content suggestion part of it.

When it comes to the missing delta from the core API, wouldn't it still make sense to fall back to integer zero instead of a NULL? Not a blocking question, just curious.

Good for RTBC from here, leaving it at NR for @marcus_johansson

@jurgenhaas regarding delta, it actually depends on how you want to use it. If the delta is needed to get the button element or the form element from the form array, then 0 will be wrong value here, as in the array parents tree, the element will not be found then, but if it is NULL then you can assume, that it is not under any 0, 1, 2, 3, ... element, but just inside the widget parent.

Thanks for clarifying this @a.dmitriiev, makes total sense now.