Problem/Motivation

To reproduce the bug:

1) You need config translation enabled :)
2) Add a new language.
3) Add a new date format.
4) Edit the new date format and go to "Translate date format".
5) Click the Add link for the new language added.
6) Expected : Date format Label + Format fields.
We have only the label field.
7) Take a look at the other formats by default (medium, long...) and as you can see the PHP date format field appears. If you add a new format for the translation(medium for example), in this case you will only add the php pattern format to the language config file (i.e. the file for FR is language.config.fr.system.date_format.medium.yml)

I found the problem while working on #2031183: Improve test coverage for node authored on widget.

CommentFileSizeAuthor
#22 2204207-22.patch13.65 KBpenyaskito
#22 2204207-22.only-test.patch5.45 KBpenyaskito
#22 interdiff.20-22.txt1.97 KBpenyaskito
#21 2204207-20.patch13.65 KBpenyaskito
#15 2204207-15.patch13.7 KBmarthinal
#15 2204207-15-only-test-should-fail.patch4.72 KBmarthinal
#14 2204207-14.patch8.56 KBmarthinal
#14 interdiff-2204207-12-14.txt1.43 KBmarthinal
#12 2204207-12.patch8.77 KBmarthinal
#10 2204207-10.patch7.35 KBmarthinal
#7 config-translation-date-format-weirdness.png43.31 KBtstoeckler
#6 2204207-6-do-not-test.patch2.15 KBmarthinal
#4 2204207-4-do-not-test.patch512 bytesmarthinal
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

marthinal’s picture

marthinal CreditAttribution: marthinal commented
Title: Missing format field when translating a date format. » Missing format field when translating a date format (when php-intl is enabled).
marthinal’s picture

marthinal CreditAttribution: marthinal commented
Issue summary: View changes

Updated summary.

marthinal’s picture

marthinal CreditAttribution: marthinal commented
Assigned: Unassigned » marthinal
marthinal’s picture

marthinal CreditAttribution: marthinal commented
FileSize
2204207-4-do-not-test.patch512 bytes

I found why intl pattern was never loaded but I need to show only the correct format(now both php and intl are loaded). We don't need to run the testbot for the moment.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented

Thanks @marthinal for reporting and working on this. The patch looks correct, as far as I'm aware. I hope I get around to testing this later this week.

marthinal’s picture

marthinal CreditAttribution: marthinal commented
Title: Missing format field when translating a date format (when php-intl is enabled). » Missing format field when translating a date format using php-intl.
Assigned: marthinal » Unassigned
FileSize
2204207-6-do-not-test.patch2.15 KB

I was trying to remove php when intl is enabled and... for the moment I don't know the correct place to do that. It's possible to override using an event subscriber but looks not possible from there because we are merging 2 arrays.

I will continue reviewing and working on it but please feel free to work on it if you want. :)

This patch overrides medium format when you are translating but this is only to show the behavior.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Issue tags: +drupaldevdays
FileSize
config-translation-date-format-weirdness.png43.31 KB

So when applying the patch in #4 and selecting a default country in your regional settings, I can reproduce the bug. See attached screenshot. Will dive into this now.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented

So I thought about this a lot today and I think we need to something else here. We really should only be showing one of the date format forms. If php-intl is available you should never see the PHP one. So we should somehow make this a single widget for both keys. Not really sure on how that's going to work.

marthinal’s picture

marthinal CreditAttribution: marthinal commented
Assigned: Unassigned » marthinal
marthinal’s picture

marthinal CreditAttribution: marthinal commented
Status: Needs work » Needs review
FileSize
2204207-10.patch7.35 KB

Let's try this as a first step.

Status: Needs review » Needs work

The last submitted patch, 10: 2204207-10.patch, failed testing.

marthinal’s picture

marthinal CreditAttribution: marthinal commented
Status: Needs work » Needs review
Issue tags: +Needs tests
FileSize
2204207-12.patch8.77 KB

Fixing problems with the test. Now the tests work as expected on my localhost. Let's try again. I think we need a test for this.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented 
+++ b/core/lib/Drupal/Core/Config/TypedConfigManager.php
@@ -191,6 +203,10 @@ public function getDefinitions() {
     }
