Problem/Motivation

Locale module's string translation is a powerful old style API which got bloated through the years with several special cases and new features. There are very specific workarounds in the installer and some hacks to support fixed runtime string overrides. By unifying this system under one API and using that across all systems, we can remove the boundaries between the installer and the runtime and make the translation system properly pluggable.

The current system has various inter-dependencies and is hard to debug, see #1807272: Mixed locale issues with upgrade scripts, unit tests, 'Table simpletest..locales_source doesn't exist', etc... for example.

Proposed solution

Introduce a TranslationInterface that is implemented by both simpler string / file based lookups as well as the "regular" Locale module database lookup. Implement a TranslationFallback wrapper to combine these lookups in order of preferred lookup so fallback between different methods is possible. Finally, use the dependency injection container to define which methods of lookup are to be used. The installer would use the file based lookup, while fully bootstrapped runtime would use the LocaleTranslation lookup (with fallbacks).

This makes it possible to remove st(), get_t() and $t (in a followup at #1838310: Remove st(), get_t() and $t for good to not clutter this issue up) in addition to all the API cleanup and generalization benefits.

Class structure

TranslationInterface classes.png

CommentFileSizeAuthor
#137 1813762-locale_translation_interface-137.patch67.32 KBParisLiakos
#137 interdiff.txt5.19 KBParisLiakos
#136 1813762-locale_translation_interface-136.patch65.69 KBParisLiakos
#129 1813762-locale_translation_interface-129.patch65.75 KBParisLiakos
#129 interdiff.txt757 bytesParisLiakos
#127 interdiff.txt522 bytesParisLiakos
#126 1813762-locale_translation_interface-126.patch65.74 KBParisLiakos
#124 1813762-locale_translation_interface-124.patch64.01 KBParisLiakos
#122 1813762-locale_translation_interface-122.patch65.63 KBParisLiakos
#122 interdiff.txt2.34 KBParisLiakos
#120 1813762-locale_translation_interface-120.patch64.92 KBParisLiakos
#120 interdiff.txt689 bytesParisLiakos
#117 1813762-locale_translation_interface-117.patch64.93 KBParisLiakos
#117 interdiff.txt827 bytesParisLiakos
#115 1813762-locale_translation_interface-115.patch64.91 KBParisLiakos
#115 interdiff.txt1.69 KBParisLiakos
#113 1813762-locale_translation_interface-113.patch64.85 KBParisLiakos
#113 interdiff.txt7.34 KBParisLiakos
#112 1813762-locale_translation_interface-112.patch65.09 KBParisLiakos
#106 1813762-locale_translation_interface-106.patch65.16 KBParisLiakos
#106 interdiff.txt5.04 KBParisLiakos
#96 1813762-locale_translation_interface-96.patch62.76 KBYesCT
#96 interdiff-94-96.txt768 bytesYesCT
#94 1813762-locale_translation_interface-94.patch62.77 KBYesCT
#90 1813762-locale_translation_interface-90.patch62.74 KBYesCT
#90 interdiff-88-90.txt740 bytesYesCT
#88 1813762-locale_translation_interface-88.patch62.57 KBYesCT
#88 interdiff-83-88.txt536 bytesYesCT
#83 1813762-locale_translation_interface-83.patch62.44 KBParisLiakos
#79 1813762-locale_translation_interface-79.patch61.81 KBParisLiakos
#79 interdiff.txt1.15 KBParisLiakos
#74 1813762-locale_translation_interface-74.patch60.66 KBParisLiakos
#69 1813762-locale_translation_interface-69.patch44.61 KBJose Reyero
#69 1813762-locale_translation_interface-69-diff.txt109.35 KBJose Reyero
#66 1813762-locale_translation_interface-66.patch125.25 KBParisLiakos
#66 interdiff.txt11.57 KBParisLiakos
#65 1813762-locale_translation_interface-65.patch125.02 KBParisLiakos
#65 interdiff.txt1.64 KBParisLiakos
#63 1813762-locale_translation_interface-62.patch124.15 KBParisLiakos
#63 interdiff.txt4.55 KBParisLiakos
#61 1813762-locale_translation_interface-60.patch124.18 KBParisLiakos
#61 interdiff.txt1.09 KBParisLiakos
#58 1813762-locale_translation_interface-58.patch123.09 KBParisLiakos
#52 1813762-locale_translation_interface-52.patch123.07 KBJose Reyero
#45 1813762-locale_translation_interface-45.patch32.84 KBJose Reyero
#44 1813762-locale_translation_interface-44.patch34.35 KBJose Reyero
#28 TranslationInterface classes.png46.24 KBGábor Hojtsy
#26 interdiff.txt1.52 KBattiks
#25 locale_translation_interface-25.patch34.83 KBJose Reyero
#22 locale_translation_interface-22.patch33.16 KBJose Reyero
#22 locale_translation_interface-22-interdiff.txt2.02 KBJose Reyero
#14 locale_translation_interface-14.patch34.01 KBJose Reyero
#9 1813762-locale_translation_interface-9.patch20.85 KBjapicoder
#5 locale_translation_interface-02.patch29.21 KBJose Reyero
locale_translation_interface-01.patch20.15 KBJose Reyero
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Issue tags: +sprint
+++ b/core/includes/bootstrap.incundefined
@@ -1487,7 +1487,6 @@ function bootstrap_hooks() {
 function t($string, array $args = array(), array $options = array()) {

@@ -1497,19 +1496,24 @@ function t($string, array $args = array(), array $options = array()) {
+  try {
+    $translation = drupal_container()->get('string.translation')->translate($options['langcode'], $string, $options['context']);
+    $string = $translation === FALSE ? $string : $translation;
   }
-  // Custom strings work for English too, even if locale module is disabled.
-  if (isset($custom_strings[$options['langcode']][$options['context']][$string])) {
-    $string = $custom_strings[$options['langcode']][$options['context']][$string];
+  catch (Exception $e) {
+    // Register a foo translation service just in case a translation is
+    // attempted before language initialization. This one will cache strings
+    // if we want to see them later.
+    // Add basic translation system using files if this is an installation.
+    if (drupal_installation_attempted()) {
+      drupal_container()->register('string.translation', 'Drupal\Core\Translation\FileTranslation');
+    }
+    else {
+      drupal_container()->register('string.translation', 'Drupal\Core\Translation\StaticTranslation');
+    }

Did you do any performance testing of this? It looks bulletproof and pretty slow (gut feel, not scientific).

+++ b/core/includes/bootstrap.incundefined
@@ -2642,13 +2646,9 @@ function drupal_installation_attempted() {
 function get_t() {
-  static $t;
-  // This is not converted to drupal_static because there is no point in
-  // resetting this as it can not change in the course of a request.
-  if (!isset($t)) {
-    $t = drupal_installation_attempted() ? 'st' : 't';
-  }
-  return $t;
+  // Use always t() which will work nicely on any situation.
+  // @todo Remove all this logic, maybe keep st() to mark installer strings.
+  return 't';

+++ b/core/includes/install.incundefined
@@ -729,48 +729,8 @@ function drupal_requirements_url($severity) {
 function st($string, array $args = array(), array $options = array()) {
-  static $strings = NULL;
-  global $install_state;
-
-  if (empty($options['context'])) {
-    $options['context'] = '';
-  }
-
-  if (!isset($strings)) {
-    $strings = array();
-    if (isset($install_state['parameters']['langcode'])) {
-      // If the given langcode was selected, there should be at least one .po
-      // file with its name in the pattern drupal-$version.$langcode.po.
-      // This might or might not be the entire filename. It is also possible
-      // that multiple files end with the same suffix, even if unlikely.
-      $files = install_find_translation_files($install_state['parameters']['langcode']);
-      if (!empty($files)) {
-        // Register locale classes with the classloader. Locale module is not
-        // yet enabled at this stage, so this is not happening automatically.
-        drupal_classloader_register('locale', drupal_get_path('module', 'locale'));
-        $strings = Gettext::filesToArray($install_state['parameters']['langcode'], $files);
-      }
-    }
-  }
-
-  require_once DRUPAL_ROOT . '/core/includes/theme.inc';
-  // Transform arguments before inserting them
-  foreach ($args as $key => $value) {
-    switch ($key[0]) {
-      // Escaped only
-      case '@':
-        $args[$key] = check_plain($value);
-        break;
-      // Escaped and placeholder
-      case '%':
-      default:
-        $args[$key] = '<em>' . check_plain($value) . '</em>';
-        break;
-      // Pass-through
-      case '!':
-    }
-  }
-  return strtr((!empty($strings[$options['context']][$string]) ? $strings[$options['context']][$string] : $string), $args);
+  // @todo Remove or keep as a t wrapper to mark config strings..?
+  return t($string, $args, $options);

I think if we can prove the file translator can nicely fit in a 500k-800k .po file in memory for the lookups (which the D7 translation instructions actually instruct people to do), we might get away removing this special case, which would be a great cleanup.

Only, if we can keep the 500k-800k .po's file data representation in memory that is.

andypost’s picture

I think this should be registered in container much early so there no need to try..catch

Jose Reyero’s picture

@andypost,
Right, I realize I have overlooked the place where it should be, that is inside drupal_container() function, that should at least provide some default translator, though it can be an empty one.
The installer one (files) should be possibly in install.inc and StringOverrides should be registered in CoreBundle.php

@Gábor Hojtsy,
Nope, no performance tests yet, I don't know how fast is drupal_container()->get('string.translation') but on the other side it's saving a lot of logic inside t().

As a side note I think handling one time exceptions should be much faster than double checking for conditions every time, though I'd like to see some benchmarks about it.

Gábor Hojtsy’s picture

Issue tags: +language-ui

Adding language-ui tag.

Jose Reyero’s picture

Status: Needs work » Needs review
FileSize
29.21 KB

New version of the patch (Moved around too many things so I don't think an inter-diff will be useful):
- Proper initialization of DIC locale translation objects so no need to try-catch anymore in t()
- Cleaned up file translation methods and functions, now FileTranslation is mostly self contained.
- Since contrib modules can now use DIC replacement, no need for 'reset/get all translations' methods in locale translation. Simplified LocaleLookup too. Localization client should be able to inject its own translator and get whatever data it needs from page strings.
- Using st() as a t() wrapper that only adds the install language in.
- Moved locale reset from locale module into the TranslationInterface objects.

To do / Questions:
- Do we need to mark installer strings? If we use the full translation po file for the installer I don't think we need this and we could drop both st() and get_t() entirely. We would need to make drupal_language_init() installer aware though.
- There's still the issue of JavaScript strings being regenerated (locale_js_alter) before the module is properly initialized for which I've added some try-catch in locale_storage(). I think this is an issue with core module loading though we need a workaround in the meanwhile (Module hook invoked before the DIC is properly initialized)

Status: Needs review » Needs work

The last submitted patch, locale_translation_interface-02.patch, failed testing.

Gábor Hojtsy’s picture

Issue tags: +feature freeze

Add feature freeze tag.

japicoder’s picture

Assigned: Unassigned » japicoder

The last patch does not apply. I take it and try to improve.

japicoder’s picture

Patch reroll. I've been testing this, but it's not working actually. The problem is that the Container is not able to find the new Locale interface, but it's defined in drupal_container(). I can't figure why it's not working.

Jose Reyero’s picture

Status: Needs work » Needs review

Moved to 'needs review' to trigger testbot.

About the DIC issues, the initialization of the container is a bit of a mess atm (in Drupal core I mean) and you need to initialize it in a few different places for it to work in all cases. (drupal_container, install.inc, CoreBundle.php...)
Also since it is used before full bootstrap, and even before language is initializaed, because of file_get_stream_wrappers(), this case is still harder. See _drupal_bootstrap_code(), _drupal_bootstrap_full().

Status: Needs review » Needs work
Issue tags: -D8MI, -sprint, -language-ui, -feature freeze

The last submitted patch, 1813762-locale_translation_interface-9.patch, failed testing.

webflo’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +D8MI, +sprint, +language-ui, +feature freeze

The last submitted patch, 1813762-locale_translation_interface-9.patch, failed testing.

Jose Reyero’s picture

Status: Needs work » Needs review
FileSize
34.01 KB

About the reroll in #9, it is missing some parts so I'm continuing from the patch #5

The reason why it was failing is because when locale module is disabled, t() still gets called a few times till the container is rebuilt. Since StringDatabaseStorage relies on schema definition to update strings, when the module is disabled (not in module list anymore) we don't have a schema for locale tables. So this is just another one of that edge conditions that make this locale really hard to debug and if we add DIC to the equation it gets still worse.

The solution (in the patch) is to remove the dependency on drupal_get_schema() of the StringDatabaseStorage, which also saves some code from the storage class. The only drawback is that if we change the schema we need to update the storage code but well, that's not hard to do.

Jose Reyero’s picture

Some notes about testing and memory benchmarks for the file translation feature:
- Using a 800 Kb po file (drupal-7.15.es.po) placed in sites/[sitefolder]/files/translations/
- The memory used to load and parse it and get a translation seems to be around 1.8 Mb.

Some code for testing and get a translated string:

$path = variable_get('locale_translate_file_directory', conf_path() . '/files/translations');
$translator =  new Drupal\Core\Translation\FileTranslation($path);
// First test the translator finds the Spanish po file, see above
$files = $files = $translation->findTranslationFiles('es');
// dpm($files) will print an array with the po file path.
 // Any source string contained in the file will do.
$source = 'Delete the selected comments';
$translation = $translator->translate('es', $source , ''); // Translating to Spanish
print $translation;
attiks’s picture

Nice work, this looks good to me, 2 minor remarks:

+++ b/core/includes/bootstrap.incundefined
@@ -2654,13 +2649,9 @@ function drupal_installation_attempted() {
+  // Use always t() which will work nicely on any situation.
+  // @todo Remove all this logic, maybe keep st() to mark installer strings.

I think it's best to use t() everywhere.

+++ b/core/modules/locale/locale.moduleundefined
@@ -1058,6 +1004,7 @@ function _locale_rebuild_js($langcode = NULL) {
+  //new Drupal\locale\LocaleTranslation();

is the comment needed?

Gábor Hojtsy’s picture

Jose: thanks for the memory testing! How is this compared to the full page memory used? http://api.drupal.org/api/drupal/core%21includes%21bootstrap.inc/constan... says D8 requires 32MB memory.

attiks’s picture

FYI: I did an drupal 8 install using 28MB, it works so we have 4MB for the po files.

Gábor Hojtsy’s picture

Hm, I'm concerned for distros like Drupal Commerce, where there are (a) many modules enabled (b) MUCH MUCH bigger .po file for the distro. The answer to that might be:

- A: on-demand lookup in the .po file without parsing the whole thing (talked about this to Jose before) OR
- B: elevating the memory limit of Drupal in general OR
- C: not producing compound distro .po files on l.d.o so it would properly download individual module files

Probably C is something to consider anyway, since the localization update integration should not import the big overall compound file AND the individual module files as well. BUT this still looks like it could be on the borderline of the memory limit(?) if @attiks data is right.

vasi1186’s picture

After a first review, I did not find anything that would look odd. I did not test the actually functionality, only code review. Just a small thing:

+++ b/core/modules/locale/lib/Drupal/locale/LocaleTranslation.phpundefined
@@ -0,0 +1,73 @@
+    if ($langcode == LANGUAGE_SYSTEM || $langcode == 'en' && !variable_get('locale_translate_english', FALSE)) {
+      return FALSE;

I know this code is correct, because of the operators precedence, but maybe just for an easier reading purpose, we should use brackets... anyway, this should not stop the issue to be rtbc in my opinion.

attiks’s picture

FYI: ini_set('memory_limit', '26M'); works as well.

For distros this is probably going to break, even without this patch. So we probably have to increase the memory limit anyways (or allow the profile to change the limit)

What's left is #16 and #20

Jose Reyero’s picture

Since I see no major objections here to the important pant of the patch (Update to DIC) while on the other side we are far from a definitive solution for whether to completely drop get_t()/st()/$t I think we should move on with the "important thing" let the other parts (that anyway are going to need a major cleanup patch) for later. So:
- Fixed issues found by @attiks #16 and @vasi186 #20
- Moving back get_t() function to its original form (we still need it if we keep 'st' for marking installer strings).

Note also that the 'st' function serves another purpose, that is injecting the installer language to t(), so there's another reason why it is still there.

And I really think we should:
1. Commit this first part which is an important step in bringing locale system up to date with DIC and D8 architecture.
2. Keep the issue at 'needs work' while we decide whether we can safely drop get_t()/st() and add the rest of the clean up that will be a few hundred lines of s/$t/t/ and s/st/t/.

Note: I am not too enthusiastic at all about raising the memory needed by Drupal installer, I think we should find a better solution for that (or keep the current one that is always an option).

attiks’s picture

Status: Needs review » Reviewed & tested by the community

I agree with #22, let's do 'baby steps'

Assuming the bot will like this, this is RTBC

Status: Reviewed & tested by the community » Needs work

The last submitted patch, locale_translation_interface-22.patch, failed testing.

Jose Reyero’s picture

Status: Needs work » Needs review
FileSize
34.83 KB

Updated 3 calls to (obsoleted by this patch) locale_reset() from test.

     // Reset locale cache.
-    locale_reset();
+    drupal_container()->get('locale.translation')->reset();
attiks’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
1.52 KB

Same as #23
I agree with #22, let's do 'baby steps'

Assuming the bot will like this, this is RTBC

Interdiff attached

catch’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: -feature freeze +needs profiling

Overall this looks great, it's refactoring some very old code and bringing it nicely up to date. I have a few questions on it, also I agree with Gabor that this could use performance testing - I'm mainly interested in seeing an xhprof run for a page with lots of t() calls, when the site is in English and has no translations - that's the usual case and the most likely to see a regression since we're currently hyper-optimized for this case. A translated page would be interesting as well but not required. Main thing would be number of function calls before/after, also whether there's any additional database queries (looks like there shouldn't be but would be good to confirm).

A few things came up, most of these are minor:

+++ b/core/includes/bootstrap.incundefined
@@ -2480,6 +2467,14 @@ function drupal_container(Container $new_container = NULL, $rebuild = FALSE) {
+    // Register a minimum translation service just in case a translation is

When can we actually hit this condition? If language service is in the container, then the answer ought to be 'never'.

+++ b/core/includes/install.incundefined
@@ -740,53 +740,23 @@ function drupal_requirements_url($severity) {
 function st($string, array $args = array(), array $options = array()) {

Looks like we can just about get rid of st() now - would it just be necessary for the installer to set the language properly? Not here of course.

+++ b/core/lib/Drupal/Core/Translation/CustomStrings.phpundefined
@@ -0,0 +1,25 @@
+  protected function loadLanguage($langcode) {
+    return variable_get('locale_custom_strings_' . $langcode, array());

Not in this issue but we need to move the custom strings out of $GLOBALS['conf'] to some other namespace and stop using variable_get(). I opened #1833516: Add a new top-level global for settings.php - move things out of $conf last week, although I suppose in this case these could potentially just move to CMI.

+++ b/core/lib/Drupal/Core/Translation/StaticTranslation.phpundefined
@@ -0,0 +1,64 @@
+/**
+ * String translator with a static cache for translations.
+ *
+ * This is a high performance way to provide a handful of string replacements.
+ */

Will this ever be used by itself? Or is it primarily a base class for custom strings and the file translator?

+++ b/core/lib/Drupal/Core/Translation/TranslationFallback.phpundefined
@@ -0,0 +1,61 @@
+/**
+ * String translator with a fallback system.
+ */

We've been calling these 'chains' elsewhere, for example see the cache chain backend. There's also ChainedRouter iirc.

Calling this a Fallback makes me think the class itself is the fallback, not that it has an array of classes to check to enable fallback.

It's great to see the same pattern being used but I think it's worth bringing the terminology into line with other subsystems.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleLookup.phpundefined
@@ -74,13 +74,7 @@ protected function resolveCacheMiss($offset) {
-    // Disabling the usage of string caching allows a module to watch for
-    // the exact list of strings used on a page. From a performance
-    // perspective that is a really bad idea, so we have no user
-    // interface for this. Be careful when turning this option off!
-    if (variable_get('locale_cache_strings', 1)) {
-      $this->persist($offset);

Is this feature no longer needed? I guess you could register a translation service that listens or similar, so yeah, cool that it's no longer needed then ;)

Also just a note that this isn't the sort of patch that is blocked on feature freeze, this is straight refactoring of an existing API to bring it up to date, and doesn't really have any module developer-facing API changes in it. On the other hand it'd be good to get it committed soonish since this looks like it's a strong step towards removing the language bootstrap phase (this has already gone, but 'low level language system dependencies/special casing'.

Gábor Hojtsy’s picture

Note that this allows removal of get_t(), $t() and st() for good, which is a HUGE win IMHO.

Created this class figure, which probably demonstrates we should find a more standard name for "CustomStrings" as it looks odd among the *Translation ones.

Also updating the issue summary with a much cleaner explanation (hopefully :).

Gábor Hojtsy’s picture

Issue summary: View changes

Updated issue summary.

Gábor Hojtsy’s picture

Issue summary: View changes

Simplify, extend as needed :)

Gábor Hojtsy’s picture

Issue summary: View changes

Fix followup link

Gábor Hojtsy’s picture

Issue summary: View changes

Updated issue summary.

Gábor Hojtsy’s picture

Title: Rework string translation / locale API fixing issues, adding consistency, TranslationInterface, DIC... » Introduce unified interfaces, use dependency injection for interface translation

Probably more human friendly title :)

Jose Reyero’s picture

@catch,
Ok about needing performance testing, though it all will come down to how fast is 'drupal_container()'.
I mean it's not just these patch that needs performance testing, all the other ton of patches committed would have needed it too. Still we don't want to make the locale system any slower so I'll add it to my to-do.

About translation service in drupal_container(),

When can we actually hit this condition? If language service is in the container, then the answer ought to be 'never'.

Yes, the answer 'ought to be never'. The same it ought to be for all the stuff created in drupal_container() I guess.
However at this point we have some critical edge cases specific of the locale system, and though this patch will fix some of them, there's for instance file_get_stream_wrappers(), which uses t().
(My question would be wtf is that doing in _drupal_bootstrap_code(), but I'm afraid this may be another long story so...)
More about these edge cases here #1807272: Mixed locale issues with upgrade scripts, unit tests, 'Table simpletest..locales_source doesn't exist', etc...
Also my fear with latest code I've seen getting in, is other hooks like hook_entity_info() that may use t() may be triggered before this full initialization too.
* About this possibly the proper fix would be to kick t() out of all info hooks and provide a 'hook_menu()-like' way of translation these strings, but this may be another long story too.
And anyway it all comes down to whether we may be able to use t() anywhere, not st nor get_t(), but t() and for that to be true, we need at the very least some minimal "translation interface" anywhere drupal_container() is used so as long as we have other objects defined in drupal_container(), we should at least define some translation-pass-through thing.

About 'custom strings' as we have now:
- (with this patch) A way for modules to inject 'translation interface' objects for t() + a fallback system
- Locale system translating English strings too.

... *They should be kicked out of Drupal core*.

(Though I didn't want to add more potentially disruptive 'features' into this patch so I thought it better is 'backwards compatible' with what we've got atm, let that discussion for later).

About 'StaticTranslation', yes, that is a base class but the reason it's not abstract is because it could be used on its own for tests or other stuff to load a predefined set of translations to be used....

Ok about 'fallback' / 'chain' terminology, that will be in next reroll.

Gábor Hojtsy’s picture

Issue tags: -sprint

Remove from sprint board to return after dec 1st then :)

fago’s picture

Related suggestion: Use a translatable class to postpone translation, see #1542144-11: Mark strings as localizable/translatable (new t()-alike string function that isn't t(), only for potx). Afaik, I think it would play well with this improvements also.

Crell’s picture

Issue tags: +Stalking Crell

Flagging for myself.

YesCT’s picture

YesCT’s picture

What shall we do here to move this forward? Are we waiting until after Feb 18th now?

YesCT’s picture

#25 was passing... lets see.

YesCT’s picture

Status: Needs review » Needs work
Issue tags: +D8MI, +needs profiling, +language-ui, +Stalking Crell

The last submitted patch, locale_translation_interface-25.patch, failed testing.

YesCT’s picture

Issue tags: +Needs reroll

does not apply, needs reroll

Gábor Hojtsy’s picture

Category: feature » task

I think given that this enables us to do major API cleanup, it would be still be possible post feb 18th. It will let us get rid of st(), get_t(), $t(), and let us use the same API for translation regardless of bootsrap/installation phase. That should be a huge win.

Gábor Hojtsy’s picture

Anyone interested in reviewing / doing performance testing? @Crell, you put this on your list earlier, but then did not come back :)

Jose Reyero’s picture

Assigned: japicoder » Jose Reyero

I want to give another try to this one, coming from #1905152: Integrate config schema with locale, so shipped configuration is translated

First of all, I'll be updating the patch, that should be easier now DIC and module bundles have a number of improvements.

Gábor Hojtsy’s picture

Sounds amazing :)

Jose Reyero’s picture

Status: Needs work » Needs review
FileSize
34.35 KB

This is the previous patch updated for latest core.

Set to 'needs review' for tests, but a few things still to-do:
- Address all issues in #27
- There seems to be some trouble with po importing tests
- Further t/st/get_t cleanup

Jose Reyero’s picture

Just a minor update of the previous patch, with some more cleanup, still a few things to do.

Status: Needs review » Needs work

The last submitted patch, 1813762-locale_translation_interface-45.patch, failed testing.

jthorson’s picture

#44 has been cycling on the testbots. Killing it manually. :(

Jose Reyero’s picture

Status: Needs work » Needs review
Jose Reyero’s picture

@jthorson,
It's happening again, could it be this patch that has sth wrong?

jthorson’s picture

Yes ... the reason these cycle is that something in the test results are preventing the communication between the testbot and qa.d.o. This may be due to there being so many failures/assertions that the communication times out or exceeds what the xmlrpc exchange can handle ... or the results end up with something in the log file which can't be processed by xmlrpc. In either case, it is specific to a given patch.

Status: Needs review » Needs work

The last submitted patch, 1813762-locale_translation_interface-45.patch, failed testing.

Jose Reyero’s picture

Status: Needs work » Needs review
FileSize
123.07 KB

No idea about why tests are failing. I cannot run some tests locally, though it may be my environment more than this patch.

Anyway, progressing with the code, this version adds a few things:
1. Renamed TranslationFallback to TranslationChain as suggested by @catch #27
2. Added some methods that will be useful, following the 'Chains' model: appendTranslation(), prepenTranslation() that..
3. ...allow us to dinamically add in FileTranslation for the installer.
4. BIG: Full get_t() replacement, not we have only t().
5. Repurposing st(), now only fur strings that run on the installer before container is initialized.
6. Removed t() from language_default_locked_languages(). Not sure about the side effects but this was breaking the LanguageUpgrade tests, and it seems to me it doesn't make sense to use t() within language initialization.

Still a few things to do:
7. Update all the docs for t/st/get_t
8. Add in installer language into regular language initialization somehow (Otherwise some installer page runs may use default language instead of installer language when using t)

Giving one more try to the patch. I may need some help from someone that can run the tests locally, to have an idea of what's going on...

Status: Needs review » Needs work

The last submitted patch, 1813762-locale_translation_interface-52.patch, failed testing.

Jose Reyero’s picture

Still no luck with tests. Some findings though:

- Tests fail with a not very helpful message of the type "The test did not complete due to a fatal error." On the apache log I get "[notice] child pid 30908 exit signal Segmentation fault (11)"
(Maybe that is why the testbot is crashing too).
- This seems to be related to unit tests using t() everywhere so if it is this function that fails, test error handling crashes too. Btw base test classes are plagued with t() too, see TestBase and WebTestBase classes.
- After the test fails, I get aditional warnings on the test results page, which seem to indicate form caching has crashed too:

Notice: Undefined index: storage in simpletest_test_form_submit() (line 187 of core/modules/simpletest/simpletest.pages.inc).
Warning: array_keys() expects parameter 1 to be array, null given in simpletest_test_form_submit() (line 187 of core/modules/simpletest/simpletest.pages.inc).
Warning: in_array() expects parameter 2 to be array, null given in simpletest_test_form_submit() (line 193 of core/modules/simpletest/simpletest.pages.inc).

All this seems to be related with t() function depending on DIC, which is the one thing introduced by the patch, and DIC not being properly handled on test crashes.
I can see some code in language() function too (not introduced by this patch) that looks like unit tests don't handle the DIC properly:

  if (!$container) {
    return language_default();
  }
  if (!$container->has('language_manager')) {
    // This happens in rare situations when the container has not been built by
    // a kernel and has no services, e.g., when t() is called during unit tests
    // for assertions.
    $container->register('language_manager', 'Drupal\Core\Language\LanguageManager');
  }

However, introducing similar code in the t() function doesn't work either, which seems to indicate the 'string translation' service is actually there but it is using the wrong drupal_container() at some points (the one from the test, that relies on locale installed, instead of the one from the unit test execution environment that doesn't have locale module enabled.

At this point I'd appreciate some help from someone who knows more about how unit tests handle the DIC.

Gábor Hojtsy’s picture

Have you tried bailing from t() altogether (by falling back on format_string()) if the container or string translation service is not there? Sounds like whatever unit tests test, they should be fine because they don't use a dual system like webtests, so if t() bails then it will be the same as if they did not call t() in the first place (or used format_string() for that matter).

ParisLiakos’s picture

Issue tags: +PHPUnit Blocker

tagging
@Jose Reyero : are you working on a sandbox? i am gonna look into this later

ParisLiakos’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
123.09 KB

Here is a reroll since the Installer test moved..
i tested locally, run-scripts.sh is behaving normal...there are a bunch of fails, but nothing dies..maybe php version has something to do with it?i have 5.4.4 locally
Lets see what testbot says now

ParisLiakos’s picture

Seems this got stuck again..

Status: Needs review » Needs work

The last submitted patch, 1813762-locale_translation_interface-58.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
1.09 KB
124.18 KB

lets try this

Status: Needs review » Needs work

The last submitted patch, 1813762-locale_translation_interface-60.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
4.55 KB
124.15 KB

now lets see fix some tests, if not all

Status: Needs review » Needs work

The last submitted patch, 1813762-locale_translation_interface-62.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
1.64 KB
125.02 KB

I tracked the 4 segfaults back to CacheArray::__destruct, which instantly reminded me the same problems Berdir had in the path whitelist patch..also i think in theme registry patch..so i applied the same solution waiting for #1786490: Add caching to the state system

lets see what testbot says now..bot??

ParisLiakos’s picture

Made some changes to bring it to latest docs standards and usage of \Drupal instead of drupal_container.
What about getting rid of locale_storage() and st() as well?

Also i think

  • all the logic of t() should be moved into ->translate()
  • add a Drupal::translation() method
  • have t() as a wrapper to Drupal::translation()->translate() so we can use that in classes that we want to unit test instead

I will roll a patch tomorrow..and will try to inject more stuff to language classes so they get to be unit-testable as well

Jose Reyero’s picture

Wow! This looks green, finally! :-)

@rootatwc,
Thank you so much.

Now I'd like to give the patch another pass with some improvements I had been working on...

ParisLiakos’s picture

Jose Reyero sure! I made a some small changes as well (been working here http://drupal.org/sandbox/rootatwc/1969260) , but i will wait for your changes! if you want you can use the sandbox
Also can we leave get_t() removal for #1838310: Remove st(), get_t() and $t for good ? This start getting really big and we already have a dedicated issue about that :)
Also: #1975490: Convert locale_custom_strings_* to settings

Jose Reyero’s picture

Added a few changes / improvements:
- Rolled back the t/get_t/st replacement, agree it will be better a follow up, posted it here, #1838310: Remove st(), get_t() and $t for good
- Renamed 'locale.translation' to 'string_translation' for the service name in the DIC. For consistency with other service names and also the initial one is not related to locale module.
- Implemented Drupal::translation() to get the 'string translation' service.
- As suggested in #66 moved t() logic into translate(), which seems to work nicely and allows for more powerful translation services. The downside is the Translation objects now need to handle default language themselves, which may cause issues during install / initialization.
Since passing the 'language_manager' service to the constructor may cause problems like the language manager being initialized too early, add a default language to get started and a setDefaultLanguage() method, which allows on one side the whole thing running without a LanguageManager and on the other side more flexibility for initialization (use installer language, build a temporary Translation service for the installer, etc...)
- When language is initialized on drupal_init_language(), now we call this method, which also allows the Translation objects to react upon that if needed.
- Added some more consistency / reliance to the st() function which can now use custom strings and file translation when running still without a container, that happens on install for the first steps.

@ParisLiakos, I'll be busy for the rest of the week, so I'll try to reply questions, but please don't wait for me to keep this moving.

Status: Needs review » Needs work

The last submitted patch, 1813762-locale_translation_interface-69.patch, failed testing.

ParisLiakos’s picture

+++ b/core/lib/Drupal/Core/Translation/TranslationChain.phpundefined
@@ -0,0 +1,101 @@
+class TranslationChain implements TranslationInterface {

This should be renamed to TranslationManager to go inline the rest of the core

+++ b/core/lib/Drupal/Core/Translation/TranslationChain.phpundefined
@@ -0,0 +1,101 @@
+  public function appendTranslation(TranslationInterface $translation) {
+    $this->translators[] = $translation;
...
+  public function prependTranslation(TranslationInterface $translation) {
+    array_unshift($this->translators, $translation);

wouldnt make sense renaming them to appendTranslator and prependTranslator? since the property is name translators..

function t($string, array $args = array(), array $options = array()) {

  $translation = Drupal::translation()->translate($string, $options);

  $string = $translation === FALSE ? $string : $translation;

  if (empty($args)) {
    return $string;
  }
  else {
    return format_string($string, $args);
  }
}

The remaining format_string logic should be moved as well in the ->translate() method using String::format(), which leaves us with t() being a one line wrapper to the service

when running still without a container, that happens on install for the first steps.

I thought we had a container on all steps?
install_begin_request() always create one

ParisLiakos’s picture

+++ b/core/core.services.ymlundefined
@@ -160,6 +160,13 @@ services:
+  string_translation.custom:
+    class: Drupal\Core\Translation\CustomStrings

i propose we rename those to translators and move them in a Translator directory. Also rename the interface, since it is actually a translator class not a translation one

+++ b/core/core.services.ymlundefined
@@ -160,6 +160,13 @@ services:
+  string_translation:
+    class: Drupal\Core\Translation\TranslationChain
+    arguments: ['@string_translation.custom']

+++ b/core/modules/locale/locale.services.ymlundefined
@@ -4,3 +4,14 @@ services:
+  locale.translation:
+    class: Drupal\Core\Translation\TranslationChain
+    arguments: ['@string_translation.custom', '@locale.translation.lookup']

So how that works? we have same class registered in two services? i am really confused now.

+++ b/core/includes/install.incundefined
@@ -938,53 +939,38 @@ function drupal_requirements_url($severity) {
+    // The translation service is not there yet, use a temporary one.
+    if (!isset($install_translation)) {
+      $install_translation = new TranslationChain(
+          new CustomStrings(),
+          install_file_translation_service()
+      );

i have still to figure out why there is no service..if there is no container we should fix that instead. Only place we dont have one is in DRUPAL_BOOTSTRAP_CONFIGURATION but t() is never called there, as far as i can see
Edit: in install_verify_database_settings()

ParisLiakos’s picture

Assigned: Jose Reyero » ParisLiakos

restoring tags
d.o--

ParisLiakos’s picture

Status: Needs work » Needs review
Issue tags: +needs profiling, +language-ui, +Stalking Crell, +PHPUnit Blocker
FileSize
60.66 KB

Made the rename/move to translators.
Added setDefaultLangcode call to the Language request listener.
Moved rest of t() logic in translate()
Moved translate() out of interface
A lot more..

Sorry interdiff to #69 not possible

Status: Needs review » Needs work

The last submitted patch, 1813762-locale_translation_interface-74.patch, failed testing.

ParisLiakos’s picture

i have no clue why those upgrade tests fail

Jose Reyero’s picture

Some thoughts about #71 and #72:

About naming, ok with any, though I think it should be StringWhatever, to differenciate de concept from EntityTranslation, etc.. (Also the other things are named 'Translation'...)

Encapsulating also the format_string(), I think we better don't, just to keep Translation interface simpler and up to the point, which is translating, not formatting. Anyway, no big deal, we can keep it.

'TranslationChain' was a suggestion in #27, and I think it makes more sense that calling it 'Manager' because it is a plain 'Translator' that implements the TranslationInterface, just like the other 'Translators' so at this point all of them are kind of replaceable. Though ok if you want to rework it a bit to be a real 'TranslationManager' (maybe we could add the format_string and/or the default language handling only there...?)
(But we better keep TranslationChain for usages like 'st' without container and also to use it in configuration translation)

Yes, there are some t() calls (st) before the database is initialized, in the case it fails I think. You can see it if you run the installer manually with no database or cause some other eary failure condition (no settings file, folder not writable, etc..)
And also if we have this fallback we can properly use translation (or simple string overrides) for these error pages.

For locale.services.yml, I think the module one overrides the core one (not sure) but yes, we should just add the LocaleTranslation without replacing (not sure how to add some callbacks to the 'string_translation' in services.yml file)

The reason why upgrade tests fail I think is the locale services being enabled without the module schema not being up to date yet (what happens when you upgrade a localized Drupal).

Maybe the solution for the last two would be using LocaleBundle instead of locale.services.yml so we can add some init checks before adding the LocaleTranslation in.

ParisLiakos’s picture

should be StringWhatever, to differenciate de concept from EntityTranslation

you are right i did not think of that..i ll change it to string_translation..

Though ok if you want to rework it a bit to be a real 'TranslationManager' (maybe we could add the format_string and/or the default language handling only there...?)

Thats what i am doing..It also works for st() in installer when there is no container

Yes, there are some t() calls (st) before the database is initialized, in the case it fails I think

Yeap, found out my self in install_verify_database_settings()

The reason why upgrade tests fail I think is the locale services being enabled without the module schema not being up to date yet (what happens when you upgrade a localized Drupal).

Maybe the solution for the last two would be using LocaleBundle instead of locale.services.yml so we can add some init checks before adding the LocaleTranslation in.

Thanks for the hints, i ll look into it!

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
1.15 KB
61.81 KB

lets see if this fixes upgrade path

Jose Reyero’s picture

Latest version looks great to me, you've fixed all the issues nicely.

Only minor issues pending, just some thoughts:
- Naming: Translator/StringTranslator/StringTranslation? I would settle for any maybe let's wait to see what other code reviewers think.
- Just to note: It may be confusing the get_t() replacement (now returns always 't') without the full replacement patch, though it is acceptable as a temporary solution because it proves this whole thing works, while waiting for #1838310: Remove st(), get_t() and $t for good
- We could change the interface's method getTranslation() to getStringTranslation(), in advance of translators being capable of translating other things too, think getPluralTranslation() or getDateTranslation(). Just a wish for now.
- Instead of TranslationManager::setDefaultLanguageCode() we could use the full language object, and setDefaultLanguage(), I think the code would look more consistent with all language handling.

Anyway, this is really ready to be committed as is, because all these DIC issues are really hard to debug by themselves, no need to clutter the patch with more code. So I'd ask for some final code review and then move to RTBC.

ParisLiakos’s picture

Naming: Translator/StringTranslator/StringTranslation?

+1 for StringTranslator

We could change the interface's method getTranslation() to getStringTranslation(), in advance of translators being capable of translating other things too, think getPluralTranslation() or getDateTranslation(). Just a wish for now.
- Instead of TranslationManager::setDefaultLanguageCode() we could use the full language object, and setDefaultLanguage(), I think the code would look more consistent with all language handling.

agreed on both

-------------------------

one more thing i have in mind and would be nice for discussing..might as well be a followup:
I think translators should be plugins:) then no need to register them as a service

Jose Reyero’s picture

Cool!
Just one more thing: There are some weird container conditions handling in language() that may not be needed anymore with this patch, maybe it's time to drop them too (if tests pass after that because I can imagine language() can be invoked from other weird places too --oh, yes, format_plural, ...)
I mean, if it breaks the tests, we can just leave it for a follow up, because it would be another of that bootstrap/DIC issues complex enough by itself....

ParisLiakos’s picture

Instead of renaming all classes prefixed with String, i just changed the namespace to Drupal\Core\StringTranslation from Drupal\Core\Translation
renamed the service to string_translation
renamed getTranslation to getStringTranslation
removed container conditions in language()

Passing the language object is not possible, since in the early steps of installer we cant use language_load() to get the object from user selected langcode

Test pass locally

Created #1978484: Make translators plugins as a followup

ParisLiakos’s picture

Ok just found https://github.com/symfony/Translation
They have a similar structure. A translator (in patch above translation manager) , translator interface and loaders (in patch above translators).. https://github.com/symfony/Translation/tree/master/Loader

Are we doing this wrong? maybe we should pull this in instead of reinventing the wheel here?

Gábor Hojtsy’s picture

@ParisLiakos: see #1570346: Let Symfony Translation component handle language messages and http://build2be.com/content/pinch-symfony-d8mi for background on ClemensTolboom's bitter fight to blend us in, and the ultimate failure not due to Clemens not trying hard enough but due both to mismatch in features provided/needed, lack of upstream support, licensing incompatibility, different roadmaps, etc.

Crell’s picture

I don't full grok everything going on here, so I mostly just looked at the central architecture and I think it looks solid. Nice work!

A few notes below:

+++ b/core/includes/bootstrap.inc
@@ -2714,7 +2680,8 @@ function get_t() {
 /**
@@ -2726,17 +2693,7 @@ function drupal_language_initialize() {

@@ -2726,17 +2693,7 @@ function drupal_language_initialize() {
  *   The type of language object needed, e.g. LANGUAGE_TYPE_INTERFACE.
  */
 function language($type) {
-  $container = drupal_container();

We probably want to mark this function @deprecated now. (I don't think we can mark t() deprecated, though.)

+++ b/core/lib/Drupal/Core/StringTranslation/Translator/StaticTranslation.php
@@ -0,0 +1,64 @@
+  /**
+   * Add translations for new language.
+   */
+  protected function loadLanguage($langcode) {
+    return array();
+  }

This seems unimplemented...

YesCT’s picture

Assigned: ParisLiakos » YesCT

I asked @ParisLiakos in irc if I could take a stab at this. Patch coming.

YesCT’s picture

I added the @depreciated for function language() [edit: corrected function name]
But it is called. so next coming patch just does the Drupal::service('language_manager')->getLanguage($type) directly in those places, at least in the changed lines from this patch.

Regarding loadLanguage being unimplemented...
grep -R "loadLanguage(" *
core/lib/Drupal/Core/StringTranslation/Translator/CustomStrings.php: protected function loadLanguage($langcode) {
core/lib/Drupal/Core/StringTranslation/Translator/FileTranslation.php: protected function loadLanguage($langcode) {
core/lib/Drupal/Core/StringTranslation/Translator/StaticTranslation.php: $this->translations[$langcode] = $this->loadLanguage($langcode);
core/lib/Drupal/Core/StringTranslation/Translator/StaticTranslation.php: protected function loadLanguage($langcode) {

I think, for example, CustomStrings implements it.. so StaticTranslation is allowing others to implement it.
Does static need more? I'm wondering since there were no failed tests. Maybe some manual testing can show a failure so we can know what kind of test to add?

YesCT’s picture

+++ b/core/includes/bootstrap.incundefined
@@ -2714,7 +2680,8 @@ function get_t() {
 function drupal_language_initialize() {
-  drupal_container()->get('language_manager')->init();
+  Drupal::service('language_manager')->init();
+  Drupal::translation()->setDefaultLangcode(language(LANGUAGE_TYPE_INTERFACE)->langcode);
 }

here was the only place I saw language() still being called.

YesCT’s picture

I'm just guessing here...

it was either this or just something like
Drupal::translation()->setDefaultLangcode(Drupal::service('language_manager')->getLanguage(LANGUAGE_TYPE_INTERFACE)->langcode);

Status: Needs review » Needs work
Issue tags: -D8MI, -needs profiling, -language-ui, -Stalking Crell, -PHPUnit Blocker

The last submitted patch, 1813762-locale_translation_interface-90.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +D8MI, +needs profiling, +language-ui, +Stalking Crell, +PHPUnit Blocker

The last submitted patch, 1813762-locale_translation_interface-90.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review
FileSize
62.77 KB

no interdiff, this is just a new git diff to remove the hunk offset messages to see if that helps.

  501  git pull --rebase
  502  drush am 1813762
  503  git status
  504  git add core*
  505  git status
  506  git commit -m "90"
  507  git diff 8.x > 1813762-locale_translation_interface-94.patch
ParisLiakos’s picture

+++ b/core/includes/bootstrap.incundefined
@@ -2603,27 +2573,23 @@ function drupal_installation_attempted() {
+  Drupal::service('language_manager')->init();
+  $default_language = Drupal::service('language_manager')->getLanguage(LANGUAGE_TYPE_INTERFACE);
+  Drupal::translation()->setDefaultLangcode($default_language->langcode);

better store language manager:

$language_manager = Drupal::service('language_manager');
$language_manager->init();
Drupal::translation()->setDefaultLangcode($language_manager->getLanguage(LANGUAGE_TYPE_INTERFACE)->langcode);

And lets move on with this issue, if no other objections

YesCT’s picture

the change to save the language_manager

YesCT’s picture

Assigned: YesCT » Unassigned
catch’s picture

Didn't review all the changes in depth but this looks very encouraging to me.

plach’s picture

+++ b/core/includes/bootstrap.inc
@@ -1525,37 +1525,7 @@ function bootstrap_hooks() {
+  return Drupal::translation()->translate($string, $args, $options);

Sorry if this has already been asked, but would it make sense to statically cache the service object returned by Drupal::translation() to save a method invocation for each t() call?

ParisLiakos’s picture

@plach: i will run some profiling later and do it if it helps

ParisLiakos’s picture

Issue tags: -needs profiling

would it make sense to statically cache the service object returned by Drupal::translation() to save a method invocation for each t() call?

We would need a way to update it when kernel is being rebuilt, or if translation service changes in runtime.
But i think this is not needed. we the changes from this patch t() now uses less memory, because it saves some calls to get the default langcode..since the translation manager already holds that in a property:) yay!
here what i got:

4 calls to t() on translateble strings, with two enabled languages:

Before:
Total Incl. MemUse (bytes): 12,632 bytes
Total Incl. PeakMemUse (bytes): 6,056 bytes
Number of Function Calls: 42

After:
Total Incl. MemUse (bytes): 11,888 bytes
Total Incl. PeakMemUse (bytes): 4,968 bytes
Number of Function Calls: 38

We save on call per t() run..which makes some difference when we call it a few hundred times:)

aspilicious’s picture

+++ b/core/includes/bootstrap.incundefined
@@ -2603,27 +2573,23 @@ function drupal_installation_attempted() {
 function get_t() {
-  static $t;
-  // This is not converted to drupal_static because there is no point in
-  // resetting this as it can not change in the course of a request.
-  if (!isset($t)) {
-    $t = drupal_installation_attempted() ? 'st' : 't';
-  }
-  return $t;
+  return 't';

awesome

+++ b/core/includes/bootstrap.incundefined
@@ -2633,19 +2599,12 @@ function drupal_language_initialize() {
+ * @deprecated as of Drupal 8.0. Use
+ *   Drupal::service('language_manager')->getLanguage($type).
  */

There is a standard for @deprecated tags somewhere. We don't need to define the alternative if I'm correct. It's already in the code beneath it.

Awesome patch :)

YesCT’s picture

#1876842: [policy, no patch] Use @deprecated to indicate a deprecated method or function
still working out the details it looks like.

Not in
http://drupal.org/node/1354 API documentation and comment standards
yet.

dawehner’s picture

Really nice work!

+++ b/core/lib/Drupal/Core/StringTranslation/TranslationManager.phpundefined
@@ -0,0 +1,151 @@
+class TranslationManager implements TranslatorInterface {

I'm wondering whether it's worth to think about caching the translations here?

+++ b/core/lib/Drupal/Core/StringTranslation/TranslationManager.phpundefined
@@ -0,0 +1,151 @@
+  public function reset() {

What do we do with the default langcode?

+++ b/core/lib/Drupal/Core/StringTranslation/Translator/CustomStrings.phpundefined
@@ -0,0 +1,25 @@
+    return variable_get('locale_custom_strings_' . $langcode, array());

So is there an issue to convert it to settings() or something different?

+++ b/core/lib/Drupal/Core/StringTranslation/Translator/FileTranslation.phpundefined
@@ -0,0 +1,103 @@
+   * {@inheritdoc}
+   */
+  public function __construct($directory) {
+    parent::__construct();

Isn't the signature totally different?

+++ b/core/lib/Drupal/Core/StringTranslation/Translator/FileTranslation.phpundefined
@@ -0,0 +1,103 @@
+   * @param $langcode
...
+   * @return

need types.

+++ b/core/lib/Drupal/Core/StringTranslation/Translator/FileTranslation.phpundefined
@@ -0,0 +1,103 @@
+    $files = file_scan_directory($this->directory, '!drupal-\d+\.\d+\.' . (!empty($langcode) ? preg_quote($langcode, '!') : '[^\.]+') . '\.po$!', array('recurse' => FALSE));

There should be certainly a comment what this regex does.

+++ b/core/lib/Drupal/Core/StringTranslation/Translator/StaticTranslation.phpundefined
@@ -0,0 +1,64 @@
+  protected function loadLanguage($langcode) {

Needs doc.

+++ b/core/lib/Drupal/Core/StringTranslation/Translator/TranslatorInterface.phpundefined
@@ -0,0 +1,40 @@
+   *   The source string
...
+   *   The string context

Missing dot at the end.

+++ b/core/lib/Drupal/Core/StringTranslation/Translator/TranslatorInterface.phpundefined
@@ -0,0 +1,40 @@
+  public function getStringTranslation($langcode, $string, $context);

I'm wondering whether a shorter method name like ->translate() would help once we inject the translation manager into objects.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleTranslation.phpundefined
@@ -0,0 +1,72 @@
+    return $translation === TRUE ? FALSE : $translation;

That's odd, do we maybe have to document that TRUE means that LocaleLookup didn't found it?

+++ b/core/modules/locale/lib/Drupal/locale/StringDatabaseStorage.phpundefined
@@ -287,40 +287,19 @@ protected function dbStringTable($string) {
+    if (!empty($keys) && ($values = $string->getValues($keys)) && count($keys) == count($values)) {

It seems to be that this line could get some explanation.

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleExportTest.phpundefined
@@ -79,9 +79,12 @@ function testExportTranslation() {
+    $this->container
+      ->get('locale.storage')
+      ->createString()
+      ->setString('February')
+      ->save();

Nice!

+++ b/core/modules/locale/locale.services.ymlundefined
@@ -6,4 +6,14 @@ services:
+# Add the lookup translator to the translation service.
+  string_translation:
+    class: Drupal\Core\StringTranslation\TranslationManager
+    arguments: [['@string_translator.custom_strings', '@string_translator.locale.lookup']]

Shouldn't we introduce a tag and let a compiler class take care about adding the translators?

+++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.phpundefined
@@ -904,6 +904,9 @@ protected function prepareEnvironment() {
+     // @todo Remove this once this class has no calls to t() and format_plural()

good luck :)

+++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.phpundefined
@@ -904,6 +904,9 @@ protected function prepareEnvironment() {
+    $this->container->register('string_translation', 'Drupal\Core\StringTranslation\TranslationManager');

Why do we need to set this manually, couldn't that be of the standard container?

ParisLiakos’s picture

Thanks for the review!

I'm wondering whether it's worth to think about caching the translations here?

we cant cache them in a manager cause translators can be appended in runtime. But translators cache their own translations.

So is there an issue to convert it to settings() or something different?

#1975490: Convert locale_custom_strings_* to settings

I'm wondering whether a shorter method name like ->translate() would help once we inject the translation manager into objects.

Translation manager already has a translate() method. I dont think translators need one, since manager will be injected

That's odd, do we maybe have to document that TRUE means that LocaleLookup didn't found it?

It is already documented on TranslatorInterface::getStringTranslation()

Shouldn't we introduce a tag and let a compiler class take care about adding the translators?

Thats a lot better idea than turning them to plugins, thanks!

Why do we need to set this manually, couldn't that be of the standard container?

Simpletest should use a minimal container, preferably with no services in it..and each individual test enable the services it needs..with t() though we have to register it initially cause t() and format_plural() calls are all over the place

ParisLiakos’s picture

tags:) lets see

Jose Reyero’s picture

I think this looks awesome and I'd just RTBC it if I understood the implications -if any- of adding that new 'string_translator' container tag and that 'RegisterStringTranslatorsPass' that seem to be introduced in the latest version, though missing from the diff (?). So just being cautious with what I don't fully understand.
-- The question is just whether introducing a new compiler pass, which seems to work nicely otherwise, has any other implications... ?

Just to note, for other reviewers:
- The resulting 'unclarified' role of 'get_t() / st() / t()' will be worked out in this follow up so I think the quick docs updating in this patch is ok, as they need to be redefined and re-documented anyway #1838310: Remove st(), get_t() and $t for good
- About custom strings, improving them is not the purpose of this patch and it should be ok just updating the current feature to keep it working, which is what is done atm. (Actually I'd rather see them out of core as it could done by any contrib module after this patch has landed but that's another issue...)

Gábor Hojtsy’s picture

How was this not on the D8MI sprint yet? :) @jose I think you meant #1838310: Remove st(), get_t() and $t for good for the get_t/st, etc. issue.

Gábor Hojtsy’s picture

dawehner’s picture

+++ b/core/lib/Drupal/Core/StringTranslation/Translator/FileTranslation.phpundefined
@@ -31,7 +31,10 @@ class FileTranslation extends StaticTranslation {
+   * Overrides \Drupal\Core\StringTranslation\Translator\StaticTranslation::__construct().

I guess constructors are special, maybe we should just use the typical "Constructs a ..."

Jose Reyero’s picture

@Gábor, right, updated the link in the comment to avoid confusion.

ParisLiakos’s picture

Rerolled and fixed #111

ParisLiakos’s picture

removed appendTranslator and prependTranslator in favor of priorities, to be consistent with rest of core

Schnitzel’s picture

Status: Needs review » Needs work

Made a Code Review, found only this bit

+++ b/core/modules/locale/lib/Drupal/locale/LocaleTranslation.phpundefined
@@ -0,0 +1,72 @@
+    if ($langcode == LANGUAGE_SYSTEM || ($langcode == 'en' && !variable_get('locale_translate_english', FALSE))) {

There is a function locale_translate_english() for this in locale.module :)

And overall this looks really nice!

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
1.69 KB
64.91 KB

ah, cool, i wasnt aware we had a function like this:)

here is a patch with it and some other improvements

Status: Needs review » Needs work

The last submitted patch, 1813762-locale_translation_interface-115.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
827 bytes
64.93 KB

yeah, well locale_translate_english brings problem during upgrade path tests, because simpletest, as usual is infested with t() calls and locale.module is not loaded yet, so there is a nice undefined function fatal error during tests

I dont think i should spend any time fixing this, since this variable will be converted to config anyway (there is a critical opened for this), which will be injected in the locale translation

ParisLiakos’s picture

attiks’s picture

+++ b/core/lib/Drupal/Core/StringTranslation/Translator/FileTranslation.phpundefined
@@ -0,0 +1,106 @@
+   * Finds installer translations either for a specific langcode or all languages.

comment too long :/

ParisLiakos’s picture

Jose Reyero’s picture

My only doubt is about sortTranslators() method. Being this a performance critical funcion, shouldn't we keep the order cached somehow without reordering every time?

if (!isset($this->sorted)) {
...
}
ParisLiakos’s picture

you are right:)
here it is with an optimization according to #121

Jose Reyero’s picture

Status: Needs review » Reviewed & tested by the community

Nice! I think this is ready to go.

ParisLiakos’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1813762-locale_translation_interface-124.patch, failed testing.

ParisLiakos’s picture

ah, a use statement..should be ok now

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
522 bytes

Status: Needs review » Needs work

The last submitted patch, 1813762-locale_translation_interface-126.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
757 bytes
65.75 KB

copy paste failure when resolving conflicts (sigh)

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

back to rtbc

Gábor Hojtsy’s picture

Schnitzel’s picture

+1, really like this, it will definitely break #2004684: Improve Accessibility for String Translation UI but I can reroll it then.

Gábor Hojtsy’s picture

Priority: Normal » Major

I think both the size and impact of this issue qualifies for a major. Surprised it was not yet marked on that level.

plach’s picture

Issue tags: +Avoid commit conflicts

And we want this in ASAP :)

catch’s picture

Status: Reviewed & tested by the community » Needs work

Overall this looks like a good step forward, nice to see t() move to a one-liner.

I didn't do an in-depth review of the whole patch, but these two stuck out as odd for a start:

+++ b/core/lib/Drupal/Core/StringTranslation/Translator/StaticTranslation.phpundefined
@@ -0,0 +1,67 @@
+  /**
+   * Add translations for new language.
+   *
+   * @param string $langcode
+   *   The langcode of the language.
+   */
+  protected function loadLanguage($langcode) {
+    return array();

Why does this only return an array?

+++ b/core/modules/locale/lib/Drupal/locale/LocaleLookup.phpundefined
@@ -83,4 +85,13 @@ protected function resolveCacheMiss($offset) {
+  /**
+   * {@inheritdoc}
+   */
+  public function __destruct() {
+    // Do nothing to avoid segmentation faults. This can be restored after the
+    // cache collector from http://drupal.org/node/1786490 is used.

Doesn't this just disable caching altogether?

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
65.69 KB

straight reroll for now so that i can interdiff

ParisLiakos’s picture

ok, this was tricky, but i believe catch's concerns are addressed
unless bot disagrees ofc

Berdir’s picture

Only checked the destruction parts and that looks good. It's ugly but it's the same pattern that we used for the path whitelist cache and we really really need the cache collector...

Jose Reyero’s picture

Just to note, reply to @catch #135 is in the patch itself

+  /**
+   * Add translations for new language.
+   *
+   * @param string $langcode
+   *   The langcode of the language.
+   */
+  protected function loadLanguage($langcode) {
+    // This class is usually a base class but we do not declare as abstract
+    // because it can be used on its own, by passing a simple array on the
+    // constructor. This can be useful while testing, but it does not support
+    // loading specific languages. All available languages should be passed
+    // in the constructor array.
+    return array();
Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Back to catch then! :)

catch’s picture

Status: Reviewed & tested by the community » Fixed

Looks better. The destructable interface stuff is indeed ugly. Berdir could we maybe re-open #1858616: Extract generic CacheCollector implementation and interface from CacheArray to try to get that in asap again?

Committed/pushed to 8.x, thanks!

Gábor Hojtsy’s picture

Issue tags: -sprint

Woot, thanks! @reyero, @ParisLiakos, great job! Now onto #1838310: Remove st(), get_t() and $t for good? (and #1966538: Translation is not updated for configuration, when the config changes) - both of which were dependent on this landing!

jibran’s picture

Title: Introduce unified interfaces, use dependency injection for interface translation » Change notice: Introduce unified interfaces, use dependency injection for interface translation
Priority: Major » Critical
Status: Fixed » Active
Issue tags: -Avoid commit conflicts +Needs change record

This is screaming for change notification.

YesCT’s picture

https://drupal.org/contributor-tasks/draft-change-notification
is instructions for drafting a change notice.
Anyone can try the first draft. :)
Just make a comment saying you are going to try, and ask questions if you have any.

ParisLiakos’s picture

Status: Active » Needs review
Issue tags: -Needs change record
Crell’s picture

I made a few small grammar tweaks, nothing major.

"(lower priority means your translator will fire first)" - This is false. Priority is opposite from weight, so low priority fires last. If the actual code that went in is using it as weight and that parenthetical is correct, that's a bug that needs to be fixed.

Also, there's code samples in the change notice for the locale storage but not the actual translation itself. That's far more useful. "What am I supposed to do now instead of calling t()" is the main question that should be answered by a code sample. (99.3% of people won't ever change the locale storage, but everyone translates strings.)

Gábor Hojtsy’s picture

Yeah, well, if we want to have people work on an injected t()-like service without calling t() that sounds like in dire need of a pattern defined before people jump on the opportunity and make out all kinds of fun things, or we'll not be able to extract the translatable strings.

ParisLiakos’s picture

bug followup:
#2018409: LocaleTranslation should have its translation property initialized as array

about #146,#147. while it is possible, lets also figure a nicer way with better DX than doing $this->translation->translate() instead of t() before putting it in the change notice, or people will start tweeeting all kind of things for D8 again

i opened #2018411: Figure out a nice DX when working with injected translation for that

Gábor Hojtsy’s picture

Title: Change notice: Introduce unified interfaces, use dependency injection for interface translation » Introduce unified interfaces, use dependency injection for interface translation
Priority: Critical » Major
Status: Needs review » Fixed

I see the change log fixed the priority and #2018411: Figure out a nice DX when working with injected translation is open for refining the DX. So looks like the change notice is good for this one.

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

Anonymous’s picture

Issue summary: View changes

Fix links.