Problem/Motivation

Now that Drupal 8 downloads real Drupal 8 translations following #1914070: Improve version fallback for install language., we can go and figure out translation problems. I found a big one right away with the "Extend" menu item. Looks like "Extend" is a word used elsewhere as "make it apply for a longer time" at least I assume that may be the case with the "Auto expire" module for example. Drupal 8 menu uses this as "add more components". This is an English word that is ambiguous for translation and would need context to disambiguate. Now the problem is menus / routing / tabs don't allow for such context to be specified.

The very concrete problem at hand is the Hungarian team translated "Extend" as the Hungarian equivalent of "lengthen" (ie. "extend my vacation", instead of "extend" as in "extend the system with more components"). I went to *fix* the translation, but given how wide this applies to, "fixing" it may as well brake it for other uses:

Mistranslation as appears in Drupal 8:
ExtendHun.png

Mistranslation as it appears on localize.drupal.org:
ExtendLDO.png

Proposed resolution

1. Need way to attach context to strings in routing / tabs / contextual links and apply context to at least the string "Extend". As more ambiguous strings are found, apply context to those too. This is covered in this patch.
2. Carry that over to the menu system: #2119551: Add string context support to menu system (ONLY the completion of this would make the "Extend" mistranslation go away and the proper contextualized text to prevail in the menu)

Remaining tasks

- Commit.

User interface changes

None.

API changes

New keys to specify context for strings in routing / tabs / action links.

#2119551: Add string context support to menu system handles carrying this data over to the menu system.
#2120235: Regression: routing / tabs / actions / contextual links lack way to attach replacement arguments to UI strings related regression
#2119497-9: Default local tasks are not to be assumed $route_name . '_tab'
#1926488: Rename toolbar "menu" tab to "manage"

Files: 
CommentFileSizeAuthor
#30 2114069-30.patch1.04 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 59,728 pass(es).
[ View ]
#27 drupal8.routing-system.2114069-27.patch14.61 KBursula
PASSED: [[SimpleTest]]: [MySQL] 59,525 pass(es).
[ View ]
#20 title_context-2114069-20.patch17.57 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 59,637 pass(es).
[ View ]
#12 title_context-2114069-12.patch16.02 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 59,059 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#12 interdiff.txt3.66 KBdawehner
#8 title_context-2114069-8.patch12.8 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] 59,317 pass(es), 8 fail(s), and 0 exception(s).
[ View ]
#8 interdiff.txt460 bytesGábor Hojtsy
#5 title_context-2114069-5.patch12.36 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 59,410 pass(es).
[ View ]
#5 interdiff.txt3.48 KBdawehner
#3 title_context-2114069-3.patch8.72 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 59,378 pass(es).
[ View ]
#3 interdiff.txt6.28 KBdawehner
#2 router-and-tabs-context.patch2.04 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 59,396 pass(es).
[ View ]
ExtendHun.png106.96 KBGábor Hojtsy
ExtendLDO.png187.17 KBGábor Hojtsy

Comments

Awesome idea!!! Now all UI strings could be easily tracked with their "context" because we have $this->t() on their base classes

Status:Active» Needs review
Issue tags:+Needs tests
StatusFileSize
new2.04 KB
PASSED: [[SimpleTest]]: [MySQL] 59,396 pass(es).
[ View ]

Note this is not really a regression, since context was not directly supported on Drupal 7 hook_menu() either (although you could use a title callback to code around that which is also supported in the router system in D8). It is a very apparent problem because now D8 uses a very ambiguous word as a major menu item, which D7 did not do. Oh well :)

It would be a bit of a problem IMHO to need to put in a custom title callback just to get string like 'Extend' have context.

So I not only complain here is some basic starter patch. I think D8 will pick up this one in favor of the system_menu() item still defined for 'Extend'. I don't have an immediate example for a local task that would need context, but there may easily be one :) However included code to context-enable that too.

Obviously will need tests, but first let's look at the concept.

Issue tags:+MenuSystemRevamp
StatusFileSize
new6.28 KB
new8.72 KB
PASSED: [[SimpleTest]]: [MySQL] 59,378 pass(es).
[ View ]

