Problem/Motivation

Date formats cannot be localized in Drupal 8. There are 3 UIs that lie to you about supporting that, but none of them do.

1. The built-in UI allows you to assign the format to different languages, but not to have different config for the same machine.
2. There is a hidden secret, forgotten UI that allows you to "translate" the formats but then it overwrites the cross-language setting in 1.
3. Config translation has a tab on date formats to translate, but that would only contain the date format label.

Proposed resolution

Use config translation (module). If we mark the date format translatable in the schema, we can expose that on localize.drupal.org and config translation module will pick it up as well. We need to have it as a translatable type, but using an existing type like label would not allow to display a proper widget for it, so defining a new derivative type lets us display a good UI in config_translation.

Remaining tasks

User interface changes

None. The UI change will be

API changes

#2052193: [META] Date format localisation is a huge mess, conflicts, does not work, regressed
#2099997: Use config translation to translate date formats
#2117711: Add tests to date format translation UI from the config_translation module is postponed on this

Files: 
CommentFileSizeAuthor
#64 date-formats-2098089-64.patch8.71 KBYesCT
PASSED: [[SimpleTest]]: [MySQL] 59,662 pass(es). View
#64 interdiff-60-64.txt1.68 KBYesCT
#60 date-formats-2098089-60.patch9.9 KBshnark
PASSED: [[SimpleTest]]: [MySQL] 59,138 pass(es). View
#60 interdiff-56-60.txt2.88 KBshnark
#59 interdiff.txt2.57 KBtim.plunkett
#56 date-formats-2098089-56.patch9.88 KBSchnitzel
PASSED: [[SimpleTest]]: [MySQL] 59,459 pass(es). View
#56 interdiff-54-56.txt602 bytesSchnitzel
#54 date-formats-2098089-54.patch9.89 KBSchnitzel
PASSED: [[SimpleTest]]: [MySQL] 59,048 pass(es). View
#54 interdiff-44-54.txt2.79 KBSchnitzel
#52 date-formats-2098089-52.patch9.88 KBSchnitzel
FAILED: [[SimpleTest]]: [MySQL] 59,607 pass(es), 0 fail(s), and 335 exception(s). View
#52 interdiff-44-52.txt2.41 KBSchnitzel
#44 date-formats-2098089-44.patch8.95 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 59,477 pass(es). View
#44 interdiff.txt1.33 KBtim.plunkett
#39 2098089-date-format-type.33-39.interdiff.txt519 bytespenyaskito
#39 2098089-date-format-type-39.only-test.patch2.39 KBpenyaskito
FAILED: [[SimpleTest]]: [MySQL] 59,920 pass(es), 6 fail(s), and 2 exception(s). View
#39 2098089-date-format-type-39.patch9.42 KBpenyaskito
PASSED: [[SimpleTest]]: [MySQL] 59,066 pass(es). View
#36 2098089-date-format-type.33-35.interdiff.txt3.05 KBpenyaskito
#36 2098089-date-format-type-35.only-test.patch2.39 KBpenyaskito
FAILED: [[SimpleTest]]: [MySQL] 59,480 pass(es), 6 fail(s), and 2 exception(s). View
#36 2098089-date-format-type-35.patch10.24 KBpenyaskito
PASSED: [[SimpleTest]]: [MySQL] 59,054 pass(es). View
#33 2098089-date-format-type.31-33.interdiff.txt2.62 KBpenyaskito
#33 2098089-date-format-type-33.only-test.patch2.39 KBpenyaskito
FAILED: [[SimpleTest]]: [MySQL] 59,028 pass(es), 6 fail(s), and 2 exception(s). View
#33 2098089-date-format-type-33.patch9.38 KBpenyaskito
PASSED: [[SimpleTest]]: [MySQL] 59,408 pass(es). View
#31 2098089-date-format-type.30-31.interdiff.txt3.19 KBpenyaskito
#31 2098089-date-format-type-31.only-test.patch2.39 KBpenyaskito
FAILED: [[SimpleTest]]: [MySQL] 59,407 pass(es), 6 fail(s), and 2 exception(s). View
#31 2098089-date-format-type-31.patch8.19 KBpenyaskito
PASSED: [[SimpleTest]]: [MySQL] 59,674 pass(es). View
#30 2098089-date-format-type.17-30.interdiff.txt1.85 KBpenyaskito
#30 2098089-date-format-type-30.only-test.patch2.39 KBpenyaskito
FAILED: [[SimpleTest]]: [MySQL] 59,532 pass(es), 6 fail(s), and 2 exception(s). View
#30 2098089-date-format-type-30.patch6.11 KBpenyaskito
PASSED: [[SimpleTest]]: [MySQL] 59,706 pass(es). View
#25 2098089-date-format-type.23-25.interdiff.txt1.43 KBpenyaskito
#25 2098089-date-format-type.23-25.interdiff.txt1.43 KBpenyaskito
#25 2098089-date-format-type-25.patch7.52 KBpenyaskito
PASSED: [[SimpleTest]]: [MySQL] 59,252 pass(es). View
#23 2098089-date-format-type-23.patch7.5 KBpenyaskito
FAILED: [[SimpleTest]]: [MySQL] 57,782 pass(es), 121 fail(s), and 168 exception(s). View
#23 2098089-date-format-type-23.only-test.patch2.37 KBpenyaskito
FAILED: [[SimpleTest]]: [MySQL] 58,880 pass(es), 6 fail(s), and 2 exception(s). View
#23 2098089-date-format-type.20-23.interdiff.txt728 bytespenyaskito
#22 2098089-date-format-type-22.patch8 KBpenyaskito
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: tests were executed, but no results were found. View
#22 2098089-date-format-type-22.only-test.patch2.37 KBpenyaskito
FAILED: [[SimpleTest]]: [MySQL] 59,272 pass(es), 6 fail(s), and 2 exception(s). View
#22 2098089-date-format-type.20-22.interdiff.txt0 bytespenyaskito
#20 2098089-date-format-type.18-20.interdiff.txt2.94 KBpenyaskito
#20 2098089-date-format-type-20.only-test.patch2.37 KBpenyaskito
FAILED: [[SimpleTest]]: [MySQL] 59,254 pass(es), 6 fail(s), and 2 exception(s). View
#20 2098089-date-format-type-20.patch8 KBpenyaskito
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: tests were executed, but no results were found. View
#19 interdiff-17-nopatch.txt3.2 KBYesCT
#18 2098089-date-format-type.17-18.interdiff.txt5.04 KBpenyaskito
#18 2098089-date-format-type-18.only-test.patch2.37 KBpenyaskito
FAILED: [[SimpleTest]]: [MySQL] 58,880 pass(es), 6 fail(s), and 2 exception(s). View
#18 2098089-date-format-type-18.patch7.47 KBpenyaskito
FAILED: [[SimpleTest]]: [MySQL] 59,419 pass(es), 1 fail(s), and 0 exception(s). View
#17 2098089-date-format-type.12-17.interdiff.txt1.68 KBpenyaskito
#17 2098089-date-format-type-17.only-test.patch2.39 KBpenyaskito
FAILED: [[SimpleTest]]: [MySQL] 58,870 pass(es), 6 fail(s), and 2 exception(s). View
#17 2098089-date-format-type-17.patch6.08 KBpenyaskito
PASSED: [[SimpleTest]]: [MySQL] 59,188 pass(es). View
#12 2098089-date-format-type.10-12.interdiff.txt2.35 KBpenyaskito
#12 2098089-date-format-type-12.only-test.patch2.39 KBpenyaskito
FAILED: [[SimpleTest]]: [MySQL] 59,108 pass(es), 6 fail(s), and 2 exception(s). View
#12 2098089-date-format-type-12.patch5.75 KBpenyaskito
FAILED: [[SimpleTest]]: [MySQL] 54,305 pass(es), 2,181 fail(s), and 199 exception(s). View
#10 2098089-date-format-type.9-10.interdiff.txt1.95 KBpenyaskito
#10 2098089-date-format-type-10.only-test.patch2.36 KBpenyaskito
FAILED: [[SimpleTest]]: [MySQL] 58,873 pass(es), 6 fail(s), and 2 exception(s). View
#10 2098089-date-format-type-10.patch5.38 KBpenyaskito
FAILED: [[SimpleTest]]: [MySQL] 53,981 pass(es), 2,189 fail(s), and 205 exception(s). View
#9 2098089-date-format-type.5-9.interdiff.txt2.92 KBpenyaskito
#9 2098089-date-format-type-9.patch3.43 KBpenyaskito
FAILED: [[SimpleTest]]: [MySQL] 58,923 pass(es), 1 fail(s), and 0 exception(s). View
#5 date-format-type-5.patch1.07 KBpenyaskito
PASSED: [[SimpleTest]]: [MySQL] 58,995 pass(es). View
date-format-type.patch1.07 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 58,983 pass(es). View

