Date range separator should be translatable

And here a patch
I added a translatable config to schema, so for correct usage, need to reinstall module datetime_range

+++ b/core/modules/datetime_range/config/schema/datetime_range.schema.yml
@@ -34,6 +34,8 @@ field.formatter.settings.daterange_default:
       type: string

type: label is equivalent of string + translatable.

Looks good...

Actually adding credit for @vijaycs85. (Only committers can actually save values in the Credits & Committing section).

Patch looks good to me, and is RC-eligible since it only touches experimental code.

However, I have a question about the related issue, #2546212: Entity view/form mode formatter/widget settings have no translation UI. Does that prevent the separator we want to add here from being translated at all? If so maybe we should postpone on that issue, because we also need test coverage for this. If there's another way to provide a translation without needing the UI, then the test could exercise that method instead. @vijaycs85, any ideas?

Reg #9: thanks @mpdonadio :)

Reg #10: I don't think we have any other option to add/update the translation without the related issue getting in. We can see the translation UI using Configuration Inspector, but no way to add/update.

Just FYI, I applied the latest patch in #2546212: Entity view/form mode formatter/widget settings have no translation UI and the patch in #6 of this issue and able to see the translation.

We can provide the language override in a test using the API. Is that better than postponing it?

I agree the fix looks good. If we want to test it, we can add an override with the API, the language manager makes it possible, @penyaskito and I have a session on this: http://www.slideshare.net/gabor.hojtsy/drupal-8-multilingual-apis slide 69 ;)

We do not yet have a UI to translate these due to #2546212: Entity view/form mode formatter/widget settings have no translation UI but I would consider the two bugs independent, since we did not make the rest of the entity mode translatable settings postponed either on not having a UI for them.

This adds a test, although I haven't worked much with config translation tests, so am unsure if this is what @Gábor Hojtsy was referring to.

Thanks!

1. +++ b/core/modules/datetime_range/tests/src/Kernel/SeparatorTranslationTest.php
   @@ -0,0 +1,130 @@
   +    $language_manager = $this->container->get('language_manager');
   +    $language_manager->getLanguageConfigOverride('nl', 'core.entity_view_display.entity_test.entity_test.default')
   +      ->set('content.' . $this->fieldStorage->getName() . '.settings.separator', 'NL_TRANSLATED!')
   +      ->save();
   

   Yes, this is exactly what Gábor and I were referring to.

2. +++ b/core/modules/datetime_range/tests/src/Kernel/SeparatorTranslationTest.php
   @@ -0,0 +1,130 @@
   +      ->setlanguage(new Language(['id' => 'nl']));
   

   Should be setLanguage (see uppercase L)

3. +++ b/core/modules/datetime_range/tests/src/Kernel/SeparatorTranslationTest.php
   @@ -0,0 +1,130 @@
   +    $this->verbose($output);
   

   I guess we should delete this debug line.

4. +++ b/core/modules/datetime_range/tests/src/Kernel/SeparatorTranslationTest.php
   @@ -0,0 +1,130 @@
   +    $this->assertContains('NL_TRANSLATED', (string) $output);
   

   Just cosmetic, but let's assert with the exclamation sign if we put that in the translation.

This addresses #18.2 and #18.4. I've left the verbose output as it is similar to other kernel tests in core as mentioned in #19.

Not sure why this was passing locally with that lower case 'l'.

Looks good. +1 to RTBC

Minor rewrite to avoid those 4 lines of duplicate code. Don't think worth a re-roll just for this fix, but if we find some other issue, we can add it:

diff --git a/core/modules/datetime_range/tests/src/Kernel/SeparatorTranslationTest.php b/core/modules/datetime_range/tests/src/Kernel/SeparatorTranslationTest.php
index 155bb61..f6cd4fd 100644
--- a/core/modules/datetime_range/tests/src/Kernel/SeparatorTranslationTest.php
+++ b/core/modules/datetime_range/tests/src/Kernel/SeparatorTranslationTest.php
@@ -34,6 +34,13 @@ class SeparatorTranslationTest extends KernelTestBase {
   protected $field;
 
   /**
+   * The field used in this test class.
+   *
+   * @var \Drupal\Core\Entity\EntityInterface
+   */
+  protected $entity;
+
+  /**
    * {@inheritdoc}
    */
   public static $modules = [
@@ -88,27 +95,24 @@ protected function setUp() {
       'status' => TRUE,
     ])->setComponent($this->fieldStorage->getName(), $display_options)
       ->save();
-  }
 
