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:
Mistranslation as it appears on localize.drupal.org:
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.
Related Issues
#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"
Comment | File | Size | Author |
---|---|---|---|
#30 | 2114069-30.patch | 1.04 KB | andypost |
#27 | drupal8.routing-system.2114069-27.patch | 14.61 KB | ursula |
#20 | title_context-2114069-20.patch | 17.57 KB | dawehner |
#12 | title_context-2114069-12.patch | 16.02 KB | dawehner |
#12 | interdiff.txt | 3.66 KB | dawehner |
Comments
Comment #1
andypostAwesome idea!!! Now all UI strings could be easily tracked with their "context" because we have
$this->t()
on their base classesComment #2
Gábor HojtsyNote 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.
Comment #3
dawehnerI 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.
Comment #4
Gábor HojtsyLol, 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 :)
Comment #5
dawehnerHa, added some unit tests for that.
Comment #6
Gábor HojtsyLooks 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.
Comment #7
dawehnerPageTitleTest::testRoutingTitle takes care about testing that.
Comment #8
Gábor Hojtsy@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 :)
Comment #9
dawehnerOH 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.
Comment #11
Gábor HojtsyAll 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.
Comment #12
dawehnerI will hate myself when #2068471: Normalize Controller/View-listener behavior with a Page object has to be rerolled again ...
Comment #13
Gábor HojtsyManually 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.
Comment #14
dawehnerThe 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.
Comment #16
Gábor HojtsyYeah 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 :)
Comment #17
Gábor HojtsyComment #18
andypostThere'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
Comment #19
andypostFrom Russian language we need View(s) menu item to have separate context as well, suppose other languages affected too ;)
Comment #20
dawehnerThe 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.
Comment #21
Gábor HojtsyMoving 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. :)
Comment #22
Gábor HojtsyAll righto, given no concerns from all the leads I pinged in 3 days, I think this is good to go :)
Comment #23
dawehnerGreat!
Comment #23.0
Gábor HojtsyUpdated issue summary.
Comment #24
Gábor HojtsyUpdated 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.
Comment #25
Gábor HojtsyAlso 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.
Comment #25.0
Gábor HojtsyUpdated summary.
Comment #26
ursula CreditAttribution: ursula commentedPatch doesn't apply to current head. I am going to reroll it using instructions at https://drupal.org/patch/reroll (today at badcamp).
Comment #27
ursula CreditAttribution: ursula commentedRerolled patch attached.
Comment #28
ursula CreditAttribution: ursula commentedUnassigned 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.
Comment #28.0
ursula CreditAttribution: ursula commentedAdded related issues to issue summary
Comment #29
alexpottCommitted b376379 and pushed to 8.x. Thanks!
We need to update the change notices
Comment #30
andypostThis change is unneeded! the title is set in code above, this hunk for admin only!
Comment #31
Gábor Hojtsy@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?
Comment #32
Gábor Hojtsyhttps://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).
Comment #33
dawehnerThis 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.
Comment #34
webchickYay, less code. :)
Committed and pushed to 8.x. Thanks!
Comment #34.0
webchickUpdated issue summary.
Comment #35
Gábor HojtsyYay, thanks! See you in #2120235: Regression: routing / tabs / actions / contextual links lack way to attach replacement arguments to UI strings :)