Comments

Gábor Hojtsy’s picture

Status: Active » Reviewed & tested by the community
+++ b/core/modules/system/config/schema/system.data_types.schema.yml
@@ -53,6 +53,12 @@ text:
+  label: PHP date format

Needs quotes.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs review

Wups :D

Jose Reyero’s picture

It looks to me like a good first step.

webflo’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +blocker

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

penyaskito’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.07 KB
PASSED: [[SimpleTest]]: [MySQL] 58,995 pass(es). View

Added quotes to the schema.

Gábor Hojtsy’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/config/schema/system.data_types.schema.yml
@@ -53,6 +53,12 @@ text:
+# PHP Date format string that is translatable and has a specific UI

Dot missing at end of line.

Gábor Hojtsy’s picture

Issue tags: +Needs tests

Actually, you know, let's do some tests for this! Once the date format is translatable, the locale_storage() should end up with the source of the date format, it may be good to add some basic test stuff for this. Locale has some tests for strings appearing in the DB when a new language is added / module is enabled, etc. In this case, the source is in system module so it should already be there, easier to test! Let's do that!

penyaskito’s picture

Assigned: Unassigned » penyaskito

Working on the tests.

penyaskito’s picture

FileSize
3.43 KB
FAILED: [[SimpleTest]]: [MySQL] 58,923 pass(es), 1 fail(s), and 0 exception(s). View
2.92 KB

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.