-  /**
-   * Tests the translation of the range separator.
-   */
-  public function testSeparatorTranslation() {
     // Create an entity.
-    $entity = EntityTest::create([
+    $this->entity = EntityTest::create([
       'name' => $this->randomString(),
       $this->fieldStorage->getName() => [
         'value' => '2016-09-20',
         'end_value' => '2016-09-21',
       ],
     ]);
+  }
+
+  /**
+   * Tests the translation of the range separator.
+   */
+  public function testSeparatorTranslation() {
 
     // Verify the untranslated separator.
-    $display = EntityViewDisplay::collectRenderDisplay($entity, 'default');
-    $build = $display->build($entity);
-    $output = $this->container->get('renderer')->renderRoot($build);
-    $this->verbose($output);
-    $this->assertContains('UNTRANSLATED', (string) $output);
+    $this->assertSeparatorTranslation('UNTRANSLATED');
 
     // Translate the separator.
     ConfigurableLanguage::createFromLangcode('nl')->save();
@@ -118,14 +122,28 @@ public function testSeparatorTranslation() {
       ->set('content.' . $this->fieldStorage->getName() . '.settings.separator', 'NL_TRANSLATED!')
       ->save();
 
+    // Change language and clear translation cache.
     $this->container->get('language.config_factory_override')
       ->setLanguage(new Language(['id' => 'nl']));
-    $this->container->get('cache_tags.invalidator')->invalidateTags($entity->getCacheTags());
-    $display = EntityViewDisplay::collectRenderDisplay($entity, 'default');
-    $build = $display->build($entity);
+    $this->container->get('cache_tags.invalidator')->invalidateTags($this->entity->getCacheTags());
+
+    $this->assertSeparatorTranslation('NL_TRANSLATED!');
+  }
+
+  /**
+   * Helper to assert translation.
+   *
+   * @param string $text
+   *   String translation.
+   *
+   * @throws \Throwable
+   */
+  protected function assertSeparatorTranslation($text) {
+    $display = EntityViewDisplay::collectRenderDisplay($this->entity, 'default');
+    $build = $display->build($this->entity);
     $output = $this->container->get('renderer')->renderRoot($build);
     $this->verbose($output);
-    $this->assertContains('NL_TRANSLATED!', (string) $output);
+    $this->assertContains($text, (string) $output);
   }
 
 }

#25 looks RTBC to me. I actually find it more readable without the patch at #26.

+++ b/core/modules/datetime_range/config/schema/datetime_range.schema.yml
@@ -32,24 +32,27 @@ field.formatter.settings.daterange_default:
+      type: label
...
+      type: label
...
+      type: label

It's not really a semantically a label. Of the existing core types the best fit is type: text.

I'm not sure whether this should be fixed in 8.2.x or is only eligible for 8.3.x because (a) new translatable strings (b) the hook_update_N

+++ b/core/modules/datetime_range/datetime_range.install
@@ -0,0 +1,22 @@
+  \Drupal::service('plugin.manager.field.formatter')->clearCachedDefinitions();

This can be an empty update because a cache flush occurs if there is an empty function.

Let's fix in 8.3.x first and discuss 8.2.x eligibility once we've got this in.

It's not really a semantically a label. Of the existing core types the best fit is type: text.

Well, our handling of text is as a multiline text widget, and this is not that, is it? :) We can introduce yet another single line translatable string type to have a semantically more correct name for it for these cases, but would that simplify the choice of the type for single line translatable strings or complicate it?

As pointed out in IRC we've (ab)used label for this already so ignore #28 - we still need to address #29 since clearing the cache twice is unnecessary.

Yay, thanks!

+++ b/core/modules/datetime_range/datetime_range.install
@@ -0,0 +1,22 @@
+/**
+ * @addtogroup updates-8.3.x
+ * @{
+ */
+
+/**
+ * Clear caches to ensure schema changes are read.
+ */
+function datetime_range_update_8301() {
+  // Empty update to cause a cache rebuild.
+}
+
+/**
+ * @} End of "addtogroup updates-8.3.x".
+ */

Ah of course this is experimental... this should be an 8.2.x update - we can't change update numbers like that. So we need to do it before committing to 8.3.x. And thne we can cherry-pick for 8.2.x

Please correct me if I am wrong : With a date range field I am getting date like "Tue, 10/11/2016 - 12:00 - Mon, 10/31/2016 - 12:00 " . Is it expected to come 'and' instead of '-' . Please see the screen. if '-' is good to show . Then I think we don't need to translate.

I think this is back to RTBC.

The steps for reproducing the issue need to be updated. I couldn't reproduce the exact issue as of now.

Since this is experimental I think it's better if it goes into 8.2.x - I'm assuming @alexpott forgot to change the version in #37 but in case there was an explicit reason to leave it against 8.3.x, assigning for a chance to comment.

Sorry only realised this after posting the review and stepping afk.

We shouldn't use hook_update_N() for empty updates to trigger a cache flush - it adds all the potential complexity of schema versions between minor branches, conflicts with other patches, inconsistencies if there's a revert etc. hook_post_update_NAME() achieves the same thing without most of that trouble.

That's great, thanks!

Committed and pushed a55967d to 8.3.x and da66644 to 8.2.x. Thanks!