Date formats cannot be translated (the core UIs are useless)

It looks to me like a good first step.

Yes. Lets add this new translatable type to proceed with config translation.

Added quotes to the schema.

Working on the tests.

This test should be right, but I think I hit a bug.

Drupal\Core\Datetime\Date::format() is passing the language given to DrupalDateTime::createFromTimestamp, but no to dateFormat(). There, we load the format and apply it, but always the default format and not the translation if any.

Added context for taking the language into account. Added only-test patch for showing that the current behavior is wrong.

Clean up and docblocks. I'm not sure if the failing tests are really related.

head is green, BookTest passes on head locally, and fails with the patch. So it's the patch for sure. But I dont know why yet.

+++ b/core/lib/Drupal/Core/Datetime/Date.php
@@ -127,11 +127,18 @@ public function format($timestamp, $type = 'medium', $format = '', $timezone = N
+      // Enter a language specific context for the language, so the right date format is loaded.
+      $language_context = config_context_enter('Drupal\language\LanguageConfigContext');
+      $language_context->setLanguage(new Language(array('id' => $langcode)));
+      $this->dateFormats[$format][$langcode] = $this->dateFormatStorage->load($format);
+      config_context_leave();

we are getting a lot of things here, maybe some are not returning the correct things, or able to access them. I really need to figure out how to debug with step through. But time for dinner now.

No worries, still working on it.

Silly me, we are trying to load the switch to a different language context no matter if the language module is there or not, so every test that does not activate the language module is failing.

Patch will follow.

Take into account if there is any language given and if the language is given for applying the configuration context.

In this patch I attempt to redefine the date service in locale, so we can remove the language module existence from the Drupal\Core\DateTime\Date class.
Locally it is not working however.

[edit: posted without seeing 18.]

1. +++ b/core/lib/Drupal/Core/Datetime/Date.php
   @@ -136,15 +136,23 @@ public function format($timestamp, $type = 'medium', $format = '', $timezone = N
   +    $needs_language_context = !empty($langcode);
   

   seems like we could se this later inside the if !isset, it is not used otherwise.

2. +++ b/core/lib/Drupal/Core/Datetime/Date.php
   @@ -136,15 +136,23 @@ public function format($timestamp, $type = 'medium', $format = '', $timezone = N
   +      $needs_language_context = $needs_language_context &&
   +          \Drupal::moduleHandler()->moduleExists('language') && !empty($langcode);
   

   should the moduleHnadler be injected?

   if it is, hmm. there is no contructor, so how do we get moduleHandler from the container?

3. +++ b/core/lib/Drupal/Core/Datetime/Date.php
   @@ -136,15 +136,23 @@ public function format($timestamp, $type = 'medium', $format = '', $timezone = N
   +        $language_context = config_context_enter('Drupal\language\LanguageConfigContext');
   +        $language_context->setLanguage(new Language(array('id' => $langcode)));
   

   these two lines were already there. I think the recommendation is if it is used more than once, then use a use statement.

4. side note (not part of this issue):
   maybe we do not need the entityManager here, and instead just pass in the actual format.
   I wonder if there is an issue for that.

Renamed LocalizedDateServiceProvider to LocaleServiceProvider because doesn't work if the ServiceProvider has not the same name than the module (thanks Berdir!).
Renamed LocalizedDate to LocaleDate for consistency.

Replying comments in #19:

1 and 2 are not relevant anymore.
3. I don't get it. Do you mean 'use Drupal\language\LanguageConfigContext' and pass only 'LanguageConfigContext'? In any case with the new patch is not relevant, is only used once.
4. No idea.

I took the docblocks from YesCT patch at #19, but my interdiff is with #18.

Oops, forgot to remove the contructor parameter.

The right patches...

Fixed the missing import and renaming for consistency.

#17 cleanup. Applied feedback from the other patches+reviews to this one.

Moved the LanguageConfigContext to the base config layer and removing the check for language module.

Added the docblock fixes from #32 and the logic for handling if a config context is active at the moment.
Added a config_context() method for avoiding coupling to ConfigFactory.

Removed config_context() from #33 and instead injected the ConfigFactory.
Fixed the pending logic issue.

Patch according to conclusions at #38.
Interdiff with #33.

Switching to injecting config_context_enter/config_context_leave is fine for a followup, but...

1. +++ b/core/includes/config.inc
   @@ -179,6 +179,19 @@ function config_context_leave() {
   +function config_context() {
   +  drupal_container()
   +    ->get('config.factory')
   +    ->getContext();
   +}
   

   Please don't introduce a new procedural wrapper (especially one using drupal_container()!)

2. +++ b/core/lib/Drupal/Core/Datetime/Date.php
   @@ -127,11 +128,44 @@ public function format($timestamp, $type = 'medium', $format = '', $timezone = N
   +      $context = config_context();
   

   Date is a service, please inject this.

That would mean we may use different config factories for different operations.

I'm reasonably sure that's BS.

If you're going to add a random procedural wrapper for a service, at least use \Drupal::service() and mark it deprecated.

Better yet.

Added #2099997 to the summary for tracking.

+++ b/core/includes/config.inc
@@ -179,19 +179,6 @@ function config_context_leave() {
-function config_context() {
-  drupal_container()

+++ b/core/lib/Drupal/Core/Datetime/Date.php
@@ -144,7 +144,8 @@ protected function dateFormat($format, $langcode = NULL) {
       if ($context !== NULL && $contextLanguage = $context->get(LanguageConfigContext::LANGUAGE_KEY)) {

Before, config_context() forgot to actually return anything. So it was *always* NULL. Now that I've fixed the bug, the tests fail.

.

I probably won't be able of dedicating more time to this one this week, so unassigning.

[edit]
I'm going to read through this. (in badcamp sprint room)

#2098119: Replace config context system with baked-in locale support and single-event based overrides seems like it would handle the @todo from that last interdiff.

#44: date-formats-2098089-44.patch queued for re-testing.

So I started to write a test to test if the loading of the langcode via context actually works (which should then fail with the code before #44).
But during writing this I found out that dateFormat() is never called with $langcode = NULL or a non existing $langcode.
So I discussed with @alexpott and he told me that I should remove then the whole Code, which is now in the attached patch.

I'm not sure why we actually have this whole system which checks for !empty($langcode).

Maybe I'm also wrong and there is a case where $langcode = NULL can happen, but I did not found any case.

oh, one small change is still necessary, but actually $langcode is used in the same line, so I think this is fine like this.

+++ b/core/lib/Drupal/Core/Datetime/Date.php
@@ -117,7 +117,7 @@ public function format($timestamp, $type = 'medium', $format = '', $timezone = N
+      $format = $this->dateFormat('fallback', $langcode)->getPattern($key, $langcode);

I think you just wanted
$format = $this->dateFormat('fallback', $langcode)->getPattern($key);

getPattern has no second param

addressing #55

#52 removed the code that needed tests.

so removing the needs tests tag

For anyone curious, here is the interdiff between my patch in #44 and the most recent one, whitespace ignored.

This was RTBC before, I think mine were the only complaints, and that has been resolved. RTBC once more.

EDIT: I crossposted, but also queued for retesting. The error was in the update.module tests, which can time out now and then. Wait for green.

I noticed coding style cleanup that could have been done in some of the changed lines of the patch.
None of these are functional changes, just nits.

1. +++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleConfigTranslationTest.php
   @@ -89,6 +89,38 @@ function testConfigTranslation() {
   +    $site_name = $this->randomName(20);
   

   Unused variable

2. +++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleConfigTranslationTest.php
   @@ -89,6 +89,38 @@ function testConfigTranslation() {
   +    $formatted_date = format_date(494015820, $type = 'medium', NULL, NULL, $langcode = $langcode);
   

   $langcode = $langcode?

here we go.
addressed those.
also found an un-used use that was added in this patch.

+++ b/core/lib/Drupal/Core/Datetime/Date.php
@@ -8,6 +8,7 @@
+use Drupal\Core\Config\Context\LanguageConfigContext;

@@ -127,11 +128,27 @@ public function format($timestamp, $type = 'medium', $format = '', $timezone = N
+      $language_context = config_context_enter('Drupal\Core\Config\Context\LanguageConfigContext');

we dont need to "use" it to use it like this.

nice!

That was pretty straight-forward feedback, and it's all been addressed, so I don't think it needs to go back to alex for a second review.

This looks great to me, thanks for the expanded test coverage.

Committed and pushed to 8.x!

noting config trans has a postponed issue