penyaskito’s picture

Status: Needs work » Needs review
FileSize
5.38 KB
FAILED: [[SimpleTest]]: [MySQL] 53,981 pass(es), 2,189 fail(s), and 205 exception(s). View
2.36 KB
FAILED: [[SimpleTest]]: [MySQL] 58,873 pass(es), 6 fail(s), and 2 exception(s). View
1.95 KB

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

Status: Needs review » Needs work

The last submitted patch, 2098089-date-format-type-10.patch, failed testing.

penyaskito’s picture

Status: Needs work » Needs review
FileSize
5.75 KB
FAILED: [[SimpleTest]]: [MySQL] 54,305 pass(es), 2,181 fail(s), and 199 exception(s). View
2.39 KB
FAILED: [[SimpleTest]]: [MySQL] 59,108 pass(es), 6 fail(s), and 2 exception(s). View
2.35 KB

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

Status: Needs review » Needs work

The last submitted patch, 2098089-date-format-type-12.patch, failed testing.

YesCT’s picture

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.

penyaskito’s picture

No worries, still working on it.

penyaskito’s picture

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.

penyaskito’s picture

Status: Needs work » Needs review
FileSize
6.08 KB
PASSED: [[SimpleTest]]: [MySQL] 59,188 pass(es). View
2.39 KB
FAILED: [[SimpleTest]]: [MySQL] 58,870 pass(es), 6 fail(s), and 2 exception(s). View
1.68 KB

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

penyaskito’s picture

FileSize
7.47 KB
FAILED: [[SimpleTest]]: [MySQL] 59,419 pass(es), 1 fail(s), and 0 exception(s). View
2.37 KB
FAILED: [[SimpleTest]]: [MySQL] 58,880 pass(es), 6 fail(s), and 2 exception(s). View
5.04 KB

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.

YesCT’s picture

FileSize
3.2 KB