I agree that adding some context is really important.

Sneaky trick to add new features ;)

Let's also add this feature to local actions and extend the test coverage here and there.

Status:Needs review» Needs work

Lol, not really a trick per say. As you can see the Hungarian translation picking the wrong meaning (given no context) in question is from 2010(!), it is just now that this ambiguous word got into a top level Drupal menu in core so this lack of context problem got exposed big time.

Agreed on the proposal for local actions, and the tests look good, but they don't yet test local actions :)

Status:Needs work» Needs review
Issue tags:+phpunit
StatusFileSize
new3.48 KB
new12.36 KB
PASSED: [[SimpleTest]]: [MySQL] 59,410 pass(es).
[ View ]

Ha, added some unit tests for that.

Looks great. We should verify if the "Extend" context works now (the route title theoretically overrides the hook_menu() title I think). If that works, then this looks good to me :) Unless we want to add a specific test for "Extend" that is, but I think the unit tests are sufficient.

PageTitleTest::testRoutingTitle takes care about testing that.

StatusFileSize
new460 bytes
new12.8 KB
FAILED: [[SimpleTest]]: [MySQL] 59,317 pass(es), 8 fail(s), and 0 exception(s).
[ View ]

@dawehner: not 'Extend' itself in that test, right? So I tried this manually, and the title does not seem to take over what is in hook_menu, so the context is not taken into account at all. Not sure what if anything will break if we remove the hook_menu title and only include it in the route (even though description is in hook_menu()). Let's see :)

OH yeah that is a form and by that a bit more tricky. This is indeed a different bug, but for example resolved by #2068471: Normalize Controller/View-listener behavior with a Page object. I remember putting some fix in another patch but for sure I don't remember which one.

In other words, we should not remove the title from hook_menu() as they might be needed for menu links in the future.

Status:Needs review» Needs work

The last submitted patch, title_context-2114069-8.patch, failed testing.

Title:Routing / tabs / contextual links lack way to attach context to UI stringsRouting / tabs / actions lack way to attach context to UI strings
Issue tags:-Contextual links

All right, the question then what can we even do about the concrete 'Extend' case? Because seems like all the added capabilities in routing/tabs/actions will not help.

Status:Needs work» Needs review
StatusFileSize
new3.66 KB
new16.02 KB
FAILED: [[SimpleTest]]: [MySQL] 59,059 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

I will hate myself when #2068471: Normalize Controller/View-listener behavior with a Page object has to be rerolled again ...

Manually testing this does not yet make 'Extend' with the proper translation in context appear in the toolbar :/ Toolbar uses menu_tree_output() which ends up in https://api.drupal.org/api/drupal/core%21includes%21menu.inc/function/_m.... I did not manage to figure out how the menu item gets a title from the route, but any it works off of stored menu items from menu_router and they only support storing one source string which is t()-ed, no contexts. I think the way the menu item is saved is not really in scope for us to fix, it would require to make the menu_router table/code support context as well, which seems like a task on its own. If that method of rendering/storing the menu items though persists, it needs to step up to the task. So I guess we can round out this one and then once committed, can open another major to figure out carrying over this information to the menu system.

The thing is, the routing system is not the menu system, so the menu title is a different title than the routing title, at least in theory.

These links will be menu links at some point, so maybe we need to support somehow the translation bits on the menu links.

Status:Needs review» Needs work
Issue tags:-string context

The last submitted patch, title_context-2114069-12.patch, failed testing.

Status:Needs work» Needs review

Yeah the menu system will need to carry over that context, but I think that would be out of the scope of this patch. So looks like we need to fix this step by step in different systems. All exposed by the choice of the ambiguous word "Expose" vs. the non-ambiguous "Modules". :D

Let's get this step in although it does not fully fix the problem, its a big step! Would be good to get one more set of eyes from the routing folks :)

Status:Needs review» Needs work

There's a special issue #1842850: Untangle menu_link access control from _menu_link_translate() and friends to decouple translation from menu-links, suppose this applicable to tasks & actions too

