Problem/Motivation

The local translator has a checkout setting:
"Allow translations for enabled languages even if no user has the necessary abilities."

If this is allowed or not should be a global setting, not a translator checkout setting.
Currently one translator instance might have it enabled and the other not.
The submitting user should simply follow the global configuration.

Proposed resolution

Move the setting to the global settings page.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#9 move_the_local-2668386-9.patch5.04 KBthenchev
#9 interdiff-2668386-9.txt886 bytesthenchev
#7 interdiff-2668386-7.txt2.26 KBthenchev
#7 move_the_local-2668386-7.patch5.37 KBthenchev
#5 move_the_local-2668386-5.patch4.89 KBthenchev

Comments

miro_dietiker created an issue. See original summary.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

Stumbled upon this while review.

It has an additional setting now:
- Use admin theme in local translator
No idea how this could be asked per translator configuration.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

It seems the admin theme setting is really saved per translator and additionally as a global setting.
So there's redundancy with this setting.

thenchev’s picture

thenchev commented
Assigned: Unassigned » thenchev

Starting with this.

thenchev’s picture

thenchev commented
Status: Active » Needs review
StatusFileSize
new move_the_local-2668386-5.patch4.89 KB

Initial patch.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work

Direction is fine, feedback on some smaller things below.

  1. +++ b/translators/tmgmt_local/config/schema/tmgmt_local.schema.yml
@@ -5,11 +5,6 @@ tmgmt_local.settings:
       label: 'Use admin theme'
-
-tmgmt.translator.settings.local:
-  type: tmgmt.translator_base
-  label: 'Drupal user settings'
-  mapping:

    I'm surprised this works, I would have expected we still need an empty transalator definition? 

  2. +++ b/translators/tmgmt_local/src/Tests/LocalTranslatorTest.php
@@ -567,8 +567,7 @@ class LocalTranslatorTest extends TMGMTTestBase {
     // Now enable the setting.
-    $translator->setSetting('allow_all', TRUE);
-    $translator->save();
+    \Drupal::configFactory()->getEditable('tmgmt_local.settings')->set('allow_all', TRUE)->save();
     /** @var Job $job */

    You can use $this->config() in tests. 

  3. +++ b/translators/tmgmt_local/tmgmt_local.module
@@ -162,6 +167,7 @@ function tmgmt_local_form_tmgmt_settings_form_submit($form, FormStateInterface $
     $config->set('use_admin_theme', $form_state->getValue('use_admin_theme'))->save();
     \Drupal::service('router.builder')->rebuild();
   }
+  $config->set('allow_all', $form_state->getValue('allow_all'))->save();
 }

    You don't have to call save() in the if anymore since we always do it below.

thenchev’s picture

thenchev commented
Status: Needs work » Needs review
StatusFileSize
new move_the_local-2668386-7.patch5.37 KB
new interdiff-2668386-7.txt2.26 KB

Addressed #6

Status: Needs review » Needs work

The last submitted patch, 7: move_the_local-2668386-7.patch, failed testing.

thenchev’s picture

thenchev commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2668386-9.txt886 bytes
new move_the_local-2668386-9.patch5.04 KB

Test fails without the "save". I think this should pass now.

  • Berdir committed 74b92ca on 8.x-1.x authored by Denchev 
    Issue #2668386 by Denchev: Move the local translator setting to global...
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Fixed

Ah, the save is needed because the rebuild then accesses it again and needs the value to be saved. I guess we could refactor it to not save but not really worth it.

Committed, thanks.

Status: Fixed » Closed (fixed)

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