[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.
penyaskito’s picture

FileSize
8 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: tests were executed, but no results were found. View
2.37 KB
FAILED: [[SimpleTest]]: [MySQL] 59,254 pass(es), 6 fail(s), and 2 exception(s). View
2.94 KB

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.

Status: Needs review » Needs work

The last submitted patch, 2098089-date-format-type-20.patch, failed testing.

penyaskito’s picture

Status: Needs work » Needs review
FileSize
0 bytes
2.37 KB
FAILED: [[SimpleTest]]: [MySQL] 59,272 pass(es), 6 fail(s), and 2 exception(s). View
8 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: tests were executed, but no results were found. View

Oops, forgot to remove the contructor parameter.

penyaskito’s picture

FileSize
728 bytes
2.37 KB
FAILED: [[SimpleTest]]: [MySQL] 58,880 pass(es), 6 fail(s), and 2 exception(s). View
7.5 KB
FAILED: [[SimpleTest]]: [MySQL] 57,782 pass(es), 121 fail(s), and 168 exception(s). View

The right patches...

Status: Needs review » Needs work

The last submitted patch, 2098089-date-format-type-23.only-test.patch, failed testing.

penyaskito’s picture

Status: Needs work » Needs review
FileSize
7.52 KB
PASSED: [[SimpleTest]]: [MySQL] 59,252 pass(es). View
1.43 KB
1.43 KB

Fixed the missing import and renaming for consistency.

Gábor Hojtsy’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/locale/lib/Drupal/locale/LocaleServiceProvider.php
    @@ -0,0 +1,35 @@
    +/**
    + * Defines the LanguageTest service provider.
    + */
    +class LocaleServiceProvider implements ServiceModifierInterface, ServiceProviderInterface {
    

    Not LanguageTest :)

  2. +++ b/core/modules/locale/lib/Drupal/locale/LocaleServiceProvider.php
    @@ -0,0 +1,35 @@
    +    // Overrides date class to provide localized dates.
    +    $definition = $container->getDefinition('date');
    +    $definition->setClass('Drupal\locale\LocaleDate');
    

    I see you have this child class so that the language context can be used, however that is only defined by language module. Doing this as a class registry replacement sounds a bit hackish to me honestly.

    I'd explore two other ways:

    - should we inject the date class and inject the proper one?

    - should the language context be moved up to the system level (since the language class is already there and language is universally understood on the field, etc. levels)

    I think the re-simplification of config that is ongoing is pointing in the direction of the system level language support in config, so that sounds like would be a good interim step here. Then there is no reason to override the class, since the base class can do the same thing. IMHO just move the language context to a global thing like context free and users are :)

  3. +++ b/core/modules/system/config/schema/system.data_types.schema.yml
    @@ -53,6 +53,12 @@ text:
    +# PHP Date format string that is translatable and has a specific UI.
    

    It does not really have a specific UI by the virtue of having a separate type, that is not really up to the schema, so I'd leave that ("has a specific UI") out.

Gábor Hojtsy’s picture

So #2098119: Replace config context system with baked-in locale support and single-event based overrides will change how language overrides are accessed / skipped, but the general logic and especially tests in this patch should still apply. I'd encourage doing a simpler language solution with #2098119: Replace config context system with baked-in locale support and single-event based overrides in mind, but not wait on #2098119: Replace config context system with baked-in locale support and single-event based overrides since the other solutions here will still apply 1-1.

Gábor Hojtsy’s picture

The combination of this with #2099997: Use config translation to translate date formats is currently targeted to provide an actual date translation experience in core. Once config translation is landing in core, we can remove the useless interfaces on language/translation and the very confused backend code as well. There are lots of confused random code that we can remove. Let's do this!

Gábor Hojtsy’s picture

Issue summary: View changes

Add meta

Gábor Hojtsy’s picture

Assigned: penyaskito » Unassigned

Discussed with @penyaskito. He does not have time for this this week, but indicated we can start from #17. Indeed. Moving the language context to the base config layer instead of in language module would help there, so we can use it without a condition on language module. Someone wants to take a stab at this starting from #17?

penyaskito’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
6.11 KB
PASSED: [[SimpleTest]]: [MySQL] 59,706 pass(es). View
2.39 KB
FAILED: [[SimpleTest]]: [MySQL] 59,532 pass(es), 6 fail(s), and 2 exception(s). View
1.85 KB

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

penyaskito’s picture

Assigned: Unassigned » penyaskito
FileSize
8.19 KB
PASSED: [[SimpleTest]]: [MySQL] 59,674 pass(es). View
2.39 KB
FAILED: [[SimpleTest]]: [MySQL] 59,407 pass(es), 6 fail(s), and 2 exception(s). View
3.19 KB

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

Gábor Hojtsy’s picture

