Hat tip to @jhodgdon for finding this.

Problem/Motivation

The title label of node types cannot be translated currently using the Config Translation module.

Proposed resolution

Node types use base field overrides to manage the title label and integrate them with the node type form in a custom way. Therefore they should take care of adding the respective config objects to the node type translation form.

A dedicated NodeTypeMapper should be added for this.

Remaining tasks

User interface changes

Adds a field to translate the node type title label.
before
after

API changes

None.

Data model changes

None.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category It's a bug since the node type title label should be translatable from the UI.
Issue priority Not critical because it doesn't break anything.
Unfrozen changes Unfrozen because it only add a field to translate the node type title label on the translate tab on the node type page.
Prioritized changes The main goal of this issue is usability because otherwise the node type title label is not translatable from the UI.
Disruption Undisruptive.
CommentFileSizeAuthor
#27 after.png45.48 KBmaxocub
#27 before.png41.51 KBmaxocub
#20 interdiff.txt7.25 KBmaxocub
#20 translate_node_type_title_label-2571337-20.patch7.43 KBmaxocub
#12 interdiff.txt3.95 KBmaxocub
#12 translate_node_type_title_label-2571337-12.patch3.15 KBmaxocub
#8 interdiff.txt416 bytesmaxocub
#7 translate_node_type_title_label-2571337-7.patch1.3 KBmaxocub
#7 interdiff.txt19.07 KBmaxocub
#5 article_translation_form.png40.58 KBmaxocub
#5 translate_node_type_title_label-2571337-5.patch1.25 KBmaxocub
#3 Selection_002.png26.96 KBrodrigoaguilera
#2 Manage fields | drupal.local - Iceweasel_001.png61.75 KBmaxocub
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tstoeckler created an issue. See original summary.

maxocub’s picture

maxocub CreditAttribution: maxocub commented
FileSize
Manage fields | drupal.local - Iceweasel_001.png61.75 KB

Am I understanding correctly that we want to add a line for the 'Title' on the 'Manage fields' page of content types thus making available the 'Translate' link?

rodrigoaguilera’s picture

rodrigoaguilera
Primary language Spanish
Location Madrid
CreditAttribution: rodrigoaguilera commented
Issue summary: View changes
FileSize
Selection_002.png26.96 KB

At the interface level I think it should be in this screenshot rather than adding a row there.

with the rest of node type configurations.

maxocub’s picture

maxocub CreditAttribution: maxocub commented

Yes, you're right, I cleared that up with Gabor earlier, thanks.

maxocub’s picture

maxocub CreditAttribution: maxocub commented
Status: Active » Needs review
FileSize
translate_node_type_title_label-2571337-5.patch1.25 KB
article_translation_form.png40.58 KB

I added a NodeTypeMapper where I add the title label to the translation form. Thanks @tstoeckler for your help!

Two things to note:

1) The title label is displayed on the translation form only if it has been edited first because of those lines in NodeTypeForm.php (lines 243-245):

if ($title_field->getLabel() != $title_label) {
  $title_field->getConfig($type->id())->setLabel($title_label)->save();
}

2) There is also a 'Help text' field that gets added, and I don't know where it comes from (see screenshot). Maybe you have an idea @gábor-hojtsy? It's not on the node type edit form and if I set a translation for it, I don't see it when I export the configuration.

Article translation form

Status: Needs review » Needs work

The last submitted patch, 5: translate_node_type_title_label-2571337-5.patch, failed testing.

maxocub’s picture

maxocub CreditAttribution: maxocub commented
Status: Needs work » Needs review
FileSize
interdiff.txt19.07 KB
translate_node_type_title_label-2571337-7.patch1.3 KB
maxocub’s picture

maxocub CreditAttribution: maxocub commented
FileSize
interdiff.txt416 bytes

The last submitted patch, 5: translate_node_type_title_label-2571337-5.patch, failed testing.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler as a volunteer commented
Status: Needs review » Needs work
Issue tags: +Needs tests
Related issues: +#2571655: ConfigNamesMapper::hasTranslatable has flawed logic

