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"

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost’s picture

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

Gábor Hojtsy’s picture

Status: Active » Needs review
Issue tags: +Needs tests
FileSize
2.04 KB

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.

dawehner’s picture

Issue tags: +MenuSystemRevamp
FileSize
6.28 KB
8.72 KB

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.

Gábor Hojtsy’s picture

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 :)

dawehner’s picture

Status: Needs work » Needs review
Issue tags: +PHPUnit
FileSize
3.48 KB
12.36 KB

Ha, added some unit tests for that.

Gábor Hojtsy’s picture

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.

dawehner’s picture

PageTitleTest::testRoutingTitle takes care about testing that.

Gábor Hojtsy’s picture

FileSize
460 bytes
12.8 KB

@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 :)

dawehner’s picture

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.

Gábor Hojtsy’s picture

Title: Routing / tabs / contextual links lack way to attach context to UI strings » Routing / 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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
3.66 KB
16.02 KB

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

Gábor Hojtsy’s picture

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.

dawehner’s picture

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.

Gábor Hojtsy’s picture

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 :)

Gábor Hojtsy’s picture

Status: Needs review » Needs work
andypost’s picture

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

andypost’s picture

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

dawehner’s picture

Status: Needs work » Needs review
FileSize
17.57 KB

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.

Gábor Hojtsy’s picture

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. :)

Gábor Hojtsy’s picture

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 :)

dawehner’s picture

Great!

Gábor Hojtsy’s picture

Issue summary: View changes

Updated issue summary.

Gábor Hojtsy’s picture

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.

Gábor Hojtsy’s picture

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.

Gábor Hojtsy’s picture

Issue summary: View changes

Updated summary.

ursula’s picture

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).

ursula’s picture

Rerolled patch attached.

ursula’s picture

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.

ursula’s picture

Issue summary: View changes

Added related issues to issue summary

alexpott’s picture

Title: Routing / tabs / actions lack way to attach context to UI strings » Change 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

andypost’s picture

Status: Active » Needs review
FileSize
1.04 KB
+++ 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!

Gábor Hojtsy’s picture

@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?

Gábor Hojtsy’s picture

Title: Change notice: Routing / tabs / actions lack way to attach context to UI strings » Routing / 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).

dawehner’s picture

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.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Yay, less code. :)

Committed and pushed to 8.x. Thanks!

webchick’s picture

Issue summary: View changes

Updated issue summary.

Gábor Hojtsy’s picture

Status: Fixed » Closed (fixed)

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