+    // We only need one date format (PHP or INTL).
+    if (array_key_exists('system.date_format.*', $this->definitions)) {
+      $this->moduleHandler->alter('date_format_pattern', $this->definitions);
+    }

This shouldn't be about date format, the typed config manager does not need to know about that.

So just always call the alter and say something like "// Allow to alter the schema."

And the alter hook should then probably be called config_schema.

Does this work to fix the problem?

So yes, this is what I was suggested, but we need reviews from the config schema people.

marthinal’s picture

marthinal CreditAttribution: marthinal commented
FileSize
interdiff-2204207-12-14.txt1.43 KB
2204207-14.patch8.56 KB

Thanks @Berdir. Rerolled.

marthinal’s picture

marthinal CreditAttribution: marthinal commented
FileSize
2204207-15-only-test-should-fail.patch4.72 KB
2204207-15.patch13.7 KB

I have modified a little bit a test to change the country by default to ES and then verify if the date format is correct(intl when ES is the country by default and php pattern when no country is selected).

There is another bug here. When you add a new date format custom (simply add a date format to reproduce...) and theres no country selected by default in our reginal settings(php pattern in this case). Then we change to spain the default ,country on regional settings and try to translate a custom format (previously added). there is no form so the test fails. This is because when we are creating the date format on submit we add only php pattern and when we build the form there is no pattern for intl.

I fixed this bug on my patch too.

let's take a look at the test results.

The last submitted patch, 15: 2204207-15-only-test-should-fail.patch, failed testing.

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾Seville 💃, Andalusia 🇪🇸, UTC+2 🇪🇺
CreditAttribution: penyaskito commented 
  1. +++ b/core/lib/Drupal/Core/Config/TypedConfigManager.php
@@ -53,6 +54,13 @@ class TypedConfigManager extends PluginManagerBase implements TypedConfigManager
+  * @var \Drupal\Core\Language\LanguageManagerInterface

    Wrong copypaste 

  2. +++ b/core/modules/config_translation/lib/Drupal/config_translation/Tests/ConfigTranslationUiTest.php
@@ -388,40 +388,73 @@ public function testDateFormatTranslation() {
+      //'' => 'None'

    Remove it if we are not going to use that?

Otherwise I think it's ready to RTBC.

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾Seville 💃, Andalusia 🇪🇸, UTC+2 🇪🇺
CreditAttribution: penyaskito commented
Status: Needs review » Needs work
Issue tags: -Needs tests
JeroenT’s picture

JeroenT
Primary language Dutch
Location 🇧🇪
CreditAttribution: JeroenT commented 
+   * @param \Drupal\Core\Extension\ModuleHandlerInterface $module_handler
+   *   The modujle handler.

Typo, modujle should be module.

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾Seville 💃, Andalusia 🇪🇸, UTC+2 🇪🇺
CreditAttribution: penyaskito commented
Assigned: marthinal » penyaskito

Rerolled #15.

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾Seville 💃, Andalusia 🇪🇸, UTC+2 🇪🇺
CreditAttribution: penyaskito commented
Status: Needs work » Needs review
FileSize
2204207-20.patch13.65 KB

Forgot to attach it -facepalm-

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾Seville 💃, Andalusia 🇪🇸, UTC+2 🇪🇺
CreditAttribution: penyaskito commented
Assigned: penyaskito » Unassigned
FileSize
interdiff.20-22.txt1.97 KB
2204207-22.only-test.patch5.45 KB
2204207-22.patch13.65 KB

Fixed 17.1 and 19.
About 17.2, the line shouldn't be commented or we are not testing that case. And if it wasn't necessary, we don't need the loop at all.
Let's see what the bot thinks.

Status: Needs review » Needs work

The last submitted patch, 22: 2204207-22.patch, failed testing.

The last submitted patch, 22: 2204207-22.only-test.patch, failed testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Related issues: +#2276183: Date intl support is broken, remove it

#2276183: Date intl support is broken, remove it might be a cleaner solution to this problem.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs work » Closed (duplicate)
Issue tags: -drupaldevdays +D8MI, +language-content

Yeah #2276183: Date intl support is broken, remove it solves this by removing intl support altogether.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

#2276183: Date intl support is broken, remove it landed, so this is resolved. :)

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented

Yeah, I agree that that's better for now. Sorry @marthinal for all your time spent on this :-/ !