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.

Comments

tstoeckler created an issue. See original summary.

maxocub’s picture

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

Issue summary: View changes
FileSize
26.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

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

maxocub’s picture

Status: Active » Needs review
FileSize
1.25 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,579 pass(es), 6 fail(s), and 71 exception(s). View
40.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

Status: Needs work » Needs review
FileSize
19.07 KB
1.3 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,598 pass(es), 0 fail(s), and 1 exception(s). View
maxocub’s picture

FileSize
416 bytes

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

tstoeckler’s picture

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

Status: Needs work » Needs review
FileSize
3.15 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,629 pass(es), 0 fail(s), and 1 exception(s). View
3.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

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

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

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

maxocub’s picture

Assigned: Unassigned » maxocub

@jaxxed: yes I am! Assigned.

tstoeckler’s picture

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

FileSize
7.43 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,680 pass(es). View
7.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

Status: Needs work » Needs review
rodrigoaguilera’s picture

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

@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

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

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

Status: Needs review » Reviewed & tested by the community

Great!

maxocub’s picture

Issue summary: View changes
FileSize
41.51 KB
45.48 KB

Added screen shots to the summary.

Gábor Hojtsy’s picture

Looks good to me too, thanks!

alexpott’s picture

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

Issue tags: -sprint

Yay, thanks all!

Status: Fixed » Closed (fixed)

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

maxocub’s picture

maxocub’s picture

Version: 8.0.x-dev » 9.x-dev
Assigned: maxocub » Unassigned