This looks really great, thanks a lot for taking this on! I have almost nothing to complain about, only a couple tiny things. What this issue will need, though, is some test coverage. I.e. a test that asserts that the title label field is found on the translation page and also that the correct title label actually shows up on the node edit form. Marking needs work (mostly) for that and tagging accordingly.

Some comments on the code:

  1. +++ b/core/modules/node/src/NodeTypeMapper.php
@@ -0,0 +1,29 @@
+namespace Drupal\node;

    This is the first time a moduel other that config_translation is providing a translation mapper, so there's no standard to go by (yet), but I personally dislike "polluting" the top-level namespace of modules. My proposal would be to move this class into the \Drupal\node\ConfigTranslation namespace. Thoughts? 

  2. +++ b/core/modules/node/src/NodeTypeMapper.php
@@ -0,0 +1,29 @@
+ * Configuration mapper for node types.

    This should be a complete sentence. We often use something like "Provides a ..." (i.e. "Provides a configuration mapper for node types.") 

  3. +++ b/core/modules/node/src/NodeTypeMapper.php
@@ -0,0 +1,29 @@
+    $this->addConfigName("core.base_field_override.node.$node_type.title");

    This makes a lot of sense, great work!

    @maxocub asked me whether we should add the other base field overrides (promoted, ...) as well. My initial response was "no", because they don't provide any translatable settings. However - like any config entity - they support third party settings, so they could be extended in contrib, with translatable information. So - having thought about it - I actually would suggest to add them here as well.

    However, that will currently not work due to #2571655: ConfigNamesMapper::hasTranslatable has flawed logic, so let's leave that out for now.

    (I.e. nothing to do for this bullet point.)

The last submitted patch, 7: translate_node_type_title_label-2571337-7.patch, failed testing.

maxocub’s picture

maxocub CreditAttribution: maxocub commented
Status: Needs work » Needs review
FileSize
translate_node_type_title_label-2571337-12.patch3.15 KB
interdiff.txt3.95 KB

Here's a new patch with a test and the two changes from #10.

Status: Needs review » Needs work

The last submitted patch, 12: translate_node_type_title_label-2571337-12.patch, failed testing.

The last submitted patch, 12: translate_node_type_title_label-2571337-12.patch, failed testing.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler as a volunteer commented
Issue tags: -Needs tests

Awesome work on the test, that is exactly what we need to test, very, very nicely done.

