Files: 
CommentFileSizeAuthor
#42 drupal-1998088-42.patch6.47 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 58,904 pass(es).
[ View ]
#42 interdiff.txt1.43 KBParisLiakos
#40 drupal-1998088-40.patch6.46 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 58,485 pass(es).
[ View ]
#38 drupal-1998088-19.patch6.47 KBcatch
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1998088-19_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#34 32-34-interdiff.txt1.31 KBalexpott
#34 1998088.34.patch9.72 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 59,240 pass(es).
[ View ]
#32 1998088.32.patch9.18 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 59,300 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#32 interdiff-27-32.txt1.79 KBalexpott
#30 1998088.27.patch7.88 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] 58,912 pass(es), 0 fail(s), and 4 exception(s).
[ View ]
#27 24-27-interdiff.txt3.96 KBalexpott
#27 1998088.27.patch7.89 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] Setup environment: failed to create checkout database.
[ View ]
#24 19-24-interdiff.txt5.03 KBalexpott
#24 drupal-1998088-24.patch7.17 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 58,824 pass(es).
[ View ]
#19 drupal-1998088-19.patch6.47 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 57,074 pass(es).
[ View ]
#19 interdiff.txt967 bytesdawehner
#15 Convert_locale_translate_english_variabletoCMI-1998088-15.patch5.52 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 57,084 pass(es).
[ View ]
#15 interdiff.txt913 bytesParisLiakos
#13 Convert_locale_translate_english_variabletoCMI-1998088-13.patch5.52 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] 58,072 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#13 interdiff.txt875 bytesParisLiakos
#11 Convert_locale_translate_english_variabletoCMI-1998088-11.patch4.66 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] 58,185 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#11 interdiff.txt3.83 KBParisLiakos
#7 Convert_locale_translate_english_variabletoCMI-1998088-7.patch2.76 KBplopesc
PASSED: [[SimpleTest]]: [MySQL] 58,003 pass(es).
[ View ]
#5 Convert_locale_translate_english_variabletoCMI-1998088-5.patch2.74 KBebrowet
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch Convert_locale_translate_english_variabletoCMI-1998088-5.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

dawehner’s picture

Issue tags:+Novice, +CMI

Adding tags

ebrowet’s picture

Assigned:Unassigned» ebrowet

hello,
i look at this ;)
i 've got correction, the time to test the patch and i post it ;)
i make correction on my patch (only space change option ) and it's goog ;)

ParisLiakos’s picture

@ebrowet: still working on this?

ebrowet’s picture

yes i still work on this ;).
comming soon

ebrowet’s picture

StatusFileSize
new2.74 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch Convert_locale_translate_english_variabletoCMI-1998088-5.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Hello,
I create the patch, i hope is good for you. If not tell me i recreate it ;)
Have a nice day
Eric

ParisLiakos’s picture

Status:Active» Needs work

Great! thanks looks good:)
Some points:

+++ b/core/modules/locale/locale.moduleundefined
@@ -818,7 +818,8 @@ function locale_form_language_admin_edit_form_alter(&$form, &$form_state) {
+    //config('locale.settings')->set('translation.path', $path)->save();

we can delete that:)

