Translation of low-level info/annotations leads to circular dependencies

As just discussed in IRC, for entity types, we could do the translation when you call getLabel(), but we a) can't do that for plugin types that still use arrays (everything else) and we still need a way to flag strings as translatable.

A nice side effect of this would be that we can avoid to cache by language, which would avoid quite a bit of complexity for plugin managers.

We can make up another annotation special function, instead of @Translation it could be something else, that is a no-op but the extractor can pick up.

Also is this a beta blocker? It definitely involves API changes.

Sounds like this is at least two separate issues? One for entities, another for everything else?

And yeah, reviewing this, I think it potentially needs to be beta-blocking for its impacts on the plugin and entity APIs.

An alternate solution is any annotation value for a specific key, eg. "display_label" or something would be extracted for translation. The base classes can include a method to return the translated metadata label. Although this also starts being messy since that would be the plugin type label not the plugin label itself :D Heh.

If it's just about annotations, then we should be able to easily change the processing code to retain the Translation object as-is and move the actual invocation of t() into __toString().

To do so, the Translation annotation class should implement Serializable.

Not exactly sure what I'm patching, but here's a prototype.

Per @tim.plunkett stream wrappers are not and will not be implemented as plugins (and they are not annotated), so they at least should have a separate issue if there's a similar problem there.

Edit: Issue for the stream wrapper conversions: #2028109: Convert hook_stream_wrappers() to tagged services.

7: drupal8.anno-trans.7.patch queued for re-testing.

I'm scared about going down that path, but let's try it for real, the other patch still called t() in the constructor and the missing serialization blew up the testbot completely with the famous unserialize() offset errors.

This does not look that bad ;)

Yeah, expected something like that.

__toString() has limits, A lot of the exceptions for example caused by passing an array of labels to an select field, which treats objects as nested optgroups.

A change like that would lead to very unexpected behavior and while we could handle it in getLabel() of entity types, other plugin definitions aren't so lucky...

