Problem/Motivation

The title on the node/add/[node type] form uses the override-free (eg. not localized) node type configuration to create the page title. This should be the overridden configuration (eg. localized).

The original report by @hass where the the machine name of the node type was used instead of the label was already fixed in the conversion in #1987762: Convert node_add_page() to a new style controller. In this issue, the label is updated to use the correct configuration to generate a localized title.

Proposed resolution

Use the overridden configuration to display the localized node type label when generating the node form page title.

Remaining tasks

Write patch.

User interface changes

String change

API changes

None

Original report by @hass

I have found a string that causes translation issues. The problem here is case. node type is a technical key that may be correct lowercase in English, but not in German at all.

NodeController.php

  public function addPageTitle(NodeTypeInterface $node_type) {
    return $this->t('Create @name', array('@name' => $node_type->type));
  }

In English this is Create article where it is in German article erstellen, but the German need to be Artikel erstellen. The fails are ucfirst is missing and human readable name is not used.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug
Unfrozen changes Unfrozen because it only changes translated UIs to display the translated node type name instead of the untranslated machine name
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

olli’s picture

Status: Active » Needs review
FileSize
1.32 KB
1.95 KB

Changed $node_type->type to $node_type->label().

Status: Needs review » Needs work

The last submitted patch, 1: drupal-2137595-1--fail.patch, failed testing.

olli’s picture

Status: Needs work » Needs review
hass’s picture

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeTypeTest.php
@@ -51,17 +51,18 @@ function testNodeTypeGetFunctions() {
+    $this->assertTitle(t('Create @name', array('@name' => $type->label())) . ' | Drupal');

This test must fail if someone changed the default site name, isn't it?

olli’s picture

FileSize
2.19 KB
1.04 KB

Thanks for the review, fixed!

xjm’s picture

Component: node.module » node system

(Merging "node system" and "node.module" components for 8.x; disregard.)

olli’s picture

FileSize
2.19 KB

Rerolled. The original problem was fixed in #1987762: Convert node_add_page() to a new style controller. This adds a test for it and uses $node_type->label() instead of $node_type->name.

olli’s picture

Status: Needs review » Needs work

That "Artikel erstellen" is still broken.

hass’s picture

What's wrong?

olli’s picture

Status: Needs work » Needs review
Issue tags: +D8MI, +language-config
FileSize
2.03 KB

Oh, "Artikel erstellen" works if I uncheck "Use the administration theme when editing or creating content" at admin/appearance. Having that checked I get "Article erstellen".

Status: Needs review » Needs work

The last submitted patch, 10: 2137595-fail.patch, failed testing.

olli’s picture

Status: Needs work » Needs review
FileSize
2.74 KB
725 bytes

I was expecting only one fail. Reloading the config entity in title callback fixes both failures.

jhedstrom’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Needs reroll

Patch needs a reroll.

I've added a beta phase evaluation to the issue summary.

hass queued 12: 2137595-12.patch for re-testing.

The last submitted patch, 12: 2137595-12.patch, failed testing.

hass’s picture

Issue tags: +String freeze

This can be committed until string freeze and need to be before.

mitrpaka’s picture

Status: Needs work » Needs review
FileSize
2.77 KB
1.06 KB

Re-rolled patch.

idebr’s picture

Issue tags: -Needs reroll
jhedstrom’s picture

Status: Needs review » Reviewed & tested by the community

This looks good, and has a test too!

idebr’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

I was just testing this, patch no longer applies. Working on a reroll.

idebr’s picture

Status: Needs work » Needs review
Issue tags: -String freeze, -Needs reroll
FileSize
2.06 KB
764 bytes
2.77 KB

I updated the tests with the changes from #2396739: Clean-up config_translation module test members - ensure property definition and use of camelCase naming convention. Did a manual test as well, patch works as advertised :)

The 'string freeze' tag does not apply, since the translatable string has remained the same.

Berdir’s picture

Assigned: Unassigned » Gábor Hojtsy

Another example where the automated translation prevention is wrong.

See #2136559: Config entity admin lists show configuration with overrides (eg. localized), instead of that workaround, I think we should get that flag for not translating config entities in, possibly in a separate issue and then fix that here.

Let's try to get Gabor in here.

+++ b/core/modules/config_translation/src/Tests/ConfigTranslationUiTest.php
@@ -379,6 +382,34 @@ public function testContactConfigEntityTranslation() {
+   * Tests the node type translation.
+   */
+  public function testNodeTypeTranslation() {
+    $type = Unicode::strtolower($this->randomMachineName(16));
+    $name = $this->randomString();
+    $this->drupalLogin($this->adminUser);
+    $this->drupalCreateContentType(array('type' => $type, 'name' => $name));
+
+    // Translate the node type name.
+    $langcode = $this->langcodes[0];

Not sure about adding even more test methods to this already very long test, maybe a separate one would be better?

The last submitted patch, 21: 2137595-21.fail_.patch, failed testing.

hass’s picture

No that is a different issue. This issue here is about using machine name where the entity name should be used.

Berdir’s picture

Yes, *and* using the manually loaded node type (even though you have the node type already available), because the upcaster prevents the entity from getting translated values because we're on an admin path. And having that feature built into the routing/upcasting API would prevent that we have to add that logic here.

olli’s picture

Gábor Hojtsy’s picture

Assigned: Gábor Hojtsy » Unassigned
Issue tags: +sprint

@olli: I think that fix makes a lot of sense. We want the content type upcast in the translated form here. I don't have strong feeling for where we include the test, it is fine where it is or in a separate class also.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Not feeling strongly about test method, leaving that to the core committers to decide then.

Didn't even know that we already have this functionality, I thought it was part of the help config entity patch. Nice, looks much better :)

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Needs a reroll

Gábor Hojtsy’s picture

Note that the use_current_language option is misleadingly named (not a fault of this patch), it toggles all overrides. See #2405939: use_current_language upcasting option is misleading, it toggles all overrides not just language. Should be possible to independently resolve from this one but I used this use case in the updated comments.

idebr’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
2.06 KB
2.51 KB

Rerolled the patch against HEAD.

The last submitted patch, 31: 2137595-31.fail_.patch, failed testing.

idebr’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC per #28 after reroll

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update

The issue summary and title do not match the patch.

idebr’s picture

Title: 'Create @name' uses machine name and not human-readable name » 'Create @name' page title uses override-free configuration (eg. not localized) instead of the overridden configuration (eg. localized)
Status: Needs work » Needs review
idebr’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

I moved the 'Problem/motivation' to the 'Original report by @hass' in the issue summary and updated the 'Problem/motivation' with a summary that matches the changes in the patch.

idebr’s picture

Issue summary: View changes
Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Looks OK to me, thanks for updating. Maybe add a note that the originally reported problem was fixed in #1987762: Convert node_add_page() to a new style controller.

idebr’s picture

Issue summary: View changes

Thanks for the heads up, Berdir. I added a paragraph on the original report by @hass in the issue summary.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed eb25a33 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed eb25a33 on 8.0.x
    Issue #2137595 by olli, idebr, mitrpaka: 'Create @name' page title uses...
Gábor Hojtsy’s picture

Issue tags: -sprint

Thanks! Let's resolve the param terminology in #2405939: use_current_language upcasting option is misleading, it toggles all overrides not just language to make this possibility more evident :)

Status: Fixed » Closed (fixed)

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