+++ b/core/modules/locale/locale.moduleundefined
@@ -828,7 +829,7 @@ function locale_form_language_admin_edit_form_alter_submit($form, $form_state) {
+    return config('locale.settings')->get('translate_english');

this has two extra spaces in front

Also, we should provide an upgrade path using update_variable_to_config inside a hook_update_N in locale.install

plopesc’s picture

Status:Needs work» Needs review
StatusFileSize
new2.76 KB
PASSED: [[SimpleTest]]: [MySQL] 58,003 pass(es).
[ View ]

Hello

Patch in #5 doesn't apply now. Re-rolling

I think that is not necessary use update_variable_to_config, given that this variable doe not exist in D7.

Regards

ebrowet’s picture

it was my issue, I did not have too much time these last time to fix my patch
too bad, it'll be for next time
sincerely,
Eric

ParisLiakos’s picture

Status:Needs review» Reviewed & tested by the community

yeah i grepped 'locale_translate_english' in D7 and i didnt find anything..so update path is not needed. lets get this in.
thanks both @ebrowet and @plopesc

Dries’s picture

Shouldn't this use Drupal::config() instead of config()?

ParisLiakos’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new3.83 KB
new4.66 KB
FAILED: [[SimpleTest]]: [MySQL] 58,185 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

yeap, i missed that
injected it to LocaleTranslation as well

Status:Needs review» Needs work

The last submitted patch, Convert_locale_translate_english_variabletoCMI-1998088-11.patch, failed testing.

ParisLiakos’s picture

Status:Needs work» Needs review
StatusFileSize
new875 bytes
new5.52 KB
FAILED: [[SimpleTest]]: [MySQL] 58,072 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, Convert_locale_translate_english_variabletoCMI-1998088-13.patch, failed testing.

ParisLiakos’s picture

Status:Needs work» Needs review
StatusFileSize
new913 bytes
new5.52 KB
PASSED: [[SimpleTest]]: [MySQL] 57,084 pass(es).
[ View ]

facepalm?

Status:Needs review» Needs work
Issue tags:-Novice, -CMI

The last submitted patch, Convert_locale_translate_english_variabletoCMI-1998088-15.patch, failed testing.

ParisLiakos’s picture

Status:Needs work» Needs review
jibran’s picture

dawehner’s picture

StatusFileSize
new967 bytes
new6.47 KB
PASSED: [[SimpleTest]]: [MySQL] 57,074 pass(es).
[ View ]
+++ b/core/modules/locale/tests/Drupal/locale/Tests/LocaleTranslationTest.phpundefined
@@ -46,7 +46,7 @@ protected function setUp() {
+    $translation = new LocaleTranslation($this->storage, $this->cache, $this->lock, $this->getConfigFactoryStub(array()));

I am sorry but this is not the right fix.

ParisLiakos’s picture

nice idea! definitely much bettter:)
shouldnt we remove the empty array() argument now from
$translation = new LocaleTranslation($this->storage, $this->cache, $this->lock, $this->getConfigFactoryStub(array()));

Jose Reyero’s picture

Sorry to show up a bit late here but I think what we'd need is the 'locale_translatable_language_list' in the configuration. And if that's ok, then 'locale_translate_english' would be just one one element of this array. My reasons:

- Other locale components do need that full list injected into DIC somehow. Right now the only way of having it without having a dependency on locale module fully loaded is duplicating the code. See the patch here #1966538: Translation is not updated for configuration, when the config changes

+class LocaleConfigManager {
+
............................
+  /**
+   * Returns list of translatable languages.
+   *
+   * @return array
+   *   Array of installed languages keyed by language name. English is omitted
+   *   unless it is marked as translatable.
+   */
+  public static function getTranslatableLanguages() {
+    $languages = language_list();
+    if (!variable_get('locale_translate_english', FALSE)) {
+      unset($languages['en']);
+    }
+    return $languages;
+  }
+
}

- Since we have that 'locale_translatable_language_list' the fact that LocaleTranslation or LocaleConfigSubscriber don't use that list is an API consistency issue.

So I think this issue would be a good chance to fix this variable in a really useful and more generic way. My intention is to post a patch in a few hours for this provided you guys agree. Otherwise I'll just keep off this one and work on other workarounds for the problem.

Jose Reyero’s picture

mtift’s picture

Assigned:ebrowet» Unassigned
Issue tags:-CMI+Configuration system

Fixing tags.

Also, is there a good reason why you can't just post your patch from #2052233: Ensure list of translatable languages is available early in bootstrap here and close that issue? Having duplicate issues is confusing.

alexpott’s picture

StatusFileSize
new7.17 KB
PASSED: [[SimpleTest]]: [MySQL] 58,824 pass(es).
[ View ]
new5.03 KB

So let's prepare the ground for #2052233: Ensure list of translatable languages is available early in bootstrap but do the minimum to get rid of the variable.

Also added missing upgrade path.

Gábor Hojtsy’s picture

Status:Needs review» Needs work

Upgrade path looks good. The schema for the config is incorrect IMHO though. It is said to be a sequence however its a language code / boolean pair. The way the schema is expressed, this is not an associative list. Otherwise looks good to me.

Jose Reyero’s picture

The patch in #24 looks like a step ahead to me, just that schema thing @Gabor mentions.

alexpott’s picture

Status:Needs work» Needs review
StatusFileSize
new7.89 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: failed to create checkout database.
[ View ]
new3.96 KB

Hmmm... so the thing is if the enable_translation is meant to be become a list of languages then isn't that a sequence.

Here's an idea - how about we change this around and make it a list of languages that have translation disabled.

Tor Arne Thune’s picture

+++ b/core/modules/locale/config/schema/locale.schema.yml
@@ -7,6 +7,12 @@ locale.settings:
+          label: 'Langauge'

Just so it's not overlooked before commit: Typo Langauge -> Language.

Gábor Hojtsy’s picture

Status:Needs review» Needs work

I think it looks good outside of that typo :)

Gábor Hojtsy’s picture

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new7.88 KB
FAILED: [[SimpleTest]]: [MySQL] 58,912 pass(es), 0 fail(s), and 4 exception(s).
[ View ]

Fixed that typo only. I think this one is good to go and can be further refined in #2052233: Ensure list of translatable languages is available early in bootstrap.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 1998088.27.patch, failed testing.

alexpott’s picture

Status:Needs work» Needs review
StatusFileSize
new1.79 KB
new9.18 KB
FAILED: [[SimpleTest]]: [MySQL] 59,300 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Interest test fails due to assumptions made about locale config :)

Status:Needs review» Needs work

The last submitted patch, 1998088.32.patch, failed testing.

alexpott’s picture

