Rename translator settings form element ID in TranslatorForm

Also terminology is unclean here. Twice settings in one form is odd.

From looking at the code:

    $form['settings'] = array(
      '#type' => 'details',
      '#open' => TRUE,
      '#title' => t('Translator settings'),
      '#tree' => TRUE,
    );
    $form['settings']['auto_accept'] = array(
      '#type' => 'checkbox',
      '#title' => t('Auto accept finished translations'),
      '#description' => t('This skips the reviewing process and automatically accepts all translations as soon as they are returned by the translation provider.'),
      '#default_value' => $entity->getSetting('auto_accept'),
      '#weight' => -1,
    );
    if (!Element::children($form['settings'])) {
      unset($form['settings']);
    }

The last check with unset never happens because of the auto_accept situation.

I think we should simply drop the settings term in the TranslatorForm level and only use that term for the plugin.
Raising prio since we are changing the form structure with this issue.

Updating the issue summary to be more clear.

Uploaded patch for issue #2569343.

Previously uploaded empty patch file: re-uploading patch for issue #2569343.

We still have two items with XYZ Settings title as a result.

I would still drop "Translator settings" completely around the auto accept checkbox. Everything on this form is kindof a setting.
And only introduce a plugin specific settings wrapper / title.

The problem with auto_accept and the settings element there is that we do currently store auto_accept within settings in the translator.

I agree that we should try to get rid of the translator settings wrapper completely, we should never have had two of them. That happened somehow during the conversion to plugins I guess.

However, to do that, this gets a bit more complicated. We need to change a number of places where we define/access auto_accept:

* The schema
* It needs to become a property in the Translator class, with methods in TranslatorInterface and Translator to access it (isAutoAccept()?)
* Search for uses of getSetting('auto_accept') and replace it with the new method.

Sorry, I have misunderstood the task, my bad.
I have already asked to @mbovan, but to be sure I post here my draft-solution and doubts.

I have added:

• the auto_accept property and its method definition in the Translator class
• isAutoAccept() prototype in TranslatorInterface header
• replaced getSetting('auto_accept') with isAutoAccept() in TranslatorForm and JobItem
• and dropped the whole $form['settings'] and $form['settings']['auto_accept'] element, keeping only $form['auto_accept']

I'm still not clear about these 2 sentences:

• @miro_dietiker at comment #9 said: "introduce a plugin specific settings wrapper / title."
• Isn't this plugin wrapper already defined as $form[‘plugin_wrapper’] in TranslatorForm?
• @Berdir at comment #10 said: "We need to change a number of places where we define/access auto_accept"
• Are you asking for a setter method for auto_accept (setAutoAccept($value)?) ?

Updated patch:

• added setAutoAccept($value) method in TranslatorInterface and Translator
• and replaced all setSetting('auto_accept', TRUE) with the new setter method

I'm still not sure what @miro_dietiker means.

Nice, this looks pretty good to me. Also tested the UI, looks a lot better now.

Miro basically said the same as I did, just not as specific. So you have his feedback covered with that patch.

Just some minor coding standard and documentation issues left, assuming the patch will pass:

1. +++ b/src/Entity/Translator.php
   @@ -107,6 +107,13 @@ class Translator extends ConfigEntityBase implements TranslatorInterface {
   +   * @var boolean
   

   for @var and @type types, it's just bool, not boolean (Also just int and not integer).

2. +++ b/src/Entity/Translator.php
   @@ -190,6 +197,15 @@ class Translator extends ConfigEntityBase implements TranslatorInterface {
    
   +  public function isAutoAccept() {
   +      return $this->auto_accept;
   +  }
   +
   +  public function setAutoAccept($value) {
   

   Needs the @inheritdoc blocks just like on the methods around those two.

3. +++ b/src/TranslatorInterface.php
   @@ -65,6 +65,25 @@ interface TranslatorInterface extends ConfigEntityInterface {
   +   * Sets a definition setting.
   

   Copied from setSetting() I guess, should be more specific.

4. +++ b/src/TranslatorInterface.php
   @@ -65,6 +65,25 @@ interface TranslatorInterface extends ConfigEntityInterface {
   +   * @return static
   +   *   The object itself for chaining.
   

   I guess this is copied from setSetting(), you can do just return $this, no description needed. (Don't change existing places, just make sure yours is correct).

Ah, I think what you are missing is adding that property to the config_export list in the Translator annotation.

Yeah nice, just some extra nitpicking:

+++ b/config/schema/tmgmt.schema.yml
@@ -31,6 +31,9 @@ tmgmt.translator.*:
+      label: Automatically accept

@@ -47,7 +50,7 @@ tmgmt.translator.*:
-  label: View translation jobs'
+  label: 'View translation jobs'

One you change an existing label to add the quotes, and your own new label still lacks quotes. Consistency. :-)

Updated patch:

• solved all minor coding standard and documentation issues @Berdir mentioned in #16 and #17
• adjusted labels quotes as @miro_dietiker mentioned in #18 :)

+++ b/config/schema/tmgmt.schema.yml
@@ -43,15 +46,15 @@ tmgmt.translator.*:
 tmgmt.translator_base:
   type: config_entity
   mapping:
     auto_accept:
       type: boolean
-      label: Automatically accept
+      label: 'Automatically accept'

We no longer need auto_accept in here as we moved it away. This will result in an empty mapping, which is fine.

while touching this, the type here is wrong, should be type: mapping, so the results looks like this:

tmgmt.translator:base:
  type: mapping
  mapping: { }

When doing iterative changes on a page, always provide an interdiff: https://www.drupal.org/documentation/git/interdiff. Then we don't have to read the full page again to check your changes.

Updated file interdiff:

• modified tmgmt.translator_base type in tmgmt.schema

Thank you, I was wondering whether this has to be removed or not.

The patch looks good.

However, several tests are failing on schema errors.

I suppose that we need to update TranslatorPluginBase::defaultSettings() and remove the auto_accept from there as it is not part of the settings any more.

Also, you can check if tests are passing locally before uploading the patch. All you need to do is to enable Testing module from Extend tab in Drupal administration. Then, you can access Testing module by clicking on Configuration > Testing. From there, you can run all TMGMT tests by checking that option or you can run specific test you need.
Happy testing! :)

Updated file interdiff:

• fixed default value in TranslatorPluginBase and LocalTranslator
• modified tmgmt.translator.settings.local type in tmgmt_local.schema

Thanks for the advice! :)

Yay, fixed some core change separately and committed the patch.
Looks much cleaner now!