Problem/Motivation

In some cases the plugins need to add validations for the settings form, maybe to validate that the current mapping source key %KEY exists or to check the api-key is valid or for any other setting that can add a plugin.

Proposed resolution

We need to modify the TranslatorPluginUiInterface to add a method called pluginSettingsFormValidate() that the plugins need to implement to validate the settings form.

Remaining tasks

User interface changes

API changes

CommentFileSizeAuthor
#11 interdiff-allow_the_plugins_to-2568887-7-11.txt3.9 KBedurenye
#11 allow_the_plugins_to-2568887-11.patch8.54 KBedurenye
#7 interdiff-allow_the_plugins_to-2568887-5-7.txt797 bytesedurenye
#7 allow_the_plugins_to-2568887-7.patch8.55 KBedurenye
#5 interdiff-allow_the_plugins_to-2568887-2-5.txt8.24 KBedurenye
#5 allow_the_plugins_to-2568887-5.patch7.97 KBedurenye
#2 allow_the_plugins_to-2568887-2.patch2.53 KBedurenye

Comments

edurenye created an issue. See original summary.

edurenye’s picture

edurenye commented
Status: Active » Needs review
StatusFileSize
new allow_the_plugins_to-2568887-2.patch2.53 KB

Done, tested manually.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Priority: Normal » Major
Status: Needs review » Needs work

Hmm... Would we want to implement PluginFormInterface() then?
Promoting to major since it changes APIs.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

See also how this is done on Monitoring / SensorForm::validateForm()

edurenye’s picture

edurenye commented
Status: Needs work » Needs review
StatusFileSize
new allow_the_plugins_to-2568887-5.patch7.97 KB
new interdiff-allow_the_plugins_to-2568887-2-5.txt8.24 KB

Ok, modified this, now means that I should open issues for each of the plugins.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Needs review » Needs work

I think that's the right direction.
Yes, please open issues in the current reference plugins such as tmgmt_mygengo, tmgmt_google, tmgmt_microsoft.

+++ b/src/TranslatorPluginUiBase.php
@@ -98,4 +85,28 @@ class TranslatorPluginUiBase extends ComponentPluginBase implements TranslatorPl
+  public function buildConfigurationForm(array $form, FormStateInterface $form_state) {
...
+  public function validateConfigurationForm(array &$form, FormStateInterface $form_state) {
...
+  public function submitConfigurationForm(array &$form, FormStateInterface $form_state) {

Interface declaration missing for the base class.

edurenye’s picture

edurenye commented
Status: Needs work » Needs review
StatusFileSize
new allow_the_plugins_to-2568887-7.patch8.55 KB
new interdiff-allow_the_plugins_to-2568887-5-7.txt797 bytes

Added the interface declaration.

I already made the followup's for the plugins #2572623: Add validations for the settings form, #2572625: Add validations for the settings form and #2211889: Authentication error to bing service dies silently at UI.
In the last one, I find this issue about add validation, for tmgmt_microsoft and I put the patch there thinking that was for D8, then I realised that was for D7, I asked there is that issue should be changed to D8 or add another issue for D8. The second option means that this issue should be backported to D7 to be able to add the validation.

juanse254’s picture

juanse254 commented
Status: Needs review » Reviewed & tested by the community

Tested locally and this apparently fixes the tests, setting to RTBC.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Reviewed & tested by the community » Needs work 
  1. +++ b/src/Form/TranslatorForm.php
@@ -156,7 +156,8 @@ class TranslatorForm extends EntityForm {
+      $form['plugin_wrapper']['settings'] += $plugin_ui->buildConfigurationForm($form['plugin_wrapper']['settings'], $form_state);

    This looks so logic, to only pass in and merge the settings form. However, when the plugin needs ajax, it still needs to know its form location.

    I would recommend to pass and merge the whole form so plugins are fully flexible. 

  2. +++ b/src/TranslatorPluginUiBase.php
@@ -98,4 +85,28 @@ class TranslatorPluginUiBase extends ComponentPluginBase implements TranslatorPl
+  public function buildConfigurationForm(array $form, FormStateInterface $form_state) {
+    if (!Element::children($form)) {
+      $form['#description'] = t("The @plugin plugin doesn't provide any settings.", array('@plugin' => $this->pluginDefinition['label']));

    Interesting. So derived plugins call the parent short before returning.. Usually we call the parent as the first step.

    I guess i would move the message creation to the caller TranslatorForm. Output if settings are still empty after calling the plugin_ui.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

1. That's a standard behavior for forms like this. Let's not solve this here, there are core issues to address this then we can make it better. Ajax doesn't go through that anyway, it would call another submit and that would receive everything.

edurenye’s picture

edurenye commented
Status: Needs work » Needs review
StatusFileSize
new allow_the_plugins_to-2568887-11.patch8.54 KB
new interdiff-allow_the_plugins_to-2568887-7-11.txt3.9 KB

I did whats in the point 2.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community

Looks fine for me.

  • Berdir committed 6f43824 on 8.x-1.x authored by edurenye 
    Issue #2568887 by edurenye: Allow the Plugins to add validations for the...
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Reviewed & tested by the community » Fixed

Looks fine to me too

I'm not sure if we should keep the separate UI classes. The API around them is very weird, but we'd also end up with huge classes.

Status: Fixed » Needs work

The last submitted patch, 11: allow_the_plugins_to-2568887-11.patch, failed testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Fixed
miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

Yeah i also thought about that, the UI pattern is strange.
Every time i review the code around the review UI, i need a few minutes to understand all the wiring.
I think there won't be multiple UI implementations for the same translator, ever.
I guess it needs a dedicated issue to discuss...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.