Status:Needs work» Needs review
Issue tags:-Novice
StatusFileSize
new9.72 KB
PASSED: [[SimpleTest]]: [MySQL] 59,240 pass(es).
[ View ]
new1.31 KB

Well that was very fun. The WebTestBase:setup() method was writing an incomplete version of locale.settings.yml to the active directory so that the translation.path can be overridden whilst testing.

Jose Reyero’s picture

Looks good to me, but the language check in getStringTranslation() could be simplified. I mean instead of:

+    if ($langcode == Language::LANGCODE_SYSTEM || ($langcode == 'en' && is_null($disable_translation)) || (is_array($disable_translation) && in_array($langcode, $disable_translation))) {
       return FALSE;
     }

We could have:

+    if ($langcode == Language::LANGCODE_SYSTEM || $disable_translation && in_array($langcode, $disable_translation)) {
       return FALSE;
     }

But yes, anyway we get both features for the same price :-)
I mean, other (contrib) modules can just add languages to that list.

However, this may look a bit twisted, but for the follow up patch I'd rather convert the list to a list of 'translatable ones'. One of the points of the other patch is being able to easily pull the list of translatable languages from configuration, we need to iterate over them in a few places, and by having a 'non translatable' list, we need both the full language_list() and the 'non translatable ones' for that. And then we may need that logic duplicated in locale module and in the components that go into DIC for config translation/update if we don't want one depending on the other. The catch is at that point, if we do it in two steps instead of one, that will mean an API change, right?

Gábor Hojtsy’s picture

Status:Needs review» Needs work

Well, in that case, we would be better off listing the translatable languages not the untranslated languages here :)

ianthomas_uk’s picture

I might be totally missing something here, but the comments from #21 onwards seem to be addressing three separate issues.

a) Converting the locale_translating_english variable to CMI.
Drupal is shipped in English and the Locale module allows the user to translate it into their own language. "Translating" from English to English is a bit of a confusing concept, so that ability is hidden using this variable.

b) The ability to easily tell which languages are available for translating INTO, early in the bootstrap and whatever modules are enabled. This can already be calculated by calling language_list() and removing English if appropriate. For good data integrity you should avoid storing something if you can calculate it, so I don't think we should be creating new config for this, but we might want to move locale_translatable_language_list() from locale.module to language.module.
(I'm a bit confused about the dependencies here, it doesn't seem correct to be calling Drupal::config('locale.settings') when you can't call locale_translatable_language_list(), so the proposed variable wouldn't have removed the dependency).

c) The ability to turn translation off for additional languages. Hopefully, once (a) and (b) were fixed there would be no more need to hard code 'en' into if statements, and this could be done entirely inside locale_translatable_language_list().

My suggestion is that this issue returns to the approach in #19 to fix (a). We then address (b) in #2052233: Ensure list of translatable languages is available early in bootstrap and (c) in a further followup if there is a use case for disabling other translations.

catch’s picture

Priority:Normal» Critical
Status:Needs work» Needs review
StatusFileSize
new6.47 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1998088-19_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Yeah I agree with #37. If the only thing that's ever going to be configured is whether English is translatable or not, that should be the only thing in the configuration.

Bumping this to critical since it's one of the very last conversions at the moment And re-uploading #19 for the bot.

Status:Needs review» Needs work

The last submitted patch, drupal-1998088-19.patch, failed testing.

ParisLiakos’s picture

Status:Needs work» Needs review
StatusFileSize
new6.46 KB
PASSED: [[SimpleTest]]: [MySQL] 58,485 pass(es).
[ View ]

lets reroll

dawehner’s picture

  1. +++ b/core/modules/locale/config/locale.settings.yml
    @@ -1,4 +1,5 @@
    cache_strings: true
    +translate_english: 0

    this is a boolean variable so the default value should be false instead.

  2. +++ b/core/modules/locale/locale.module
    @@ -818,7 +818,7 @@ function locale_form_language_admin_edit_form_alter(&$form, &$form_state) {
    +  Drupal::config('locale.settings')->set('translate_english', intval($form_state['values']['locale_translate_english']))->save();

    @@ -828,7 +828,7 @@ function locale_form_language_admin_edit_form_alter_submit($form, $form_state) {
    +  return Drupal::config('locale.settings')->get('translate_english');

    We do add slashes also to Drupal in procedural code.

ParisLiakos’s picture

StatusFileSize
new1.43 KB
new6.47 KB
PASSED: [[SimpleTest]]: [MySQL] 58,904 pass(es).
[ View ]

you are right

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

Thank you!

catch’s picture

Status:Reviewed & tested by the community» Fixed

Committed/pushed to 8.x, thanks!

Gábor Hojtsy’s picture

Issue tags:+D8MI, +language-ui

D8MI tags.

Gábor Hojtsy’s picture

#2052233: Ensure list of translatable languages is available early in bootstrap explores the expansion of this variable (as it was for a while above) to have a full list of translatable things to avoid circular dependencies / allow for earlier caching, etc. See you there. :)

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