Problem

How does tour module deal with languages? For one it has language codes in the .yml filenames and it has langcode properties in the tour files, but then it does not actually export a tour langcode in .yml (fixing that in #1935000: Some configuration entities are created as in language unknown). I don't understand why does it include a language code in the yml filename? If it would want to ship with translations for tours, it should use the config translation system (locale.config.fr.$tour_name.yml for French, etc.), that would only contain translations of parts of steps that are to be translated.

Proposal

Figure out what languages do in filenames and what is the plan for tour translations.

Files: 
CommentFileSizeAuthor
#17 tour-lang-1935120.17.interdiff.txt824 byteslarowlan
#17 tour-lang-1935120.17.patch8.37 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 53,024 pass(es). View
#12 tour-lang-1935120.12.interdiff.txt517 byteslarowlan
#12 tour-lang-1935120.12.patch7.56 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 53,024 pass(es), 1 fail(s), and 0 exception(s). View
#10 tour-lang-1935120.10.interdiff.txt679 byteslarowlan
#10 tour-lang-1935120.10.patch7.59 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 53,066 pass(es). View
#7 tour-lang-1935120.7.patch7.36 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 52,406 pass(es). View

Comments

Gábor Hojtsy’s picture

Category: task » bug

Probably more likely a bug(?).

tim.plunkett’s picture

It is exported in the yaml, see http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/modul...

But yes, putting it in the filename is incorrect.

larowlan’s picture

Thanks for picking this up

larowlan’s picture

Also, we might need to revisit these hunks too - they might not be correct in light of the new config context.
http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/modul...

 // @todo replace this with http://drupal.org/node/1918768 once it is committed.
 114   $path = current_path();
 115   $langcode = language(LANGUAGE_TYPE_CONTENT)->langcode;
 116   $tour_items = array();
 117   // Load all of the items and match on path.
 118   $tour_ids = entity_query('tour')
 119     ->condition('langcode', $langcode)
 120     ->execute();
 121   $tours = entity_load_multiple('tour', $tour_ids);

And the tests include an italian tour, which may now need to use the context system
See the three files here http://drupalcode.org/project/drupal.git/tree/refs/heads/8.x:/core/modul...

larowlan’s picture

Also, this should probably use LANGUAGE_TYPE_INTERFACE, not content

larowlan’s picture

Assigned: Unassigned » larowlan

attempting to get my head around the new overrides

larowlan’s picture

Assigned: larowlan » Gábor Hojtsy
Status: Active » Needs review
FileSize
7.36 KB
PASSED: [[SimpleTest]]: [MySQL] 52,406 pass(es). View

So this takes care of renaming and cleaning up the yml files.
Doesn't address #4 or #5, assigning to Gabor to get his input on that code.
Fwiw the new overrides just work (in local testing)!

Gábor Hojtsy’s picture

Status: Needs review » Needs work

The config overrides are not new, they are available in their current form since 2012 August: http://drupal.org/node/1646580#comment-6375296. It is true it only got documented recently in combination with contexts, since without contexts, they were kind of just hanging in there :)

So indeed, the right use of yml vs. language would be to

1. Not include a language code in the filename/key.
2. Include a top level "langcode: " key in the file that would specify the language.
3. Add any translations of tours in a locale.config.$langcode.$original_name.yml file.
4. Only include values that need overrides / translation in the locale.config.* file.

BTW It is suggested that shipped tours ship in English first, but if they provide a langcode for another language as a top level element in the yml, they could be any language practically (and translated to other languages from there). So contrib distros or modules that are written for a specific foreign market can ship with foreign language tours easily, just don't include any langcode in the filename but include a correct top level langcode element. For core, I think the only thing that makes sense is tours/tips are shipped in English first.

Looking at the patch locale.config.it.tour.tour.tour-test.yml seems to include all values, which is not correct. It should only include the translated values (and if they are lower in the hierarchy, any parents that are required to reproduce the structure). When the translated config is loaded, the translations are deep-merged with the original file.

larowlan’s picture

Assigned: Gábor Hojtsy » larowlan

Thanks Gábor, will sort tomorrow.

larowlan’s picture

Status: Needs work » Needs review
FileSize
7.59 KB
PASSED: [[SimpleTest]]: [MySQL] 53,066 pass(es). View
679 bytes

This removes the extra stuff that isn't translated from the italian translation in the test tour.
Tests still pass locally.
Props to whoever wrote this override system, it just works ™

Gábor Hojtsy’s picture

Starting to look pretty good :) Is your test loading the page in Italian and checking the tour that way to check that Italian works? There does not seem to be any related changes here?!

+++ b/core/modules/tour/tests/tour_test/config/locale.config.it.tour.tour.tour-test.ymlundefined
@@ -0,0 +1,7 @@
+id: tour-test
+label: Tour test italian
+langcode: it
+tips:
+  tour-test-1:
+    label: La pioggia cade in spagna
+    body: Per lo più in pianura.

The id and the langcode should not be in this file either. The id is not different from the base file again and the langcode is in the filename already.

larowlan’s picture

FileSize
7.56 KB
FAILED: [[SimpleTest]]: [MySQL] 53,024 pass(es), 1 fail(s), and 0 exception(s). View
517 bytes

Removes the langcode and id from the override file.
There are already tests for the presence of the italian tip when loading the page in Italian - see http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/modul... onwards.

So if the tests pass, the overrides are working as expected, although I haven't tested it manually since #7.

Status: Needs review » Needs work

The last submitted patch, tour-lang-1935120.12.patch, failed testing.

Gábor Hojtsy’s picture

Now it would not work since the code wants to find langcode: it in a file, but that is not there anymore I guess. So the current code would find tours that are initially written for Italian but not those which were translated to Italian.

larowlan’s picture

Tests for the win!
So we are using an entity query, with a langcode condition to find the tour entities.
I guess that chunk (as alluded in #4) needs work.

larowlan’s picture

@Gabor - what would be the expected behaviour in this instance:
1) There is a tour for /foo that is declared in English
2) There is no translation of that tour into Italian
3) The user visits /it/foo

Should the English tour be displayed?

larowlan’s picture

Status: Needs work » Needs review
FileSize
8.37 KB
PASSED: [[SimpleTest]]: [MySQL] 53,024 pass(es). View
824 bytes

So this removes the language condition from the entity query, and instead just loads all tour objects.
Tests pass again.

Gábor Hojtsy’s picture

Yeah, the locale.conf.* files are not config entities, so I'm not sure why would that entity query find them. Sounds like a bug with entity queries. Should be its own issue.

larowlan’s picture

Hi @Gabor, the original translation is a config entity. But the existing code (fixed in the latest patch) was filtering them to the current language, which as you rightly pointed out in #14 meant we didn't find translations. So the latest patch at #17 removes the language condition from the entity query, and loads all tours for that path, if there is a translation - it is used (as tested by the existing tests) but if no translation exists, the english (or original) version is shown, I think this is the desired behaviour and is consistent with other patterns in core.
If you agree with this behaviour, I think this patch is ready to go.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

I agree this is the expected behaviour. Looks cleaner and uses the locale override system nicely.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks!

Gábor Hojtsy’s picture

Issue tags: -sprint

Superb, thanks!

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