Getting there. Only one code style and one logic issue :)

  1. +++ b/core/lib/Drupal/Core/Datetime/Date.php
    @@ -127,11 +127,33 @@ public function format($timestamp, $type = 'medium', $format = '', $timezone = N
    +   * @param string $format The machine name of the date format.
    +   * @param string $langcode The langcode of the language to use.
    +   * If NULL, we load the interface language format pattern.
    +   * @return string The pattern for the date format in the given language.
    

    Newlines/indent before param descriptions, empty line before @return, etc. missing :)

  2. +++ b/core/lib/Drupal/Core/Datetime/Date.php
    @@ -127,11 +127,33 @@ public function format($timestamp, $type = 'medium', $format = '', $timezone = N
    +    // If a langcode is not given, we use the interface language.
    +    $needs_language_context = !empty($langcode);
    +    if (!$needs_language_context) {
    +      $langcode = $this->languageManager->getLanguage(Language::TYPE_INTERFACE)->id;
    +    }
    +    if (!isset($this->dateFormats[$format][$langcode])) {
    +      // Enter a language specific context for the language if some language is
    +      // given, so the right date format is loaded.
    +      if ($needs_language_context) {
    +        $language_context = config_context_enter('Drupal\Core\Config\Context\LanguageConfigContext');
    +        $language_context->setLanguage(new Language(array('id' => $langcode)));
    +      }
    +      $this->dateFormats[$format][$langcode] = $this->dateFormatStorage->load($format);
    +      if ($needs_language_context) {
    +        config_context_leave();
    +      }
    

    There is only one problem with this code. It assumes if no language was given, then we should cache the option under the interface language code negotiated, BUT it does not make sure we go into that context.

    So if the function was invoked in a different context then the langcode we cache the value under will be different than the language context used to look up the format. So either we need to always go into that context or *better*, attempt to retrieve the language from the context, and if something is set there than use that language, otherwise go into the interface language context.

    You can get the context from the config factory by invoking getContext() on it.

penyaskito’s picture

FileSize
9.38 KB
PASSED: [[SimpleTest]]: [MySQL] 59,408 pass(es). View
2.39 KB
FAILED: [[SimpleTest]]: [MySQL] 59,028 pass(es), 6 fail(s), and 2 exception(s). View
2.62 KB

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.

Gábor Hojtsy’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Datetime/Date.php
@@ -127,11 +128,43 @@ public function format($timestamp, $type = 'medium', $format = '', $timezone = N
+      $context = config_context();
+      if ($context !== NULL && $contextLanguage = $context->get(LanguageConfigContext::LANGUAGE_KEY)) {
+        $langcode = $contextLanguage;

If empty($langcode) and these conditions match is the only case when we don't need to explicitly enter a language context later on. So if we are already in some language context and know the langcode from there. If that is not the case, then we need to enter a specific context, since we are not sure if the context we are in matches the langcode we think we get the data in, right? :)

Gábor Hojtsy’s picture

The config translation module is fully capable now to provide a user interface for the new date_format type, see #2099997: Use config translation to translate date formats and is awaiting more tests to be written as soon as this lands in core (#2117711: Add tests to date format translation UI).

penyaskito’s picture

Status: Needs work » Needs review
FileSize
10.24 KB
PASSED: [[SimpleTest]]: [MySQL] 59,054 pass(es). View
2.39 KB
FAILED: [[SimpleTest]]: [MySQL] 59,480 pass(es), 6 fail(s), and 2 exception(s). View
3.05 KB

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

Gábor Hojtsy’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Datetime/Date.php
@@ -127,11 +137,44 @@ public function format($timestamp, $type = 'medium', $format = '', $timezone = N
+        $language_context = config_context_enter('Drupal\Core\Config\Context\LanguageConfigContext');
...
+        config_context_leave();

Use the config factory to do these too and then this all looks like done to me :)

Gábor Hojtsy’s picture

So after talking about this more on IRC, config_context_*() does a lot of utility stuff, like ensuring the class exists/implements the context interface and also reusing context instances instead of instantiating new ones all the time. Those are things we don't get from the injected config factory *at all* because they are only in the procedural function. #1926968: move config_context_enter to a container-aware service was won't fixed earlier, so it is not likely we are getting a solution on the factory or in the form of another service. Finally, #2098119: Replace config context system with baked-in locale support and single-event based overrides may remove the whole context system altogether, so these functions will definitely be looked at again.

So I think we should get back to #33 + the 1 line fix for my comment in #34 that you already included in #36. No injection of the config factory and we can keep config_context() as a global function as you added it. That would be RTBC as in as good as we can do under current APIs.

penyaskito’s picture

Status: Needs work » Needs review
FileSize
9.42 KB
PASSED: [[SimpleTest]]: [MySQL] 59,066 pass(es). View
2.39 KB
FAILED: [[SimpleTest]]: [MySQL] 59,920 pass(es), 6 fail(s), and 2 exception(s). View
519 bytes

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

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Yay! Thanks. As written in #38 this is as good as we can get with our API now. Code looks good and definitely makes date format translation work and shippable.

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work

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.

Gábor Hojtsy’s picture

Status: Needs work » Reviewed & tested by the community

@tim.plunkett: as per #38, the config context functions do not have an injectable equivalent and in fact @alexpott closed down an issue where that was proposed. So we could inject config factory to *get* context but not to enter/leave. That would mean we may use different config factories for different operations. Given the current context API this is the best possible way AFAIS. Please advise in more concrete terms otherwise. More info in #38 :)

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work

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.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.33 KB
8.95 KB
PASSED: [[SimpleTest]]: [MySQL] 59,477 pass(es). View

Better yet.

tim.plunkett’s picture

Issue summary: View changes

Added #2099997 to the summary for tracking.

Status: Needs review » Needs work

The last submitted patch, date-formats-2098089-44.patch, failed testing.

tim.plunkett’s picture

+++ 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.

tim.plunkett’s picture

Issue tags: +Needs tests

.

Gábor Hojtsy’s picture

Thanks for the update. re #43, the point of injecting the config factory is so it can be different from the global one on the container, so I'd be surprised if sometimes using the global one even though we let it injected would be a good idea. It was explored above and then got discarded.

Now back to fixing the tests/bug :)

penyaskito’s picture

Assigned: penyaskito » Unassigned

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

YesCT’s picture

[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.

Schnitzel’s picture

Status: Needs work » Needs review

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

Schnitzel’s picture

FileSize
2.41 KB
9.88 KB
FAILED: [[SimpleTest]]: [MySQL] 59,607 pass(es), 0 fail(s), and 335 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, date-formats-2098089-52.patch, failed testing.

Schnitzel’s picture

Status: Needs work » Needs review
FileSize
2.79 KB
9.89 KB
PASSED: [[SimpleTest]]: [MySQL] 59,048 pass(es). View

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

tim.plunkett’s picture

+++ 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

Schnitzel’s picture

FileSize
602 bytes
9.88 KB
PASSED: [[SimpleTest]]: [MySQL] 59,459 pass(es). View

addressing #55

YesCT’s picture

Issue tags: -Needs tests

#52 removed the code that needed tests.

so removing the needs tests tag

Status: Reviewed & tested by the community » Needs work

The last submitted patch, date-formats-2098089-56.patch, failed testing.

tim.plunkett’s picture

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

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.

shnark’s picture

Status: Needs work » Needs review
FileSize
2.88 KB
9.9 KB
PASSED: [[SimpleTest]]: [MySQL] 59,138 pass(es). View

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.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community
Gábor Hojtsy’s picture

Issue tags: +Spark

Worked on this as part of the Spark project as well. Tagging for that.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  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?

YesCT’s picture

Status: Needs work » Needs review
FileSize
1.68 KB
8.71 KB
PASSED: [[SimpleTest]]: [MySQL] 59,662 pass(es). View

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.

Schnitzel’s picture

Status: Needs review » Reviewed & tested by the community

nice!

Gábor Hojtsy’s picture

Assigned: Unassigned » alexpott
webchick’s picture

Assigned: alexpott » Unassigned
Status: Reviewed & tested by the community » Fixed

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!

webchick’s picture

Issue summary: View changes

noting config trans has a postponed issue

Gábor Hojtsy’s picture

Issue summary: View changes
Issue tags: -sprint
Gábor Hojtsy’s picture

Issue tags: +Configuration context

Status: Fixed » Closed (fixed)

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