+++ b/core/lib/Drupal/Core/Form/OptGroup.php
@@ -44,7 +44,12 @@ public static function flattenOptions(array $array) {
-        static::doFlattenOptions($value->option, $options);
+        if (property_exists($value, 'option')) {
+          static::doFlattenOptions($value->option, $options);
+        }
+        else {
+          $options[$key] = 1;
+        }

We have unit test coverage of this code (moving in #2259301: Move OptGroup tests out of FormBuilderTest), we should add to it.

All I see in the OP is an example of a fixed issue. I don't quite understand why this is a *beta blocker*.

Although performance should not be judged by gut, my gut feel is returning a class with __toString() from t() at all times sounds like something that would be problematic for performance at best :/

Fixed form.inc exceptions and added to OptGroupTest

Hopefully fixing up the tests.

Fixing the last exception

+++ b/core/lib/Drupal/Core/Config/StorableConfigBase.php
@@ -183,6 +183,9 @@ protected function castValue($key, $value) {
+    if (is_object($value) && method_exists($value, '__toString')) {
+      $value = (string) $value;
+    }

+++ b/core/modules/block/src/BlockBase.php
@@ -36,7 +36,7 @@ public function label() {
     $definition = $this->getPluginDefinition();
-    return $definition['admin_label'];
+    return (string) $definition['admin_label'];

+++ b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
@@ -1160,11 +1160,11 @@ protected function prepareFilterSelectOptions(&$options) {
       else {
-        $options[$value] = strip_tags(decode_entities($label));
+        $options[$value] = strip_tags(decode_entities((string) $label));
       }

+++ b/core/modules/views_ui/src/ViewListBuilder.php
@@ -229,8 +229,8 @@ protected function getDisplaysList(EntityInterface $view) {
-      if (!empty($definition['admin'])) {
-        $displays[$definition['admin']] = TRUE;
+      if (isset($definition['admin']) && $display_admin = (string) $definition['admin']) {
+        $displays[$display_admin] = TRUE;
       }

All of these changes could use an inline comment to clarify why the code is doing what it is doing.

Aside from that, this looks good to me.

Wondering whether the following hunk could/should check explicitly for instanceof Translation to avoid false-positives?

+++ b/core/lib/Drupal/Core/Config/StorableConfigBase.php
@@ -183,6 +183,9 @@ protected function castValue($key, $value) {
+    if (is_object($value) && method_exists($value, '__toString')) {
+      $value = (string) $value;
+    }

"What could possibly go wrong?" should drive that aspect.

Comments added and some small performance improvements and bringing this inline with the same changes as #1825952: Turn on twig autoescape by default

Thanks! That addresses my concerns.

1. +++ b/core/lib/Drupal/Core/Config/StorableConfigBase.php
   @@ -183,7 +183,10 @@ protected function castValue($key, $value) {
        }
   -    if ((is_scalar($value) || $value === NULL)) {
   +    // If objects can be casted to strings then allow configuration to save their
   +    // string representations.
   +    // @see \Drupal\Core\Annotation\Translation
   +    if ((is_scalar($value) || $value === NULL || (is_object($value) && method_exists($value, '__toString')))) {
   

   This is kind of interesting, it means that we store translatable strings in configuration? Isn't that a problem? I suspect this happens for views for example?

2. +++ b/core/modules/views/src/Tests/Plugin/DisplayTest.php
   @@ -74,6 +74,7 @@ public function testDisplayPlugin() {
   +    $displays['display_test_1']['display_title'] = (string) $displays['display_test_1']['display_title'];
   

   If those are the kind of hacks that we want to live with, so be it ;)

Some sort of profiling would be nice here, but I'm not sure what exactly we should profile.

Also, this is tagged with needs issue summary update and I'm also pretty sure this needs a change record?`So setting back to needs review for now.

For the record, basic_auth in a non-en site (or one that translates en) is currently broken again and this time, it's not the annotations but a loop due to t() calls in the the field definitions in combination with the per-role translation cache. But I think the "solution" there is simply that auth providers can't use load() to get a user object, they have to use UserSession instead.

1,2 - Maybe we can avoid all of this by casting the display_title in the view.

I'm not sure of what to profile either.

Actually, thinking more about this, there's an interesting side effect.

This means that whatever plugin labels we use are translated at runtime and not on discovery, which means that they're no longer cached.

So on the requests that actually need that information we have additional t() calls, which are then however cached by the LocaleLookup cache collector I guess.

However, this essentially means that our plugin definitions are no longer language specific* and we could possibly remove the by-language caching that we're doing by default in a follow-up?

* With exceptions, like modules might be calling t() in alter/build hooks or derivatives?

So something to check might be if/how many plugin labels we're accessing for normal requests, if any? I guess most will be backend only.

@Berdir yep I think we could remove the caching per language - but yep definitely a followup.

+++ b/core/lib/Drupal/Core/Config/StorableConfigBase.php
@@ -146,7 +146,7 @@ protected function validateValue($key, $value) {
-    elseif ($value !== NULL && !is_scalar($value)) {
+    elseif ($value !== NULL && !is_scalar($value) && !(is_object($value) && method_exists($value, '__toString'))) {

In light of #32.1 and #33, is this still needed?

With exceptions, like modules might be calling t() in alter/build hooks or derivatives?

@Berdir yep I think we could remove the caching per language - but yep definitely a followup.

While changing plugin definition cache to not be per language can be follow up, can we really defer *_info(), *_alter(), and derivatives to a follow up? For example, are we sure we want Annotation::get() to return $this, which is not alterable, or do we want to introduce a separate class in the StringTranslation component instead? Not necessarily to replace all cases of t() with, but at least to replace the ones related to plugin definitions, whether they come from annotations or not.

I was going to ask a similar question. I think it's totally fine to document not to use language-specific information in *_alter() (i.e. do not use t()), but instead you should simply be able to do $definition['title'] = new TranslatedString('My title');. I don't think instantiating an Annotation would make much sense in that context.

Something like this?

+++ b/core/lib/Drupal/Core/StringTranslation/Translation.php
@@ -0,0 +1,86 @@
+class Translation {

Wondering if the class name is confusing, given that TranslationInterface exists in the same namespace, but this class doesn't implement it. Maybe TranslationWrapper? Or a better one if someone has a suggestion?

Fixed tests and renamed class to TranslationWrapper

Thanks @alexpott! I don't know what others think, but I personally find that much more natural than linking it to the Annotation class.

Regarding the name: I'd rather think we should rename TranslationInterface and go with Translation (or even StringTranslation for this one), but that might be just me. I can certainly live with TranslationWrapper as well.

Changed hook_stream_wrappers() to use the new TranslationWrapper class and added test based on failure from #2241461: locale + basic_auth results in current_user circular reference

1. +++ b/core/lib/Drupal/Core/Annotation/Translation.php
   @@ -8,7 +8,7 @@
   +use Drupal\Core\StringTranslation\TranslationWrapper as StringTranslation;
   

   i dont see why we need the as statement. StringTranslation is really generic, i like TranslationWrapper more

2. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationWrapper.php
   @@ -0,0 +1,86 @@
   + * Definition of Drupal\Core\StringTranslation\Translation.
   

   should be contains

3. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationWrapper.php
   @@ -0,0 +1,86 @@
   +   * @param array $values
   +   *   Possible array keys:
   +   *   - value (required): the string that is to be translated.
   +   *   - arguments (optional): an array with placeholder replacements, keyed by
   +   *     placeholder.
   +   *   - options (optional): an array of additional options.
   +   */
   +  public function __construct($string, array $arguments = array(), array $options = array()) {
   

   @param needs update

4. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationWrapper.php
   @@ -0,0 +1,86 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function __toString() {
   +    return $this->render();
   

   nice, but i cant see where the inheritdoc comes from :)

5. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationWrapper.php
   @@ -0,0 +1,86 @@
   +  function __sleep() {
   

   needs visibility

6. +++ b/core/modules/block/src/BlockBase.php
   @@ -36,7 +36,9 @@ public function label() {
   +    // Cast the admin label to a string since it is an object.
   +    // @see \Drupal\Core\Annotation\Translation
   +    return (string) $definition['admin_label'];
   
   +++ b/core/modules/views/src/Entity/View.php
   @@ -195,7 +195,9 @@ public function addDisplay($plugin_id = 'page', $title = NULL, $id = NULL) {
   +      // Default display titles are translatable annotations.
   +      // @see \Drupal\Core\Annotation\Translation
   +      'display_title' => (string) $title,
   
   +++ b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
   @@ -1160,11 +1160,13 @@ protected function prepareFilterSelectOptions(&$options) {
   +        // Cast the label to a string since it can be an object.
   +        // @see \Drupal\Core\Annotation\Translation
   +        $options[$value] = strip_tags(decode_entities((string) $label));
   
   +++ b/core/modules/views/src/Plugin/views/relationship/RelationshipPluginBase.php
   @@ -79,7 +79,9 @@ protected function defineOptions() {
   +      // Definition labels are translatable annotations.
   +      // @see \Drupal\Core\Annotation\Translation
   +      $label = (string) $this->definition['label'];
   
   +++ b/core/modules/views_ui/src/ViewListBuilder.php
   @@ -229,8 +229,10 @@ protected function getDisplaysList(EntityInterface $view) {
   +      // Cast the admin label to a string since it is an object.
   +      // @see \Drupal\Core\Annotation\Translation
   +      if (isset($definition['admin']) && $display_admin = (string) $definition['admin']) {
   

   shouldnt the @see point to TranslationWrapper?

7. +++ b/core/modules/locale/locale.module
   @@ -210,9 +211,9 @@ function locale_theme() {
   -      'name' => t('Translation files'),
   +      'name' => new TranslationWrapper('Translation files'),
   ...
   -      'description' => t('Translation files'),
   

   this means we need to train the string extractor to also look for new TranslationWrapper(*) besides t(*) and $this->t(*).

   So a followup for that should be opened

1. Fixed
2. Fixed
3. Fixed
4. Fixed
5. Fixed
6. Fixed and made the comment more consistent
7. I have no idea where to open the issue - I looked in the two queues mentioned on https://localize.drupal.org/issues but could not find any previous issue to update the code parser.

#2016629: Refactor bootstrap to better utilize the kernel broke this.

49: 2244447.49.patch queued for re-testing.

+++ b/core/lib/Drupal/Core/StringTranslation/TranslationWrapper.php
@@ -0,0 +1,86 @@
+   * @param array $values
+   *   Possible array keys:
+   *   - string (required): the string that is to be translated.
+   *   - arguments (optional): an array with placeholder replacements, keyed by
+   *     placeholder.
+   *   - options (optional): an array of additional options.
+   */
+  public function __construct($string, array $arguments = array(), array $options = array()) {

i meant there are 3 @params here and not just one?

Opened #2280843: Add support for D8 TranslationWrapper

Lol fixed

Patch looks great. Just needs a change record, and then I think it can be RTBC.

Working on this change record right now over at Documentation table at sprint.

Draft of change record here: https://drupal.org/node/2281377

+++ b/core/modules/locale/tests/modules/early_translation_test/early_translation_test.services.yml
--- /dev/null
+++ b/core/modules/locale/tests/modules/early_translation_test/src/Auth.php

+++ b/core/modules/locale/tests/modules/early_translation_test/src/Auth.php
@@ -0,0 +1,63 @@
+/**
+ * Test authentication provider.
+ */
+class Auth implements AuthenticationProviderInterface {
...
+  public function __construct(EntityManagerInterface $entity_manager) {
+    $this->userStorage = $entity_manager->getStorage('user');
+  }

It took me a while to figure out why the early_translation_test module is needed and what exactly it helps.

(or well maybe i still did not get it?)

The class comment is wrong. It actually tests parsing the annotation of User's entity storage, that contains a translation wrapped by TranslationWrapper?

Improved the test help.

thanks, thats definitely helpful

@tim.plunkett noticed some things.

+++ b/core/modules/views_ui/src/ViewListBuilder.php
@@ -229,15 +229,15 @@
-      // Cast the admin label to a string since it is an object.
-      // @see \Drupal\Core\StringTranslation\TranslationWrapper
-      if (isset($definition['admin']) && $display_admin = (string) $definition['admin']) {
-        $displays[$display_admin] = TRUE;
+      if (isset($definition['admin'])) {
+        // Cast the admin label to a string since it is an object.
+        // @see \Drupal\Core\StringTranslation\TranslationWrapper
+        $displays[] = (string) $definition['admin'];
       }
     }
 
-    ksort($displays);
-    return array_keys($displays);
+    sort($displays);
+    return $displays;

Much much cleaner! Thanks.

This breaks when the wrapper is used for the needle argument of strpos(). See #2282453: strpos() does not work with TranslationWrapper for a fix.

Folks here might be interested in #2509180: [meta] Various plugin definition alter() hooks incorrectly document to use t() .