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
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 |
Comment | File | Size | Author |
---|---|---|---|
#31 | 2137595-31.patch | 2.51 KB | idebr |
#31 | 2137595-31.fail_.patch | 2.06 KB | idebr |
Comments
Comment #1
olli CreditAttribution: olli commentedChanged $node_type->type to $node_type->label().
Comment #3
olli CreditAttribution: olli commentedComment #4
hass CreditAttribution: hass commentedThis test must fail if someone changed the default site name, isn't it?
Comment #5
olli CreditAttribution: olli commentedThanks for the review, fixed!
Comment #6
xjm(Merging "node system" and "node.module" components for 8.x; disregard.)
Comment #7
olli CreditAttribution: olli commentedRerolled. 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.
Comment #8
olli CreditAttribution: olli commentedThat "Artikel erstellen" is still broken.
Comment #9
hass CreditAttribution: hass commentedWhat's wrong?
Comment #10
olli CreditAttribution: olli commentedOh, "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".
Comment #12
olli CreditAttribution: olli commentedI was expecting only one fail. Reloading the config entity in title callback fixes both failures.
Comment #13
jhedstromPatch needs a reroll.
I've added a beta phase evaluation to the issue summary.
Comment #16
hass CreditAttribution: hass commentedThis can be committed until string freeze and need to be before.
Comment #17
mitrpaka CreditAttribution: mitrpaka commentedRe-rolled patch.
Comment #18
idebr CreditAttribution: idebr commentedComment #19
jhedstromThis looks good, and has a test too!
Comment #20
idebr CreditAttribution: idebr commentedI was just testing this, patch no longer applies. Working on a reroll.
Comment #21
idebr CreditAttribution: idebr commentedI 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.
Comment #22
BerdirAnother 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.
Not sure about adding even more test methods to this already very long test, maybe a separate one would be better?
Comment #24
hass CreditAttribution: hass commentedNo that is a different issue. This issue here is about using machine name where the entity name should be used.
Comment #25
BerdirYes, *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.
Comment #26
olli CreditAttribution: olli commentedSomething like this?
Comment #27
Gábor Hojtsy@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.
Comment #28
BerdirNot 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 :)
Comment #29
alexpottNeeds a reroll
Comment #30
Gábor HojtsyNote 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.
Comment #31
idebr CreditAttribution: idebr commentedRerolled the patch against HEAD.
Comment #33
idebr CreditAttribution: idebr commentedBack to RTBC per #28 after reroll
Comment #34
alexpottThe issue summary and title do not match the patch.
Comment #35
idebr CreditAttribution: idebr commentedUpdated the title based on the terminology used in #2405939: use_current_language upcasting option is misleading, it toggles all overrides not just language and #1934152: FormBase::config() and ConfigFormBase::config() work entirely differently, may result in unexpected side effects
Comment #36
idebr CreditAttribution: idebr commentedI 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.
Comment #37
idebr CreditAttribution: idebr commentedComment #38
BerdirLooks 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.
Comment #39
idebr CreditAttribution: idebr commentedThanks for the heads up, Berdir. I added a paragraph on the original report by @hass in the issue summary.
Comment #40
alexpottCommitted eb25a33 and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation to the issue summary.
Comment #42
Gábor HojtsyThanks! 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 :)