From Russian language we need View(s) menu item to have separate context as well, suppose other languages affected too ;)

Status:Needs work» Needs review
StatusFileSize
new17.57 KB
PASSED: [[SimpleTest]]: [MySQL] 59,637 pass(es).
[ View ]

The FieldUIRouteTest is a failure because we do actually test the wrong expected title, see #2102125-34: Big Local Task Conversion

Once you have something like #title or _title on the routing drupal_set_title() does not longer override, so we have to adapt the comment case.

We could think about moving the component to something like routing.

Component:user interface text» routing system

Moving component. IMHO this is RTBC as-is, but we'll need to do more in the menu system to get the 'Extend' menu item translate properly at the end in the menu. Would be good to get one more pair of eyes on this from the routing folks. I'll ping some people. :)

Status:Needs review» Reviewed & tested by the community

All righto, given no concerns from all the leads I pinged in 3 days, I think this is good to go :)

Great!

Issue summary:View changes

Updated issue summary.

Updated issue summary as well to make it clear this does not fix the whole underlying issue in itself and opened #2119551: Add string context support to menu system for carrying this over to the menu system which will build on this to use the right text.

Issue tags:+blocker

Also opened #2120235: Regression: routing / tabs / actions / contextual links lack way to attach replacement arguments to UI strings which is a related regression @dawehner pointed out in #2119497-9: Default local tasks are not to be assumed $route_name . '_tab'. Marking this a blocker since it blocks those two other issues from being worked on, one of which is a regression.

Issue summary:View changes

Updated summary.

Assigned:Unassigned» ursula
Issue tags:+Needs reroll

Patch doesn't apply to current head. I am going to reroll it using instructions at https://drupal.org/patch/reroll (today at badcamp).

Issue tags:-Needs tests, -Needs reroll
StatusFileSize
new14.61 KB
PASSED: [[SimpleTest]]: [MySQL] 59,525 pass(es).
[ View ]

Rerolled patch attached.

Assigned:ursula» Unassigned

Unassigned myself and put it back to RTBC - I think since this is just a clean re-roll with no conflicts.
Needs test was added in comment #2, tests were added in several subsequent comments. It looks like to me we have enough tests here.

Issue summary:View changes

Added related issues to issue summary

Title:Routing / tabs / actions lack way to attach context to UI stringsChange notice: Routing / tabs / actions lack way to attach context to UI strings
Status:Reviewed & tested by the community» Active
Issue tags:+Needs change record

Committed b376379 and pushed to 8.x. Thanks!

We need to update the change notices

Status:Active» Needs review
StatusFileSize
new1.04 KB
PASSED: [[SimpleTest]]: [MySQL] 59,728 pass(es).
[ View ]

+++ b/core/modules/comment/lib/Drupal/comment/CommentFormController.php
@@ -115,6 +115,9 @@ public function form(array $form, array &$form_state) {
+      else {
+        $form['#title'] = $this->t('Preview comment');
+      }
     }

This change is unneeded! the title is set in code above, this hunk for admin only!

@andypost: I'm not sure why this was added, but also not sure why to remove it :D It is certainly not in the preview flow, it is in the form builder. @dawehner?

Title:Change notice: Routing / tabs / actions lack way to attach context to UI stringsRouting / tabs / actions lack way to attach context to UI strings
Priority:Major» Normal
Issue tags:-Needs change record

https://drupal.org/node/2123027 is a specific change notice added for this.

Updated https://drupal.org/node/2044515 (local tasks change notice), https://drupal.org/node/2092643 (routes docs, not found appropriate place to update in change notice at https://drupal.org/node/1800686), https://drupal.org/node/2007444 (local actions change notice).

Status:Needs review» Reviewed & tested by the community

This was added to fix the test failures in https://qa.drupal.org/pifr/test/655908 but the big local task conversion fixed that test on a different way, so this can be removed again.

Status:Reviewed & tested by the community» Fixed

Yay, less code. :)

Committed and pushed to 8.x. Thanks!

Issue summary:View changes

Updated issue summary.

Status:Fixed» Closed (fixed)

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