Basic authentication broken for non-english sites

I'm trying to reproduce this bug so I can test the patch but I'm not getting it. I've installed from the latest Drupal 8 dev with the language set to German and then I followed the instructions on https://www.drupal.org/documentation/modules/basic_auth which culminate with "try various routes like router_test/test11". When I try that route, I don't get any indication of a circular recursion. I just get an "Access Denied" message, which I believe is what one would expect?

@berdir
It seems not so nice if you have to create TranslationWrappers for your own all over the place. One simple way could be to introduce \Drupal::t() which creates these objects?

+++ b/core/lib/Drupal/Core/Entity/Annotation/ConfigEntityType.php
@@ -36,7 +37,7 @@ class ConfigEntityType extends EntityType {
+    $this->definition['group_label'] = New TranslationWrapper('Configuration', array(), array('context' => 'Entity type group'));

Do we write code in german now ;) Let's lowercase "New".

Another idea I had was to pass along some kind of object to baseFieldDefinition

Was this happening due to the nonsensical logic in #2240459: Applying default language fallback rules to interface translation has impressively bad results that was fixed since then? Or still happening? Will send the test only patch for retest.

Berdir says still a problem.

[5:35pm] berdir: GaborHojtsy: so when you call t() the first time, it needs to get the roles for the currently logged in user. and if that is an entity (and not a plain object like CurrentUser), that needs to load field/property definitions to get that. and those call t() => loop
[5:35pm] GaborHojtsy: berdir: hm, not clear why that is a loop, the t()s should return?
[5:35pm] GaborHojtsy: berdir: they are not looking at the same thing to translate?
[5:36pm] berdir: GaborHojtsy: LocaleLookup::__construct()
[5:37pm] berdir: GaborHojtsy: the first call to t() will call that, that calls t() somewhere down in getRoles(), then that calls the constructor again because it has not yet been initialized, then we call getRoles() again...

Needs to be updated in the summary.

[5:37pm] berdir: GaborHojtsy: the first call to t() will call that, that calls t() somewhere down in getRoles(), then that calls the constructor again because it has not yet been initialized, then we call getRoles() again...

Okay, so as always, it is a bad idea to execute any code beside $this->foo = $foo; in the constructor.
What about deferring the call a bit. I haven't actually tried whether the approach works, but could be interesting.

So that approach is at least green.

So the thing is that now the t() call goes like this:

-> t()
  -> LocaleTranslation::getStringTranslation
  -> new LocaleLookup ...
  -> $translation = $this->translations[$langcode][$context]->get($string);
    -> getCid()
    -> UserRole::baseFieldDefinitions
      -> t()
      -> LocaleTranslation::getStringTranslation
      -> $this->translations[$langcode][$context]->get($string)
      -> return the base field title

So as you see the deferring helps for the next call.

In HEAD it works like the following:

-> t()
  -> LocaleTranslation::getStringTranslation
  -> new LocaleLookup ...
    -> UserRole::baseFieldDefinitions
    -> t()
      -> new LocaleLookup
        ->
          ->

Great!

+++ b/core/lib/Drupal/Core/Entity/Annotation/ConfigEntityType.php
@@ -36,7 +37,7 @@ class ConfigEntityType extends EntityType {
-    $this->definition['group_label'] = $this->t('Configuration', array(), array('context' => 'Entity type group'));
+    $this->definition['group_label'] = new TranslationWrapper('Configuration', array(), array('context' => 'Entity type group'));

+++ b/core/lib/Drupal/Core/Entity/Annotation/ContentEntityType.php
@@ -36,7 +37,7 @@ class ContentEntityType extends EntityType {
-    $this->definition['group_label'] = $this->t('Content', array(), array('context' => 'Entity type group'));
+    $this->definition['group_label'] = new TranslationWrapper('Content', array(), array('context' => 'Entity type group'));

Should we explicit document these cases and why we do this? PS: Does potx has support for this already?

@dawehner: yes, potx supports TranslationWrapper.

@berdir: we have language.entity.und and language.entity.zxx already pre-shipped with the language module, we don't write them from the code modified here. Or you referred to something else that is written out into config?

+++ b/core/modules/system/src/Tests/Entity/EntityLanguageTestBase.php
@@ -106,11 +106,7 @@ protected function setUp() {
+    $this->installConfig(array('language'));

So this would also include language.(negotation, mappings and types).yml
do we really want those as well? I guess it is not problematic at all.

Me neither now :)

The fix looks as good as it can get I guess :) This is a tricky circular dependency.

+++ b/core/lib/Drupal/Core/Entity/Annotation/ConfigEntityType.php
@@ -36,7 +37,7 @@ class ConfigEntityType extends EntityType {
-    $this->definition['group_label'] = $this->t('Configuration', array(), array('context' => 'Entity type group'));
+    $this->definition['group_label'] = new TranslationWrapper('Configuration', array(), array('context' => 'Entity type group'));

+++ b/core/lib/Drupal/Core/Entity/Annotation/ContentEntityType.php
@@ -36,7 +37,7 @@ class ContentEntityType extends EntityType {
-    $this->definition['group_label'] = $this->t('Content', array(), array('context' => 'Entity type group'));
+    $this->definition['group_label'] = new TranslationWrapper('Content', array(), array('context' => 'Entity type group'));

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/StringItem.php
@@ -39,7 +40,7 @@ public static function defaultSettings() {
-      ->setLabel(t('Text value'));
+      ->setLabel(new TranslationWrapper('Text value'));

+++ b/core/lib/Drupal/Core/Language/LanguageManager.php
@@ -197,13 +198,13 @@ public function getDefaultLockedLanguages($weight = 0) {
-      'name' => $this->t('Not specified'),
+      'name' => new TranslationWrapper('Not specified'),
...
-      'name' => $this->t('Not applicable'),
+      'name' => new TranslationWrapper('Not applicable'),

Is there some hard rule to where should we use TranslationWrapper? I mean people may be puzzled that one field type uses it while others don't. IMHO we should explain it with comments or do some other kind of long term knowledge preservation.

////

Also do we want to pursue any of the remaining tasks in this issue? I think not, but then would be good to say it there. That section is to be used for things still outstanding for commit on an issue.

It would be great to mention in some update docs/CRs that TranslationWrapper is replacement for st() ;)

@andypost: No, not at all a replacement. st() did translation right away using an array in memory reading from a specific file in the filesystem, while TranslationWrapper() does not do translation until cast to a string sometime later and will use all the regular services that t() would use at the time, same backend. So the behaviours have nothing to do with each other.

@Berdir: looks like this is down to the comment cleanups discussed.

@all: special thanks for maintaining a stellar issue summary.

Looks fine by me. While there is some black magic to which ones to apply the TranslationWrapper treatment, there does not seem to be a possibility to do this cleaner.

Looks good to me as well. TranslationWrapper is the only way we have at the moment to resolve these circular dependencies, so as long as we explain why we're using it I'm happy.

Yay, thanks.