I have two minor comments before this is RTBC from my point of view (once it's green).

  1. --- a/core/modules/config_translation/src/Tests/ConfigTranslationUiTest.php
+++ b/core/modules/config_translation/src/Tests/ConfigTranslationUiTest.php

@@ -432,6 +432,33 @@ public function testNodeTypeTranslation() {
+  public function testNodeTypeTitleLabelTranslation() {

    Instead of adding this to the already really convoluted ConfigTranslationUiTest I think we should create a new NodeTypeTranslationTest in Node module. 

  2. +++ b/core/modules/config_translation/src/Tests/ConfigTranslationUiTest.php
@@ -432,6 +432,33 @@ public function testNodeTypeTranslation() {
+    $this->drupalGet("node/add/$type");
...
+    $this->drupalGet("fr/node/add/$type");

    Instead of hardcoding the translated path here, you should pass ['language' => new Language(['id' => 'fr'])] into $options.

maxocub’s picture

maxocub CreditAttribution: maxocub commented

About #15/1, beside moving my new test to the new file, I guess I should also move 'testNodeTypeTranslation' there?

I still find it pretty weird that we have to edit the title label first to be able to translate it. Shouldn't we improve that. I'm still trying to figure out how to get rid of the exception from the tests, and it seems to be related.

jaxxed’s picture

jaxxed CreditAttribution: jaxxed at Wunder commented

@maxocub are you still on this? (it is currently unassigned)

maxocub’s picture

maxocub CreditAttribution: maxocub commented
Assigned: Unassigned » maxocub

@jaxxed: yes I am! Assigned.

tstoeckler’s picture

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

About #15/1, beside moving my new test to the new file, I guess I should also move 'testNodeTypeTranslation' there?

Yes, please!! :-) Anything that makes this terrible (because terribly long to run) test smaller is very much appreciated.

maxocub’s picture

maxocub CreditAttribution: maxocub commented
FileSize
translate_node_type_title_label-2571337-20.patch7.43 KB
interdiff.txt7.25 KB

#15/1:
I put the two node type tests in the new 'NodeTypeTranslationTest.php'

#15/2:
I haven't succeeded to put the language into the options of drupalGet, I don't know why, it always give me the English path. I removed the hardcoded 'fr' and used $langcode instead, I don't know if it makes it better...

About the exception from the earlier test results, I modified the test 'testNodeTypeTranslation' to edit the node title before trying to add a new translation. This is a hack around the fact that we can't translate a title label unless it has been modified before.

maxocub’s picture

maxocub CreditAttribution: maxocub commented
Status: Needs work » Needs review
rodrigoaguilera’s picture

rodrigoaguilera
Primary language Spanish
Location Madrid
CreditAttribution: rodrigoaguilera commented
Status: Needs review » Needs work

Great work! Thank you
I tested this on the interface and it makes the label translatable.

+    $this->drupalGet("$langcode/node/add/$type");

Maybe it was not working for you because the language option in drupalGet must be a language object
For example

$language = \Drupal::languageManager()->getLanguage($langcode);
$this->drupalGet('node/8', ['language' => $language]);

https://api.drupal.org/api/drupal/core!modules!simpletest!src!WebTestBas...

Does this need a beta evaluation?

maxocub’s picture

maxocub CreditAttribution: maxocub commented

@rodrigoaguilera: I did use the language object with drupalGet() (and I just tried again) and it always give me the English path anyway.

I tried this:

$this->drupalGet("node/add/$type", ['language' => \Drupal::languageManager()->getLanguage($langcode)]);

and also:

$this->drupalGet("node/add/$type", ['language' => new Language(['id' => $langcode])]);
rodrigoaguilera’s picture

rodrigoaguilera
Primary language Spanish
Location Madrid
CreditAttribution: rodrigoaguilera commented
Issue tags: +Needs beta evaluation

Ok,
I realized you have to call
$this->rebuildContainer();
if you want those 2 calls to work but to put the langcode in the URL is not that ugly and happens in other tests.

So from my point of view The only thing missing is the beta evaluation.

maxocub’s picture

maxocub CreditAttribution: maxocub commented
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs beta evaluation
rodrigoaguilera’s picture

rodrigoaguilera
Primary language Spanish
Location Madrid
CreditAttribution: rodrigoaguilera commented
Status: Needs review » Reviewed & tested by the community

Great!

maxocub’s picture

maxocub CreditAttribution: maxocub commented
Issue summary: View changes
FileSize
before.png41.51 KB
after.png45.48 KB

Added screen shots to the summary.

Gábor Hojtsy’s picture

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

Looks good to me too, thanks!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Nice tests and nice fix. Committed e16b2b0 and pushed to 8.0.x. Thanks!

  • alexpott committed e16b2b0 on 8.0.x 
    Issue #2571337 by maxocub, rodrigoaguilera, tstoeckler: Node type title...
Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Issue tags: -sprint

Yay, thanks all!

Status: Fixed » Closed (fixed)

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

maxocub’s picture

maxocub CreditAttribution: maxocub commented
maxocub’s picture

maxocub CreditAttribution: maxocub commented
Version: 8.0.x-dev » 9.x-dev
Assigned: maxocub » Unassigned
maxocub’s picture

maxocub CreditAttribution: maxocub as a volunteer and commented

Version: 9.x-dev » 9.0.x-dev

The 9.0.x branch will open for development soon, and the placeholder 9.x branch should no longer be used. Only issues that require a new major version should be filed against 9.0.x (for example, removing deprecated code or updating dependency major versions). New developments and disruptive changes that are allowed in a minor version should be filed against 8.9.x, and significant new features will be moved to 9.1.x at committer discretion. For more information see the Allowed changes during the Drupal 8 and 9 release cycles and the Drupal 9.0.0 release plan.